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

[George Rimar via Phabricator via llvm-commits]

grimar 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
----------------
peter.smith wrote:
> 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?
I meant the change in the function signature seems to be because of a clang-format :)
No arguments were changed or added.



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

https://reviews.llvm.org/D73474