[PATCH] D71572: [ItaniumCXXABI] Make tls wrappers properly comdat

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm

This will be a small change in behavior, but nobody on ELF should notice because things with vague linkage there are both ELF-weak and comdat.



================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2551
+  // actually handle multiple TUs defining the same wrapper.
+  if (CGM.getTriple().isOSWindows() && CGM.supportsCOMDAT() &&
+      Wrapper->isWeakForLinker())
----------------
rsmith wrote:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > rnk wrote:
> > > > IMO we should be doing the same thing on ELF, you can see the code pattern used elsewhere:
> > > > http://llvm-cs.pcc.me.uk/?q=setComdat
> > > > 
> > > > Before comdats were made explicit in the IR and we stopped implicitly making comdat groups for odr things on ELF, these wrappers would've been in a comdat group.
> > > Ok, so remove the `isOSWindows()` and maybe remove the comment altogether as it isn't windows specific any longer?
> > In the current tests, for linux there's explicit checks that these aren't marked as comdat, see e.g. https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGenCXX/cxx11-thread-local.cpp#L208. Is there a specific reason for that, or should it be changed?
> The test is intending to check that the wrapper is not in a comdat keyed off the variable. Putting it in a trivial comdat is fine and correct; please update the tests to check that it's in the right comdat, not just that it's in some comdat. (I seem to regularly forget that `weak` / `linkonce` / `weak_odr` / `linkonce_odr` don't actually work as documented any more...)
I think this is the confirmation you need, the new tests look correct to me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71572/new/

https://reviews.llvm.org/D71572