[PATCH] D40278: Object: Improve COFF irsymtab comdat representation.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Object/IRSymtab.cpp:160
+    llvm::raw_string_ostream OS(Name);
+    Mangler::getNameWithPrefix(OS, C->getName(), M->getDataLayout());
+
----------------
rnk wrote:
> Every time I see getNameWithPrefix called without a GlobalValue, I consider it a bit buggy because it won't add the @N byte suffix for __stdcall functions. However, clang always mangles extern "C" names itself for functions with non-standard conventions, so there's no way to exercise this bug from C, it has to come from LLVM IR.
> 
> Do we care? Should we fatal error in AsmPrinter when a symbol in a comdat would require a suffix? Most languages that aren't C++ aren't going to care about this.
Thanks. For me that raised the question of how AsmPrinter mangles comdat names. I thought it just called `Mangler::getNameByPrefix` on the name as I did here, but it looks like in fact we look up the leader symbol using the name of the comdat and mangle its name [1].

That seems really weird to me, but for now it seems best to be consistent with that here. We might consider making a change along the lines you suggest, but that's probably an orthogonal change.

[1] http://llvm-cs.pcc.me.uk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#1027


https://reviews.llvm.org/D40278