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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
