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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D73542#1865913 <https://reviews.llvm.org/D73542#1865913>, @thakis wrote:

> > I'm happy to work on either, although I think starting with the smallest change would be a good way to start.
>
> Sounds good. Happy to test the simple warning on our code (with the fix to crashpad's asm reverted, to make sure the warning would find that).


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?



================
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))) {
----------------
`bit0 != isBlx`


================
Comment at: lld/ELF/Arch/ARM.cpp:408
+  if ((relt == R_ARM_CALL && (bit0 ? !isBlx : isBlx)) ||
+      (relt == R_ARM_THM_CALL && (bit0 ? isBlx : !isBlx))) {
+    if (s.isSection()) {
----------------
`bit0 == isBlx`


================
Comment at: lld/ELF/Arch/ARM.cpp:412
+      // the type. Use the section name as getName() returns an empty string.
+      const auto *d = cast<Defined>(&s);
+      warn(getErrorLocation(loc) + "branch and link relocation: " +
----------------
`const auto &d = cast<Defined>(s);` works


================
Comment at: lld/ELF/Arch/ARM.cpp:414
+      warn(getErrorLocation(loc) + "branch and link relocation: " +
+           toString(relt) + " to STT_SECTION symbol " + d->section->name +
+           " ; interworking not performed");
----------------
`cast<Defined>(s).section->name`

Delete the variable `d`


================
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;
----------------
When `rel.sym == nullptr`, is the R_ARM_CALL invalid? If yes, we can guard these statements with a single `rel.sym` nullness check.


================
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);
----------------
Is the condition (including the checks performed in `warnOnStateMismatch`) simply: `rel.sym && !rel.sym->isFunc() && isBlx`?


================
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);
----------------
Is the condition (including the checks performed in `warnOnStateMismatch`) simply: `rel.sym && !rel.sym->isFunc() && !isBlx`?


================
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
----------------
`--defsym sym=0x13001` can avoid `%S/Inputs/arm-thumb-abs.s`


================
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
----------------
Is `--no-threads` significant?


================
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
----------------
Synthesize debug info with `llvm-mc -g`

Then use something like `# CHECK: {{.*}}.s:[[# @LINE+1]]:1: warning: ` to check the line numbers.


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

https://reviews.llvm.org/D73542