[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:
> > > 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?
> I've noticed this because tried to find what changed in the method signature at first. 
> If you formatted the same lines you changed then it would be completly fine, but this lines are a bit too far form another changes you have and often such refactorings are committed as separate NFC changes which does not requre a review.
> Also this makes the commit history cleaner (and it is much easier to make a git "blame" operation when the history is clean).
> 
> So I'd really suggest to commit such refactoring(s) as a NFC change separatelly if you want to do them.
Thanks, will split out and commit separately as NFC.


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

https://reviews.llvm.org/D73474