[PATCH] D78422: [LLD][ELF][ARM] Fix ARM Exidx order for non monotonic section order

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

The patch looks good. I think applying it does not hurt.

> `// RUN:         .text.1 0x80000200 : AT(0x80000200) { *(.text.1) } \`
>  `// RUN:         .text.2 0x80000100 : AT(0x80000100) { *(.text.2) } \`

This is interesting. Does linux/arch/arm have such a use case where `VMA(.text.1) > VMA(.text.2)` with optional `LMA(.text.1) < LMA(.text.2)`? If it does, we probably should let it drop that requirement.

For symbol assignments, lld has such a diagnostic:

  void LinkerScript::setDot(Expr e, const Twine &loc, bool inSec) {
    uint64_t val = e().getValue();
    if (val < dot && inSec)
      error(loc + ": unable to move location counter backward for: " +
            ctx->outSec->name);

the only software I know (unintentionally) making use of this property is seabios but I have a patch to fix that (https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/KBHQU4OLW2THG5Q5F5X7EWTQDJHKNVYC/ )

Not sure how the Linux kernel leverage non-increasing VMAs but the BFD logic does not make me feel comfortable that it can handle various corner cases well:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/ldlang.c;hb=49d1d1f53df552f0592b1cc4cc9a74db70d5ffa7#l5782

> I think that this also affects the standard SHF_LINK_ORDER, I think resolveSHFLinkOrder() probably needs to be moved to a similar place.

My understanding is that when linked-to sections span multiple output sections, the order is unspecified. That said, I agree that respecting the VMA order instead of the section header table order seems more reasonable.
If we are going to support non-increasing VMAs, making generic SHF_LINK_ORDER aligns with .ARM.exidx should be preferable.
I'll still decipher more about BFD's non-increasing VMA support.

> This change moves the finalisation of .ARM.exidx till after the first call to AssignAddresses.

`assignAddresses`



================
Comment at: lld/test/ELF/arm-exidx-script-order.s:1
+// RUN: llvm-mc --arm-add-build-attributes --triple=armv7a-linux-gnueabihf -filetype=obj %s -o %t.o
+// RUN: echo "SECTIONS { \
----------------
`// REQUIRES: ARM`


================
Comment at: lld/test/ELF/arm-exidx-script-order.s:10
+// RUN: ld.lld --script %t.script %t.o -o %t
+// RUN: llvm-readobj -x .ARM.exidx  %t | FileCheck %s
+
----------------
delete excess space before `%t`


================
Comment at: lld/test/ELF/arm-exidx-script-order.s:17
+// 0x80000000 + 0x28 = 0x80000028, 0x80000008 + 0xf8 = 0x80000100 
+// 0x80000000 24000000 08849780 f8000000 20849980
+// 0x80000010 + 0x1f0 = 0x8000200, 0x80000018 + 0x1ec = 0x8000204
----------------
Missing `CHECK-NEXT: `


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78422/new/

https://reviews.llvm.org/D78422