Functions with available_externally linkage will not be emitted to object
files (they will just be undefined symbols), so it does not make sense to
put them in comdats.
Details
Diff Detail
Event Timeline
This is correct, but we have lots of instances that should use the same logic:
$ git grep supportsCOMDAT.*isWeakForLinker
lib/CodeGen/CGDecl.cpp: if (supportsCOMDAT() && GV->isWeakForLinker())
lib/CodeGen/CGVTT.cpp: if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
lib/CodeGen/CGVTables.cpp: if (CGM.supportsCOMDAT() && Fn->isWeakForLinker())
lib/CodeGen/CodeGenModule.cpp: if (supportsCOMDAT() && GV->isWeakForLinker() &&
lib/CodeGen/CodeGenModule.cpp: if (supportsCOMDAT() && GV->isWeakForLinker())
lib/CodeGen/ItaniumCXXABI.cpp: if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
lib/CodeGen/ItaniumCXXABI.cpp: } else if (CGM.supportsCOMDAT() && guard->isWeakForLinker()) {
lib/CodeGen/ItaniumCXXABI.cpp: if (CGM.supportsCOMDAT() && GV->isWeakForLinker())
How about we add a maybeSetTrivialComdat overload that takes a GlobalObject or something? I'm not asking you to do the migration if you don't want to, but just to add the central place that we should be calling to handle the case where we don't have an easily available Decl with GVA linkage.
Yeah that was actually going to be my followup question, how many other places need this. My concern about just grepping and changing was just the issue I encountered here, where the linkage type was actually not finally set at the point where the comdat was added (hence moving it from generateThunk to emitThunk).
Also it looks like many of those cases can actually use the existing maybeAddTrivialComdat because they have Decls available. Anyway here's another patch with a maybeSetTrivialComdat overload.
Do you have an opinion about whether it should take a GlobalValue or just a GlobalObject? I'm a little bit unclear about linkage and comdats for aliases.
Actually the other places that needed fixing were all quite straightforward,
so just fix them too. Also make maybeSetTrivialComdat take a GO instead of a GV.
We could maybe even just replace the maybeSetTrivialComdat that takes the Decl
too, but that's probably another CL.
indentation seems off