[PATCH] D77248: [llvm][IR] Add dso_local_equivalent Constant
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Nov 14 10:34:10 PST 2020
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
More information about the llvm-commits
mailing list