[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:
> 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.
> 
Yes that is true, the function symbol presumably due to an automatic refactoring had overflowed 80 columns so I took the opportunity to fix it. I can split it out into a separate patch if you want, but it seemed sufficiently small to roll it in with this one?


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

https://reviews.llvm.org/D73474