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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Incidentally, this is no longer dependent on https://reviews.llvm.org/D30657, since the handling moved out of the ELFAsmParser. Will update this patch to remove the dependence, but that is still a good standalone fix.



================
Comment at: lib/Object/ModuleSymbolTable.cpp:61
+static void handleSymverAliases(const Module &M, RecordStreamer &Streamer) {
+  // The name in the assembler will be mangled, but the name in the IR
+  // might not, so we first compute a mapping from mangled name to GV
----------------
pcc wrote:
> `if (Streamer.symverAliases().empty()) return;`
Good idea, will add


================
Comment at: lib/Object/ModuleSymbolTable.cpp:74
+    Mang.getNameWithPrefix(MangledName, &GV, /*CannotUsePrivateLabel=*/false);
+    if (MangledName != GV.getName())
+      MangledNameMap[MangledName] = &GV;
----------------
pcc wrote:
> You can make the code a little simpler by putting all globals in the map.
I was trying to reduce the size of the map we create, since in most cases we won't need an entry. I'll go ahead and remove the guard, the map creation shouldn't be common in the first place.


================
Comment at: lib/Object/RecordStreamer.h:26
+  // aliasee to its list of aliases.
+  DenseMap<const MCSymbol *, std::vector<MCSymbol *>> SymverAliasMap;
   void markDefined(const MCSymbol &Symbol);
----------------
pcc wrote:
> Can just be a vector of (alias, aliasee) pairs.
I prefer to keep it this way so the processing in handleSymverAliases can stay the same (i.e. only  look for the Aliasee in the IR once for all aliases). 


================
Comment at: test/LTO/X86/symver-asm.ll:19
+; Local values used in inline assembly must be specified on the
+; llvm.compiler.used so they aren't incorrectly internalized.
+ at llvm.compiler.used = appending global [1 x i8*] [i8* bitcast (i32 ()* @io_cancel_local_0_4 to i8*)], section "llvm.metadata"
----------------
pcc wrote:
> You mean so they aren't DCE'd?
Right, or more specifically, not linked into the combined module. Will change to "DCE'd during module linking".


https://reviews.llvm.org/D30485