[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:
> > 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.


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

https://reviews.llvm.org/D73474