[PATCH] D135642: [MC] .addrsig_sym: ignore unregistered symbols

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

I think this looks good, but make sure @tejohnson's concerns are addressed.

In D135642#3850471 <https://reviews.llvm.org/D135642#3850471>, @MaskRay wrote:

> In D135642#3849835 <https://reviews.llvm.org/D135642#3849835>, @rnk wrote:
>
>> If we have this, should we revert D101512 <https://reviews.llvm.org/D101512> as a code simplification?
>
> It can be considered a follow-up if this new behavior appears stable in the main branch.

Sounds good

>> Please update the .addrsig docs available at https://llvm.org/docs/Extensions.html#machine-specific-assembly-syntax to document that symbols that are neither defined nor referenced are not recorded as address-significant.
>
> Done.

Thanks!

>> Also, I'm starting to reconsider D135427 <https://reviews.llvm.org/D135427>, which made non-prevailing local comdat members available_externally. Why did you choose available_externally? That is an external linkage. If there are any references to the local linkage symbol, they will now become references to an external symbol with the same name, rather than becoming errors, or relocations against discarded sections. The native linker *discards* non-prevailing comdat members, so we should do that too in complex cases. Using available_externally is something we do to support optimization (inlining).
>
> Because marking such a symbol in a non-prevailing COMDAT `available_externally` is the only available and pratical mechanism, considering that it may be referenced by symbols in the same COMDAT. The symbol cannot just be eliminated (without introducing a GC pass).
>
> External references to local linkage symbols in a non-prevailing COMDAT group is disallowed. The generic ABI says "... References to this symbol table entry from outside the group are not allowed."
> (ld.lld `error: relocation refers to a symbol in a discarded section: `)
> This property means that local linkage symbols in COMDAT groups are essentially leaves in a reference graph. I think D135427 <https://reviews.llvm.org/D135427> treatment has sane semantics.

I think what I'm saying is that yes, such references to local linkage symbols from outside of a comdat are not allowed. In a native link, they result in exactly that error message. How can we ensure that we issue an equivalent linker error in that case during ThinLTO? Anyway, we can discuss further on that review thread.



================
Comment at: llvm/docs/Extensions.rst:380
 
-This marks ``sym`` as address-significant.
+If ``sym`` is not emitted as a symbol, this directive is a no-op.
+Otherwise, mark ``sym`` as address-significant.
----------------
I would elaborate: If sym is not otherwise referenced or defined anywhere else in the file, this directive is a no-op.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135642