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

Peter Collingbourne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 7 14:18:31 PST 2017


pcc added a comment.

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).

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



================
Comment at: lib/Object/ModuleSymbolTable.cpp:112
+      [&](StringRef Name) -> MCSymbolAttr {
+    auto *GV = M.getNamedValue(Name);
+    if (!GV)
----------------
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`.


https://reviews.llvm.org/D30485





More information about the llvm-commits mailing list