[PATCH] D73542: [LLD][ELF][ARM] Do not substitute BL/BLX for non STT_FUNC symbols.

Wed Feb 12 08:14:26 PST 2020

MaskRay added a comment.

In D73542#1871695 <https://reviews.llvm.org/D73542#1871695>, @psmith wrote:

> > I asked because I wonder whether a warning is the right approach. GNU ld does not warn. If the projects that are affected is just Chromium, do we still need a warning? It seems @thakis has already fixed the bug.
> >  A warning can help users catch bugs, but it can also make a user with a legitimate use case frustrated.
>
> Reasons for a warning
>
> - We are making a change in behavior to match bfd, projects only linked with gold or LLD for some portion of time could suffer runtime errors.
> - As thakis points out, Chromium has extensive testing, bisection to find problems, and someone like thakis to report and fix, not all projects will be so well served.
> - The author of the problem in Chromium didn't know that they needed to set the type of the symbol to STT_FUNC, I think that this situation is common.
> - We can limit the warning to cases where LLD would change behaviour, hence catch problems due to this change.
> - All compiled code will be correctly typed, it is only assembly language we need to worry about. Reasons for not warning
> - There are false positives, which in one corner case (bl to section symbol + offset) could be difficult to resolve.
> - It makes the code more complicated
>
>   At the moment I'd prefer a warning. My suggestion for how to proceed:
> - Try with a warning, see if there are false positives reported
> - If no then keep it, if yes then write a more complex check using the mapping symbols

Well said. Let's give it a try!

================
Comment at: lld/test/ELF/Inputs/arm-thumb-abs.s:1
+/// absolute symbol with bit-0 set. If used as a B, BL or BLX target no
+/// interworking will be done as sym has type STT_NOTYPE
----------------
This file can be deleted.

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

https://reviews.llvm.org/D73542