ARM: replace ARMEXIDXSection with EXIDX fragment/piece model

[quic-seaswara]

No description provided.

[quic-areg]

Moving to Fragments is a good idea, but this patch bundles several things that should be their own PR:

1. Replacing ARMEXIDXSection with ARMEXIDXFragment
2. sortEXIDX was heavily rewritten leading to a regression
3. CANTUNWIND entry was silently dropped for the entry symbol
4. Map file change

for 2, this patch corrupts .ARM.exidx for linker scripts that split exidx across multiple rules.:

cat <<\! > 1.c
void _start(void) {}
void f1(void)     {}
void f2(void)     {}
void f3(void)     {}
!
cat <<\! > script.t
SECTIONS {
  .text (0x11000) : { *(.text._start) *(.text.f1) *(.text.f2) *(.text.f3) }
  .ARM.exidx : {
    *(.ARM.exidx.text.f3) *(.ARM.exidx.text.f2)
    *(.ARM.exidx.text.f1) *(.ARM.exidx.text._start)
  }
}
!
clang -target arm-none-eabi -ffunction-sections -c 1.c -o 1.o
ld.eld 1.o -T script.t -e _start -o a.out
llvm-readelf -u a.out

[quic-seaswara]

Moving to Fragments is a good idea, but this patch bundles several things that should be their own PR:

1. Replacing ARMEXIDXSection with ARMEXIDXFragment

ARMEXIDXFragment and sortEXIDX have to go together.

2. sortEXIDX was heavily rewritten leading to a regression

See my comment below.

3. CANTUNWIND entry was silently dropped for the entry symbol

I am not sure if this is needed. I am able to run the tests and it appears there is no test catching this too.

Will see if it is needed and update.

4. Map file change

for 2, this patch corrupts .ARM.exidx for linker scripts that split exidx across multiple rules.:

cat <<\! > 1.c
void _start(void) {}
void f1(void)     {}
void f2(void)     {}
void f3(void)     {}
!
cat <<\! > script.t
SECTIONS {
  .text (0x11000) : { *(.text._start) *(.text.f1) *(.text.f2) *(.text.f3) }
  .ARM.exidx : {
    *(.ARM.exidx.text.f3) *(.ARM.exidx.text.f2)
    *(.ARM.exidx.text.f1) *(.ARM.exidx.text._start)
  }
}
!
clang -target arm-none-eabi -ffunction-sections -c 1.c -o 1.o
ld.eld 1.o -T script.t -e _start -o a.out
llvm-readelf -u a.out

Thanks for your review.

Yes, if you explicitly specify ARM.exidx sections it will definitely be broken. Apparently the SHF_LINK_ORDER support needs to go in and there is a dependency on the patch, and when that lands in, we will get .ARM.exidx sorted accordingly.

I will seperate the map file change to a seperate PR.

[quic-areg]

A few more thoughts, briefly:

ARMEXIDXFragment and sortEXIDX have to go together

Switching to fragments implies a change to sortEXIDX, but not a whole rewrite of 150 lines that introduces new logic. The new changes to sorting logic, offset calculation, etc are beyond the scope of a refactor.

I am not sure if this is needed. I am able to run the tests and it appears there is no test catching this too.

"no test catches this" does not imply that it is or is not needed. In any case, it is a new behavioral change that is beyond the scope of a refactor imo.

[quic-seaswara]

A few more thoughts, briefly:

ARMEXIDXFragment and sortEXIDX have to go together

Switching to fragments implies a change to sortEXIDX, but not a whole rewrite of 150 lines that introduces new logic. The new changes to sorting logic, offset calculation, etc are beyond the scope of a refactor.

I am not sure if this is needed. I am able to run the tests and it appears there is no test catching this too.

"no test catches this" does not imply that it is or is not needed. In any case, it is a new behavioral change that is beyond the scope of a refactor imo.

Yes, thanks for continuing to review this change.

This change by no means is a refactor and needs to be fully tested. This is big change to the overall exidx support.

Though it is important to keep the current design and making sure there are no regressions, we need to move forward by designing it better.

I will make sure that testing does not regress (tests that we have), and the functionality easy to be verified, so the map file changes would be needed as well.

The test case that you mentioned will have to be a follow up based on the comments, that I mentioned to suport SHF_LINK_ORDER.

I think I added this change for reviews early, will convert this to a draft.