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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

> > I have some questions. Are the two warnings added for migration convenience for some projects? Are they still useful after the migration? Do they have false positives? i.e. Is it not allowed to emit BLX referencing a Thumb STT_NONE symbol in ARM state? Is it not allowed to emit BL referencing an ARM STT_NONE symbol in Thumb state? If there are legitimate use cases, will the warnings break their builds if they are configured with -Wl,--fatal-warnings?
>
> The warnings are for migration convenience. They are still useful after the migration as very few people coming to Arm and Thumb assembly know that writing .type symbol, %function is important. I think most existing files in projects have a macro for declaring functions and this usually takes care of the type, but it could still save some new projects some pain.
>
> The ABI rule is essentially symbol type is STT_FUNC, linker's responsibility for interworking, all other cases are the programmer's responsibility.
>
> False positives are possible, although most of these are cases where LLD will now get the interworking right, and LLD 10.0 would get it wrong, so in theory they should be rare in code bases already linked by LLD.
>
>   .thumb
>   // foo has STT_NOTYPE, LLD 10.0 will treat as ARM and incorrectly turn this into a BLX
>   // LLD 11.0 will warn with false positive
>   foo: bx lr
>           bl foo
>
>
>   // foo has STT_NOTYPE, LLD 10.0 will treat as ARM and incorrectly turn this into a BL
>   // LLD 11.0 will warn with false positive
>   .thumb
>   foo: bx lr
>   .arm
>           blx foo
>
>
> The bl to a "section symbol + offset" is the only case where the warning can't be fixed with .type symbol, %function.
>
>   .section .foo, "ax", %progbits
>   .thumb
>   nop
>   bx lr
>  
>   bl .foo + 4
>
>
> Yes the false positives could fail a build if fatal warnings was enabled. The only case I can think of that would do this is if someone wrote something like https://github.com/ClangBuiltLinux/linux/issues/325#issuecomment-457160379 but with the b changed to a bl. I don't it would be possible to set the type on the local label or section symbol as the programmer wouldn't know what it was.
>
>   #define __futex_atomic_ex_table(err_reg)
>   "3:\n"
>   " .pushsection __ex_table,"a"\n"
>   " .align 3\n"
>   " .long 1b, 4f, 2b, 4f\n"
>   " .popsection\n"
>   " .pushsection .text.fixup,"ax"\n"
>   " .align 2\n"
>   "4: mov %0, " err_reg "\n"
>   " bl 3b\n"
>   " .popsection"
>
>
> We could eliminate the false positives if we use the mapping symbols the disassembler uses to cross check the state. LLD isn't set up to use the mapping symbols efficiently or easily though, essentially a linear scan through the objects local symbols. I'm happy to write something using them, but thought that it was better to try the minimal change first.
>
> Will upload another diff with the changes.


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.


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

https://reviews.llvm.org/D73542