[PATCH] D30485: Mark .symver versioned symbol as global

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/MC/MCParser/ELFAsmParser.cpp:748
+  // for the linker.
+  getStreamer().EmitSymbolAttribute(Alias, MCSA_Global);
   return false;
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > pcc wrote:
> > > > > I don't think this is right. See `ELFObjectWriter::executePostLayoutBinding`: `.symver` symbols inherit their binding from the symbol they alias.
> > > > > 
> > > > > I think we may need something equivalent to the `executePostLayoutBinding` logic in `ModuleSymbolTable::CollectAsmSymbols`.
> > > > Thanks for pointing me to that, I was having trouble finding good documentation on the .symver directive, but I see that if I make the aliased symbol local that gas makes the versioned symbol also local.
> > > > 
> > > > However, I'm not sure I agree that the handling in executePostLayoutBinding, which deals with .symver specifically, should be added to CollectAsmSymbols, which is doing more general handling of the symbols returned from the streamer. It seems like we should do this in the .symver handling here within the ELFAsmParser, and instead set the appropriate symbol attribute based on the symbol binding of the aliased Sym (instead of unconditionally setting to global as I have done here). 
> > > > 
> > > The problem is that we cannot resolve the binding until we have seen the entire input because a later directive may change the binding. For example:
> > > 
> > > ```
> > > foo:
> > > .symver foo, bar@@baz
> > > 
> > > .weak foo
> > > ```
> > > In this case `bar@@baz` has weak binding but that is not known until the end of the input.
> > > 
> > > It may be that we can handle this within the parser but I don't think it would be right to do it entirely at the point where we see the directive.
> > Good point.
> > 
> > Sigh, my test case expanded to include the local and weak cases hit https://bugs.llvm.org//show_bug.cgi?id=25679 for the local case. I think as Rafael said there we should relax this assertion, although I am not sure what the right conditions for relaxing this are - we no longer have the local aliased symbol defined anywhere so can't check that its binding was local.
> > 
> > For the original bug I am fixing here, getting this right unfortunately seems tricky - the binding of the aliased symbol could be set in the module asm via a directive, or as is  probably more likely, in the IR, e.g.:
> > 
> > module asm ".symver io_cancel_local_0_4,io_cancel_local@@LIBAIO_0.4"
> > 
> > define internal i32 @io_cancel_local_0_4() {
> >   ret i32 0
> > }
> > 
> > In that case, we don't have the information in the asm parser about the aliased symbol's binding. We don't even currently have this in CollectAsmSymbols, although I suppose we could pass in the Module. But it seems complex to get this right - we need to identify these .symver aliases, and get the binding of the aliased symbol, either from the asm or from the IR. Not sure of a cleaner way to do this right...
> > 
> > Ultimately the emitted object has the right bindings, because the binding is set on the alias in ELFObjectWriter::executePostLayoutBinding. But the issue is ensuring the symbol iterator in the InputFile skips the right symbols.
> Yes, this is tricky. I think we will have to change `ModuleSymbolTable` to keep track of the set of defined symbol names (and as part of that use the aliased symbol to compute the binding of the alias). I think that is something we'll need to do anyway to deduplicate symbols when we create the bitcode symbol table.
> 
> If we also keep track of which symbols are aliased with `.symver` we can inhibit internalization for those symbols, which would seem to fix pr25679.
It's also tricky because CollectAsmSymbols is target agnostic, and the symver directive is specific to ELF. Even the symbol binding is only accessible via the ELF symbol type.

After looking at this some more, I think the most straightforward way is to use a callback from the parser that is defined in CollectAsmSymbols, that would a) add the aliased symbol to the llvm.compiler.used set to avoid internalization; and b) return the binding info based on the linkage type (assuming it is defined in the IR). 

Then have a virtual method probably on the MCAsmParserExtension that can be invoked at the end of parsing and would be overridden for the ELFAsmParser to do the right thing for symver aliases (which would need to be tracked), and which uses the callback.


https://reviews.llvm.org/D30485