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

[Peter Smith via Phabricator via llvm-commits]

psmith added a comment.

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

> Yup, it bisected to this change as well.
>
> It's now in a much bigger binary and analysis will probably take a bit longer than last time.
>
> Would you mind reverting this while we figure out what exactly is going on? We haven't had a working trunk version in over a week now, and it'd be good if we could update chromium's llvm to make sure there are no other new regressions.


I don't mind reverting for now as this is mostly a bring the tools to conform to the ld.bfd as the Linux kernel has had problems with at least branch (not branch with link) case. I'm away at a conference and don't have easy access to a terminal, would you be able to revert this and the fix? A test case would be great. I don't mind if it is an lld --reproduce (probably need a google drive link) and a pointer to which call is wrong?



================
Comment at: lld/ELF/Arch/ARM.cpp:457
+    bool isBlx = (read16le(loc + 2) & 0x1000) == 0;
+    bool interwork = (rel.sym && rel.sym->isFunc()) || rel.type == R_PLT_PC;
+    if (interwork ? (val & 1) == 0 : isBlx) {
----------------
MaskRay wrote:
> ```
> -     bool interwork = (rel.sym && rel.sym->isFunc()) || rel.type == R_PLT_PC;
> +     bool interwork = (rel.sym && rel.sym->isFunc()) || rel.expr == R_PLT_PC;
> ```
> 
> When no IFUNC is involved, `R_PLT_PC->R_PC` optimization is entirely decided by `sym.isPreemptible`.
> Can we use `sym.isPreemptible` instead?
> 
> When IFUNC is involved (there is also a FreeBSD extension `-z ifunc-noplt`), I get more confused.
> 
> ```lang=c
> // Relocations.cpp:1323
>   if (!sym.isPreemptible && (!sym.isGnuIFunc() || config->zIfuncNoplt)) {
> ```
> 
> (I thought about IFUNC, but apologies that I did not raise the point in the review.)
I'll need to check tomorrow. We're using knowledge that a PLT entry is always Arm state, perhaps sym.isInPlt() would be the clearest way to express that intent. I think sym.isPreemptible is a good proxy for that but not as descriptive. That would also avoid potential problems with the FreeBSD extension if it resulted in a R_PLT_PC relocation but no PLT entry.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73542