[ELF] Handle relocations in synthetic .eh_frame with a non-zero offset within the output section #65966

simpal01 · 2023-09-11T14:11:03Z

When the .eh_frame section is placed at a non-zero
offset within its output section, the relocation
value within .eh_frame are computed incorrectly.

We had similar issue in .ARM.exidx section and it has been
fixed already in https://reviews.llvm.org/D148033.

While applying the relocation using S+A-P, the value
of P (the location of the relocation) is getting wrong.
P is:
P = SecAddr + rel.offset, But SecAddr points to the
starting address of the outputsection rather than the
starting address of the eh frame section within that
output section.

This issue affects all targets which generates .eh_frame
section. Hence fixing in all the corresponding targets it affecting.

smithp35

This will only fix the problem for AArch64, but it also affects other targets. Assuming relocateAlloc is the best place to fix this, then it would need applying in all instances of relocateAlloc.

smithp35 · 2023-09-11T16:13:48Z

lld/ELF/Arch/AArch64.cpp

@@ -770,6 +770,9 @@ void AArch64::relocateAlloc(InputSectionBase &sec, uint8_t *buf) const {
  uint64_t secAddr = sec.getOutputSection()->addr;
  if (auto *s = dyn_cast<InputSection>(&sec))
    secAddr += s->outSecOff;
+  else if (auto *eh = dyn_cast<EhInputSection>(&sec))


I don't think this problem specific to AArch64? It would also affect the other targets that use .eh_frame such as x86_64.

As I understand it this was triggered by the Picolibc linker script (https://github.com/picolibc/picolibc/blob/main/picolibc.ld.in) with extract:

.except_ordered : { *(.gcc_except_table *.gcc_except_table.*) KEEP (*(.eh_frame .eh_frame.*)) *(.ARM.extab* .gnu.linkonce.armextab.*) } >flash AT>flash :text

Which has the .eh_frame sections after .gcc_except_table.

I made a failing test case for x86_64 by editing the output of GNU ld's --verbose and adding a dummy QUAD statement. I've not included the whole ld --verbose output as it is rather long.

/* Insert QUAD(0) to make .eh_frame start at non 0 offset within ouptut section */ .eh_frame : ONLY_IF_RO { QUAD(0); KEEP (*(.eh_frame)) *(.eh_frame.*) }

With the simple test program

#include <stdio.h> int main(void) { try { throw 55; } catch (int i) { printf("Caught int %d\n", i); } catch (...) { printf("Caught generic\n"); } return 0; }

With results:

clang++ -fuse-ld=bfd throw.cpp -o throw.exe -Wl,--script=ld.script ./throw.exe Caught int 55 clang++ -fuse-ld=lld throw.cpp -o throw.exe -Wl,--script=ld.script ./throw.exe [1] 444896 abort (core dumped) ./throw.exe

yeah this affects all other targets (might be X86 and PPC as well).

i could not find the general way of fixing this without modifying target->relocateAlloc.

OK, reading through the code. The .ARM.exidx sections were also InputSections so we could alter their outSecOff to compensate. However EhInputSections are not InputSections so this won't work here. We need to find the outSecOff of the ehframe section (which is usually 0, but with some linker scripts isn't).

I don't know of a legal EhInputSection that would return getParent() == nullptr so it should be possible to remove the nested if.

I suggest use ehIn and ehFrame to make it a bit clearer which sections we are referring to. I've use SyntheticSection as it is what EhInputSection getParent() although that is derived from InputSection so in the code below InputSection would work too.

if (auto *ehIn = dyn_cast<EhInputSection>(&sec) { SyntheticSection *ehFrame = ehIn->getParent(); SecAddr += ehFrame->outSecOff; }

smithp35

Thanks for fixing the commits, that makes it easier to review.

I can see that this would need a fix in relocateAlloc. I think you'll need to update the generic one in Target.cpp as well as the Target specific ones for x86_64 and PPC (and any other I've missed).

You'll need to add x86_64, PPC and at least one other target's (for the generic Target) so that we've covered all the code-paths. By replacing the nops with a common data directive you might be able to make the same file work for all 3. However I think it would still need separate files so that the REQUIRES only had one target.

smithp35 · 2023-09-12T18:01:17Z

lld/ELF/Arch/AArch64.cpp

@@ -770,6 +770,9 @@ void AArch64::relocateAlloc(InputSectionBase &sec, uint8_t *buf) const {
  uint64_t secAddr = sec.getOutputSection()->addr;
  if (auto *s = dyn_cast<InputSection>(&sec))
    secAddr += s->outSecOff;
+  else if (auto *eh = dyn_cast<EhInputSection>(&sec))


OK, reading through the code. The .ARM.exidx sections were also InputSections so we could alter their outSecOff to compensate. However EhInputSections are not InputSections so this won't work here. We need to find the outSecOff of the ehframe section (which is usually 0, but with some linker scripts isn't).

I don't know of a legal EhInputSection that would return getParent() == nullptr so it should be possible to remove the nested if.

I suggest use ehIn and ehFrame to make it a bit clearer which sections we are referring to. I've use SyntheticSection as it is what EhInputSection getParent() although that is derived from InputSection so in the code below InputSection would work too.

if (auto *ehIn = dyn_cast<EhInputSection>(&sec) { SyntheticSection *ehFrame = ehIn->getParent(); SecAddr += ehFrame->outSecOff; }

simpal01 · 2023-09-22T16:16:21Z

lld/ELF/SyntheticSections.cpp

+  // Adding outSecOff as finalizeAddressDependentContent()
+  // may have altered the corresponding outSecOff. This is
+  // required to get the correct PC relative offset.
+  off = off + outSecOff;


on a second thought, adding outSecOff inside the condition might be more sensible.

if ((enc & 0x70) == DW_EH_PE_pcrel) return addr + getParent()->addr + off;

I agree, I think

if ((enc & 0x70) == DW_EH_PE_pcrel) return addr + getParent()->addr + off + outSecOff;

Is easier to understand than modifying off.

smithp35

Can you rename the PR and the commit message to remove [AARCH64] as this is now a general change.

I think with this and your own suggestion of moving the change in getFdePc inside the if statement it will be ready for my approval. We'll want to get MaskRay to take a look too.

smithp35 · 2023-09-28T13:13:58Z

lld/ELF/SyntheticSections.cpp

+  // Adding outSecOff as finalizeAddressDependentContent()
+  // may have altered the corresponding outSecOff. This is
+  // required to get the correct PC relative offset.
+  off = off + outSecOff;


I agree, I think

if ((enc & 0x70) == DW_EH_PE_pcrel) return addr + getParent()->addr + off + outSecOff;

Is easier to understand than modifying off.

simpal01 · 2023-09-30T16:22:58Z

Can you rename the PR and the commit message to remove [AARCH64] as this is now a general change.

I think with this and your own suggestion of moving the change in getFdePc inside the if statement it will be ready for my approval. We'll want to get MaskRay to take a look too.

Done

simpal01 · 2023-09-30T16:29:52Z

@MaskRay Could you please have a look into this patch?

lld/ELF/Arch/AArch64.cpp

lld/test/ELF/eh-frame-nonzero-offset-aarch64.s

MaskRay · 2023-09-30T17:25:57Z

[LLD] lld incorrectly handles .eh_frame when it has a non-zero offset within its output section.

The convention is to use an imperative sentence and omit the trailing dot.

Perhaps [ELF] Handle relocations in synthetic .eh_frame with a non-zero offset within the output section will be better.
(Some patches use [LLD][ELF] or [lld][ELF] but I find [lld] redundant.)

…ro offset within its output section. When the .eh_frame section is placed at a non-zero offset within its output section, the relocation value within .eh_frame are computed incorrectly. We had similar issue in AArch32 and it has been fixed already in https://reviews.llvm.org/D148033. While applying the relocation using S+A-P, the value of P (the location of the relocation) is getting wrong. P is: P = SecAddr + rel.offset, But SecAddr points to the starting address of the outputsection rather than the starting address of the eh frame section within that output section.

… non-zero offset within its output section.

…offset within its output section. Adding outSecOff in eh_frame_hdr section as finalizeAddressDependentContent() may have altered the corresponding outSecOff. This is required to get the correct initial location of each FDE entries.

…o offset within the output section

simpal01 · 2023-10-02T20:50:04Z

[LLD] lld incorrectly handles .eh_frame when it has a non-zero offset within its output section.

The convention is to use an imperative sentence and omit the trailing dot.

Perhaps [ELF] Handle relocations in synthetic .eh_frame with a non-zero offset within the output section will be better. (Some patches use [LLD][ELF] or [lld][ELF] but I find [lld] redundant.)

Done

I plan to define RISCV::relocateAllocate in a subsequent change. Add a test to verify `else if (auto *ehIn = dyn_cast<EhInputSection>(&sec)) secAddr += ehIn->getParent()->outSecOff;`

llvmbot added lld lld:ELF labels Sep 11, 2023

simpal01 requested review from hctim, smithp35 and MaskRay September 11, 2023 14:11

simpal01 marked this pull request as draft September 11, 2023 14:20

simpal01 marked this pull request as ready for review September 11, 2023 15:14

simpal01 self-assigned this Sep 11, 2023

smithp35 reviewed Sep 11, 2023

View reviewed changes

simpal01 force-pushed the main branch 2 times, most recently from d9195a2 to eb4d993 Compare September 12, 2023 10:35

simpal01 requested a review from amilendra September 12, 2023 12:17

smithp35 reviewed Sep 12, 2023

View reviewed changes

simpal01 force-pushed the main branch 2 times, most recently from 6a61837 to 2e3a0de Compare September 22, 2023 15:19

simpal01 commented Sep 22, 2023

View reviewed changes

smithp35 reviewed Sep 28, 2023

View reviewed changes

simpal01 changed the title ~~[LLD][AARCH64] lld incorrectly handles .eh_frame when it has a non-zero offset within its output section.~~ [LLD]lld incorrectly handles .eh_frame when it has a non-zero offset within its output section. Sep 30, 2023

simpal01 force-pushed the main branch from 9978fb2 to a6e2b6c Compare September 30, 2023 16:21

MaskRay reviewed Sep 30, 2023

View reviewed changes

lld/ELF/Arch/AArch64.cpp Outdated Show resolved Hide resolved

MaskRay reviewed Sep 30, 2023

View reviewed changes

lld/test/ELF/eh-frame-nonzero-offset-aarch64.s Outdated Show resolved Hide resolved

MaskRay reviewed Sep 30, 2023

View reviewed changes

lld/test/ELF/eh-frame-nonzero-offset-aarch64.s Outdated Show resolved Hide resolved

MaskRay reviewed Sep 30, 2023

View reviewed changes

lld/test/ELF/eh-frame-nonzero-offset-aarch64.s Outdated Show resolved Hide resolved

simpal01 changed the title ~~[LLD]lld incorrectly handles .eh_frame when it has a non-zero offset within its output section.~~ [ELF] Handle relocations in synthetic .eh_frame with a non-zero offset within the output section Sep 30, 2023

simpal01 added 3 commits October 2, 2023 21:42

fixup! [LLD][AARCH64] lld incorrectly handles .eh_frame when it has a…

897f57f

… non-zero offset within its output section.

fixup! [ELF] Handle relocations in synthetic .eh_frame with a non-zer…

555b611

…o offset within the output section

simpal01 force-pushed the main branch from ea41c84 to 555b611 Compare October 2, 2023 20:48

MaskRay approved these changes Oct 3, 2023

View reviewed changes

simpal01 merged commit 3cde1d8 into llvm:main Oct 3, 2023

[ELF] Handle relocations in synthetic .eh_frame with a non-zero offset within the output section #65966

[ELF] Handle relocations in synthetic .eh_frame with a non-zero offset within the output section #65966

Uh oh!

Conversation

simpal01 commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

simpal01 Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

simpal01 Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

smithp35 Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

simpal01 Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smithp35 Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

simpal01 commented Sep 30, 2023

Uh oh!

simpal01 commented Sep 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaskRay commented Sep 30, 2023

Uh oh!

simpal01 commented Oct 2, 2023

Uh oh!

Uh oh!

simpal01 commented Sep 11, 2023 •

edited

Loading

simpal01 Sep 22, 2023 •

edited

Loading