[PATCH] D77248: [llvm][IR] Add dso_local_equivalent Constant

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:36
+    // Propagate function attributes.
+    Stub = Function::Create(F->getFunctionType(), F->getLinkage(), StubName, M);
+    Stub->setAttributes(F->getAttributes());
----------------
MaskRay wrote:
> leonardchan wrote:
> > MaskRay wrote:
> > > I think the stub should be InternalLinkage (if you want to retain a symbol) or PrivateLinkage (if you don't want a symbol). You would not need hidden visibility below.
> > > 
> > > If you use a hidden function, and two translation units have dso_local declarations of the same function, the produced stubs will cause a duplicate definition error at link time.
> > Hmm. I'm guessing the link error would only occur though if comdats weren't supported? In that case I think perhaps the stubs should be internal/private to avoid multiple definitions.
> For stubs for declarations (not defined), there are some pitfalls. On ELF, if in a comdat, a hidden symbol works better because it can be merged with symbols in other translations; if not in a comdat, private/internal linkage has to be used to avoid linker issues. Such a comdat concept probably does not work on COFF. Mach-O does not have comdat.
> 
> The pass serves for binary formats which I think are not your main concern, am I right? :) If you cannot find people willing to review the Mach-O/COFF part, I recommend that you drop this pass from this patch because it is not needed by ELF (and your use case will condition the support on ELF anyway)
Ah, ok I'll drop this pass then. The immediate use case for us is only ELF formats. I wasn't sure to what extent this should be supported for other binary formats, but I suppose those could be added down the line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77248