Skip to content

[lld] Incorrectly Internalizing a symbol defined through explicit template instantiation when using '-flto -fuse-ld=lld' #111341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pranav-159 opened this issue Oct 7, 2024 · 3 comments · Fixed by #119332
Labels

Comments

@pranav-159
Copy link

When creating a shared library of which there are two files with implicit and explicit template instantiations of a particular class template respectively. If the file with the implicit instantiation comes first at linking, then the member functions get internalized by lld. (If the order is reverse i.e, explicit instantiation file comes first then the member functions won't get internalized.)
Note: When using llvm-gold plugin this issue is not seen.
In the reproducer. We created a shared library as mentioned above and linked it with an executable with extern template which causes an undefined symbol error as the required functions are internalized by lld when creating the shared library, which is not expected of the linker

@github-actions github-actions bot added the lld label Oct 7, 2024
@pranav-159
Copy link
Author

This issue seems to occur after refactoring the symbol resolution code in lld ELF ac2911, 9e6840

@pranav-159
Copy link
Author

Can this issue be resolved by changing how Comdat symbols are created and resolved in lld/ELF/InputFiles.cpp, createBitcodeSymbol function as Defined rather than Undefined as shown below?

diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 7adc35f20984..eab63ee9b7a0 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1748,12 +1748,20 @@ createBitcodeSymbol(Symbol *&sym, const std::vector<bool> &keptComdats,
     sym = symtab.insert(saver().save(objSym.getName()));

   int c = objSym.getComdatIndex();
-  if (objSym.isUndefined() || (c != -1 && !keptComdats[c])) {
+  if (objSym.isUndefined()) {
     Undefined newSym(&f, StringRef(), binding, visibility, type);
     sym->resolve(newSym);
     sym->referenced = true;
     return;
   }
+  if ((c != -1 && !keptComdats[c])) {
+    Defined newSym(&f, StringRef(), binding, visibility, type, 0, 0, nullptr);
+    if (objSym.canBeOmittedFromSymbolTable())
+      newSym.exportDynamic = false;
+    sym->resolve(newSym);
+    sym->referenced = true;
+    return;
+  }

   if (objSym.isCommon()) {
     sym->resolve(CommonSymbol{&f, StringRef(), binding, visibility, STT_OBJECT,

@EugeneZelenko EugeneZelenko added lld:ELF and removed lld labels Oct 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 7, 2024

@llvm/issue-subscribers-lld-elf

Author: None (pranav-159)

When creating a shared library of which there are two files with implicit and explicit template instantiations of a particular class template respectively. If the file with the implicit instantiation comes first at linking, then the member functions get internalized by lld. (If the order is reverse i.e, explicit instantiation file comes first then the member functions won't get internalized.) Note: When using llvm-gold plugin this issue is not seen. In the [reproducer](https://godbolt.org/z/7nov5Tc84). We created a shared library as mentioned above and linked it with an executable with extern template which causes an undefined symbol error as the required functions are internalized by lld when creating the shared library, which is not expected of the linker

MaskRay added a commit to MaskRay/llvm-project that referenced this issue Dec 10, 2024
A linkonce_odr definition can be omitted in LTO compilation if
`canBeOmittedFromSymbolTable()` is true in all bitcode files.

Currently, we don't respect the `canBeOmittedFromSymbolTable()` bit from
symbols in a non-prevailing COMDAT, which could lead to incorrect
omission of a definition when merging a prevailing linkonce_odr and a
non-prevailing weak_odr, e.g. an implicit template instantiation and an
explicit template instantiation.

To fix llvm#111341, allow the non-prevailing COMDAT code path to clear the
`ltoCanOmit` bit, so that `VisibleToRegularObj` could be false in
LTO.cpp. We could resolve either an Undefined or a Defined. For
simplicity, just use a Defined like the prevailing case.
MaskRay added a commit that referenced this issue Dec 10, 2024
A linkonce_odr definition can be omitted in LTO compilation if
`canBeOmittedFromSymbolTable()` is true in all bitcode files.
Test more linkage merge scenarios.

The lo_and_wo symbol tests #111341.
MaskRay added a commit that referenced this issue Dec 11, 2024
A linkonce_odr definition can be omitted in LTO compilation if
`canBeOmittedFromSymbolTable()` is true in all bitcode files.

Currently, we don't respect the `canBeOmittedFromSymbolTable()` bit from
symbols in a non-prevailing COMDAT, which could lead to incorrect
omission of a definition when merging a prevailing linkonce_odr and a
non-prevailing weak_odr, e.g. an implicit template instantiation and an
explicit template instantiation.

To fix #111341, allow the non-prevailing COMDAT code path to clear the
`ltoCanOmit` bit, so that `VisibleToRegularObj` could be false in
LTO.cpp. We could resolve either an Undefined or a Defined. For
simplicity, just use a Defined like the prevailing case (similar to how
we resolve symbols in ObjectFile COMDAT reviews.llvm.org/D120626).

Pull Request: #119332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants