[lld][ELF] Resolve Symbols with same comdat index as Defined Symbols

[pranav-159]

Context:
While creating BitcodeSymbol, if we resolve the symbols with same comdat index using Undefined Symbol then the exportDynamic property of symbol is not affected by symbols processed/resolved after first symbol which is incorrect (Example: first symbol processed may have exportDynamic property as false but the following symbol with same comdat index may have exportDynamic property as true. Here, in order to be correct, we should change the exportDynamic property of symbol to true at symbol resolution but is not happening in current code.)

Patch Details:

• Instead of Undefined Symbol used Defined Symbol to resolve symbols with same comdat index.
• Added a LIT testcase to demonstrate the issue

This patch solves the issue #111341.
@MaskRay Could you please review the patch.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: None (pranav-159)

Changes

While creating BitcodeSymbol, if we resolve the symbols with same comdat index using Undefined Symbol then the exportDynamic property of symbol is not affected by symbols processed/resolved after first symbol which is incorrect (Example: first symbol processed may have exportDynamic property as false but the following symbol with same comdat index may have exportDynamic property as true. Here, in order to be correct, we should change the exportDynamic property of symbol to true at symbol resolution but is not happening in current code.)
This patch solves the issue #111341.
@MaskRay Could you please review the patch.

---

Full diff: https://github.com/llvm/llvm-project/pull/115140.diff

2 Files Affected:

• (modified) lld/ELF/InputFiles.cpp (+11-2)
• (added) lld/test/ELF/lto/comdat-weakodr-linkonceodr-visibility.ll (+36)

diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 0d3db373138874..899b7565af8d1b 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1756,13 +1756,22 @@ static void createBitcodeSymbol(Ctx &ctx, Symbol *&sym,
     sym = ctx.symtab->insert(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(ctx, newSym);
     sym->referenced = true;
     return;
   }
+  int c = objSym.getComdatIndex();
+  if (c != -1 && !keptComdats[c]) {
+    Defined newSym(ctx, &f, StringRef(), binding, visibility, type, 0, 0,
+                   nullptr);
+    if (objSym.canBeOmittedFromSymbolTable())
+      newSym.exportDynamic = false;
+    sym->resolve(ctx, newSym);
+    sym->referenced = true;
+    return;
+  }
 
   if (objSym.isCommon()) {
     sym->resolve(ctx, CommonSymbol{ctx, &f, StringRef(), binding, visibility,
diff --git a/lld/test/ELF/lto/comdat-weakodr-linkonceodr-visibility.ll b/lld/test/ELF/lto/comdat-weakodr-linkonceodr-visibility.ll
new file mode 100644
index 00000000000000..776c7ff6f6bee7
--- /dev/null
+++ b/lld/test/ELF/lto/comdat-weakodr-linkonceodr-visibility.ll
@@ -0,0 +1,36 @@
+; RUN: rm -rf %t.dir
+; RUN: split-file %s %t.dir
+; RUN: cd %t.dir
+; RUN: echo %t.dir
+; RUN: llvm-as explicit.ll -o explicit.bc
+; RUN: llvm-as implicit.ll -o implicit.bc
+
+
+;; Case 1:
+; RUN: ld.lld explicit.bc implicit.bc -o case1.so -shared -save-temps
+; RUN: llvm-nm case1.so.0.2.internalize.bc | FileCheck %s
+
+;; Case 2:
+; RUN: ld.lld implicit.bc explicit.bc -o case2.so -shared -save-temps
+; RUN: llvm-nm case2.so.0.2.internalize.bc | FileCheck %s
+
+; CHECK: W foo
+
+;--- explicit.ll
+target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+$foo = comdat any
+define weak_odr void @foo() local_unnamed_addr comdat {
+  ret void
+}
+
+
+;--- implicit.ll
+target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+$foo = comdat any
+define linkonce_odr void @foo() local_unnamed_addr comdat {
+  ret void
+}

[pranav-159]

Ping

[pranav-159]

Ping @rui314 @pcc

[MaskRay]

Thanks for writing the initial patch, but I am going to take over #119332 Note, we could just drop keptComdat in this function. I am going to add more linkage tests as well.

[MaskRay]

Obsoleted by #119332