mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2519-2520
return VarLinkage;
+ // On Windows, WeakODR is a no-op, boiling down to the same as normal
external
+ // linkage.
+ if (CGM.getTriple().isOSWindows())
----------------
rnk wrote:
> mstorsjo wrote:
> > rnk wrote:
> > > I would say that this is inaccurate. It greatly affects what the
> > > optimizer is allowed to do.
> > >
> > > It looks like we forgot to put a comdat on these things, is that not the
> > > correct fix?
> > Oh, ok.
> >
> > The full case I was trying to fix (but forgot to recheck after changing
> > this bit) is that when used with `-ffunction-sections`, the tls wrapper
> > function ends up as comdat `one_only`, which then gives multiple definition
> > errors. So perhaps the issue is in the handling of `-ffunction-sections`
> > wrt weak_odr?
> Ah, that is an interesting wrinkle. I'm surprised that things worked without
> -ffunction-sections, though. I would've expected the two plain external
> definitions to produce multiple definition errors.
Without `-ffunction-sections`, there's no separate thread wrapper produced at
all, so everything else is generated with working linkage. I just haven't
happened to build code that forces generation of a tls wrapper without
`-ffunction-sections` yet.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2522
+ if (CGM.getTriple().isOSWindows())
+ return llvm::GlobalValue::LinkOnceODRLinkage;
return llvm::GlobalValue::WeakODRLinkage;
----------------
rsmith wrote:
> I think the thread wrapper should probably be `linkonce_odr` across all
> platforms, at least in all TUs that don't contain a definition of the
> variable. Every such TU is supposed to provide its own copy regardless, so
> making it non-discardable seems to serve no purpose.
>
> That said, I suspect this is only hiding the real problem (by discarding the
> symbol before it creates a link error), and you'd still get link errors if
> you have two TUs that both use the same thread-local variable and happen to
> not inline / discard their thread wrappers.
There's actually a case already where these are made linkonce for other TUs at
https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/ItaniumCXXABI.cpp#L2639-L2642:
```
// If this isn't a TU in which this variable is defined, the thread
// wrapper is discardable.
if (Wrapper->getLinkage() == llvm::Function::WeakODRLinkage)
Wrapper->setLinkage(llvm::Function::LinkOnceODRLinkage);
```
In what cases would two TUs create two non-inline/discardable wrappers - and
wouldn't that cause similar linking errors on other platforms as well?
Repository:
rG LLVM Github Monorepo
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
