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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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());
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77248