[PATCH] D30485: Perform symbol binding for .symver versioned symbols

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D30485#694931, @pcc wrote:

> In https://reviews.llvm.org/D30485#694812, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D30485#693665, @pcc wrote:
> >
> > > The parser doesn't seem like the correct place to handle this.
> >
> >
> > Why? The ELFAsmParser already has .symver handling and is specific to ELF. The benefit of handling there is that all clients get the correct binding. My concern with doing this in the MCStreamer & RecordStreamer is that the RecordStreamer is currently format-agnostic. The MCStreamer has some format specific Emit* methods on it, but they are empty and overridden by the various format-specific object file writers derived via MCObjectStreamer.
>
>
> The parser is supposed to be just a parser. The streamer is meant to control what to do with each directive. It is fine if it contains special handling for ELF because the RecordStreamer is supposed to be handling all directives that can introduce a symbol reference or definition in the object file, even those which happen to be specific to one object format (in fact, I suspect that we may need to override methods like `BeginCOFFSymbolDef` in the future).


Fair enough.

> 
> 
>>> If you introduce an `EmitELFSymverDirective` method on `MCStreamer`, and extend the `AsmSymbol` callback to `CollectAsmSymbols` with something that lets the caller know that this is a `symver` symbol and the name of the aliasee,
>> 
>> I think what would need to be done is to add this virtual method on the MCStreamer, which by default doesn't do anything, and only implement it for the derived RecordStreamer class. For RecordStreamer, it would need to save the mapping between the alias and aliasee. Then in CollectAsmSymbols, we'd have to walk this saved mapping, and get and apply the same symbol attribute to the alias as applied to its aliasee (either by invoking RecordStreamer::EmitSymbolAttribute, or by applying the same RecordStreamer::State to the aliasee when iterating through the RecordStreamer's Syymbols string map). We also need to get the symbol binding from the IR in the case where the aliasee is defined in IR.
> 
> Yes, you could even handle all non-symver symbols first and then handle the symver symbols by walking the partially computed ModuleSymbolTable. (I think it is fine if it uses a linear scan because module asm symbols are uncommon.)

I changed to handling this in the RecordStreamer and CollectAsmSymbols.

> 
> 
>>>   I think you can move all handling of this directive into `RecordStreamer` and the `AsmSymbol` callback provided by `ModuleSymbolTable`.
>> 
>> Do you mean move it into the invocation of CollectAsmSymbols from ModuleSymbolTable::addModule (i.e. into it's definition of AsmSymbol)? This will only benefit that one client of CollectAsmSymbols (although that is the one that matters for this bug).
> 
> If a caller of CollectAsmSymbols needs 100% accurate binding information, it will effectively need to create a ModuleSymbolTable. I haven't looked very closely at what the other callers are doing though. It may be that they can get by with inaccurate bindings for symvers.

All the handling is in CollectAsmSymbols, none needed in the AsmSymbol callbacks.



================
Comment at: lib/Object/ModuleSymbolTable.cpp:112
+      [&](StringRef Name) -> MCSymbolAttr {
+    auto *GV = M.getNamedValue(Name);
+    if (!GV)
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > I don't think this is right, as `Name` may be mangled.
> > Won't the name in IR also be mangled?
> In ELF it could be something like `@"\01foo"`. In that case the mangled name is `foo`.
Ah, wasn't familiar with cases like that. Since I need to go from the mangled name (in asm) to a potentially unmangled name (in IR), I ended up needing to precompute a map. Added a case like this to the test.


https://reviews.llvm.org/D30485