[PATCH] D73474: [LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked an inline comment as done.
peter.smith added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:267
+                     uint64_t branchAddr, const Symbol &s,
+                     int64_t /*a*/) const {
   // If S is an undefined weak symbol and does not have a PLT entry then it
----------------
grimar wrote:
> Unrelated?
Assuming this is the:

```
  // If S is an undefined weak symbol and does not have a PLT entry then it
  // will be resolved as a branch to the next instruction.
  if (s.isUndefWeak() && !s.isInPlt())
    return false;
```
I think that is still relevant. It would be for cases like:

```
.weak weakfunction
.type weakfunction, %function
.thumb
b weakfunction
 // we want b weakfunction to resolve to the next instruction if weakfunction is undefined
bx lr
```
If weakfunction were undefined then it would have a value of the address of the next instruction which would have bit 0 clear. This would look to the code below as if it were an Arm function.

It could be possible to integrate the check into the code below, something like (not tested):
```
case R_ARM_THM_JUMP24:
    // Source is Thumb, all PLT entries are ARM so interworking is required.
    // Otherwise we need to interwork if STT_FUNC Symbol has bit 0 clear (ARM).
    if (expr == R_PLT_PC || (s.isFunc() && !s.isUndefWeak() && (s.getVA() & 1) == 0))
``` 
The early exit is, to me, a bit simpler to understand, but I'd be happy to explore the alternative if you'd prefer, possibly a follow up patch?


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

https://reviews.llvm.org/D73474