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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/MC/MCParser/ELFAsmParser.cpp:748
+  // for the linker.
+  getStreamer().EmitSymbolAttribute(Alias, MCSA_Global);
   return false;
----------------
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.


https://reviews.llvm.org/D30485