[PATCH] D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols.

Tue Feb 11 15:44:24 PST 2020

psmith added a comment.

> I have some questions. Are the two warnings added for migration convenience for some projects? Are they still useful after the migration? Do they have false positives? i.e. Is it not allowed to emit BLX referencing a Thumb STT_NONE symbol in ARM state? Is it not allowed to emit BL referencing an ARM STT_NONE symbol in Thumb state? If there are legitimate use cases, will the warnings break their builds if they are configured with -Wl,--fatal-warnings?

The warnings are for migration convenience. They are still useful after the migration as very few people coming to Arm and Thumb assembly know that writing .type symbol, %function is important. I think most existing files in projects have a macro for declaring functions and this usually takes care of the type, but it could still save some new projects some pain.

The ABI rule is essentially symbol type is STT_FUNC, linker's responsibility for interworking, all other cases are the programmer's responsibility.

False positives are possible, although most of these are cases where LLD will now get the interworking right, and LLD 10.0 would get it wrong, so in theory they should be rare in code bases already linked by LLD.

  .thumb
  // foo has STT_NOTYPE, LLD 10.0 will treat as ARM and incorrectly turn this into a BLX
  // LLD 11.0 will warn with false positive
  foo: bx lr
          bl foo

  // foo has STT_NOTYPE, LLD 10.0 will treat as ARM and incorrectly turn this into a BL
  // LLD 11.0 will warn with false positive
  .thumb
  foo: bx lr
  .arm
          blx foo

The bl to a "section symbol + offset" is the only case where the warning can't be fixed with .type symbol, %function.

  .section .foo, "ax", %progbits
  .thumb
  nop
  bx lr

  bl .foo + 4

Yes the false positives could fail a build if fatal warnings was enabled. The only case I can think of that would do this is if someone wrote something like https://github.com/ClangBuiltLinux/linux/issues/325#issuecomment-457160379 but with the b changed to a bl. I don't it would be possible to set the type on the local label or section symbol as the programmer wouldn't know what it was.

  #define __futex_atomic_ex_table(err_reg)
  "3:\n"
  " .pushsection __ex_table,"a"\n"
  " .align 3\n"
  " .long 1b, 4f, 2b, 4f\n"
  " .popsection\n"
  " .pushsection .text.fixup,"ax"\n"
  " .align 2\n"
  "4: mov %0, " err_reg "\n"
  " bl 3b\n"
  " .popsection"

We could eliminate the false positives if we use the mapping symbols the disassembler uses to cross check the state. LLD isn't set up to use the mapping symbols efficiently or easily though, essentially a linear scan through the objects local symbols. I'm happy to write something using them, but thought that it was better to try the minimal change first.

Will upload another diff with the changes.

================
Comment at: lld/ELF/Arch/ARM.cpp:407
+  bool bit0 = s.getVA() & 0x1;
+  if ((relt == R_ARM_CALL && (bit0 ? !isBlx : isBlx)) ||
+      (relt == R_ARM_THM_CALL && (bit0 ? isBlx : !isBlx))) {
----------------
MaskRay wrote:
> `bit0 != isBlx`
Thanks, for spotting, will apply and the ones below.

================
Comment at: lld/ELF/Arch/ARM.cpp:459
+    // PLT entries are always ARM state so we know we don't need to interwork.
+    bool isBlx = (read32le(loc) & 0xfe000000) == 0xfa000000;
+    bool interwork = rel.sym && rel.sym->isFunc() && rel.expr != R_PLT_PC;
----------------
MaskRay wrote:
> When `rel.sym == nullptr`, is the R_ARM_CALL invalid? If yes, we can guard these statements with a single `rel.sym` nullness check.
At the moment yes. If we ever need relocateNoSym with either R_ARM_CALL or R_ARM_THM_CALL we'll need to revisit. I've left an assert for the moment.

================
Comment at: lld/ELF/Arch/ARM.cpp:461
+    bool interwork = rel.sym && rel.sym->isFunc() && rel.expr != R_PLT_PC;
+    if (!interwork && rel.sym)
+      warnOnStateMismatch(loc, rel.type, *rel.sym, isBlx);
----------------
MaskRay wrote:
> Is the condition (including the checks performed in `warnOnStateMismatch`) simply: `rel.sym && !rel.sym->isFunc() && isBlx`?
I think so. I've rewritten to have warnOnStateMismatch be just a warning, and have integrated the logic to trigger it into R_ARM_CALL and R_ARM_THM_CALL.

================
Comment at: lld/ELF/Arch/ARM.cpp:508
+    bool interwork = (rel.sym && rel.sym->isFunc()) || rel.expr == R_PLT_PC;
+    if (!interwork && rel.sym)
+      warnOnStateMismatch(loc, rel.type, *rel.sym, isBlx);
----------------
MaskRay wrote:
> Is the condition (including the checks performed in `warnOnStateMismatch`) simply: `rel.sym && !rel.sym->isFunc() && !isBlx`?
I've separated the logic from the warning as in R_ARM_CALL.

================
Comment at: lld/test/ELF/arm-thumb-interwork-abs.s:4
+// RUN: llvm-mc --triple=armv7a-linux-gnueabihf -arm-add-build-attributes -filetype=obj -o %t2.o %S/Inputs/arm-thumb-abs.s
+// RUN: ld.lld %t.o %t2.o -o %t --no-threads 2>&1 | FileCheck %s --check-prefix=WARN
+// RUN: llvm-objdump --no-show-raw-insn -d %t | FileCheck %s
----------------
MaskRay wrote:
> `--defsym sym=0x13001` can avoid `%S/Inputs/arm-thumb-abs.s`
Thanks, have applied.

================
Comment at: lld/test/ELF/arm-thumb-interwork-notfunc.s:3
 // RUN: llvm-mc --triple=armv7a-linux-gnueabihf -arm-add-build-attributes -filetype=obj -o %t.o %s
-// RUN: ld.lld %t.o -o %t
+// RUN: ld.lld %t.o -o %t --no-threads 2>&1 | FileCheck %s --check-prefix=WARN
 // RUN: llvm-objdump --no-show-raw-insn -d %t | FileCheck %s
----------------
MaskRay wrote:
> Is `--no-threads` significant?
Not in the test, it was very helpful when writing as there are two sections with similar error messages, but they could come out interleaved. I've removed it and from the other test.

================
Comment at: lld/test/ELF/arm-thumb-interwork-notfunc.s:78
  beq.w thumb_func_with_explicit_notype
+ bl .arm_target
+// WARN: branch and link relocation: R_ARM_THM_CALL to STT_SECTION symbol .arm_target ; interworking not performed
----------------
MaskRay wrote:
> Synthesize debug info with `llvm-mc -g`
> 
> Then use something like `# CHECK: {{.*}}.s:[[# @LINE+1]]:1: warning: ` to check the line numbers.
Thanks for the tip, I wasn't aware of that.

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

https://reviews.llvm.org/D73542