[ELF] Respect ltoCanOmit for symbols in non-prevailing COMDAT #119332
Conversation
Created using spr 1.3.5-bogner
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Fangrui Song (MaskRay) ChangesA linkonce_odr definition can be omitted in LTO compilation if Currently, we don't respect the To fix #111341, allow the non-prevailing COMDAT code path to clear the Full diff: https://github.com/llvm/llvm-project/pull/119332.diff 3 Files Affected:
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 2084fcfd4d651a..c44773d0b7dabe 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1709,7 +1709,6 @@ static uint8_t mapVisibility(GlobalValue::VisibilityTypes gvVisibility) {
}
static void createBitcodeSymbol(Ctx &ctx, Symbol *&sym,
- const std::vector<bool> &keptComdats,
const lto::InputFile::Symbol &objSym,
BitcodeFile &f) {
uint8_t binding = objSym.isWeak() ? STB_WEAK : STB_GLOBAL;
@@ -1726,8 +1725,7 @@ 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;
@@ -1766,10 +1764,10 @@ void BitcodeFile::parse() {
// ObjFile<ELFT>::initializeSymbols.
for (auto [i, irSym] : llvm::enumerate(obj->symbols()))
if (!irSym.isUndefined())
- createBitcodeSymbol(ctx, symbols[i], keptComdats, irSym, *this);
+ createBitcodeSymbol(ctx, symbols[i], irSym, *this);
for (auto [i, irSym] : llvm::enumerate(obj->symbols()))
if (irSym.isUndefined())
- createBitcodeSymbol(ctx, symbols[i], keptComdats, irSym, *this);
+ createBitcodeSymbol(ctx, symbols[i], irSym, *this);
for (auto l : obj->getDependentLibraries())
addDependentLibrary(ctx, l, this);
diff --git a/lld/test/ELF/lto/Inputs/internalize-exportdyn.ll b/lld/test/ELF/lto/Inputs/internalize-exportdyn.ll
index 585b99ae5a513a..5bfe15c92b3f18 100644
--- a/lld/test/ELF/lto/Inputs/internalize-exportdyn.ll
+++ b/lld/test/ELF/lto/Inputs/internalize-exportdyn.ll
@@ -1,6 +1,31 @@
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"
+$ext_and_ext = comdat any
+$lo_and_ext = comdat any
+$lo_and_wo = comdat any
+$wo_and_lo = comdat any
+
+declare void @foo(i64)
+
define weak_odr void @bah() {
ret void
}
+
+define void @ext_and_ext() local_unnamed_addr comdat {
+ call void @foo(i64 2)
+ ret void
+}
+
+define linkonce_odr void @lo_and_ext() local_unnamed_addr comdat {
+ call void @foo(i64 2)
+ ret void
+}
+
+define weak_odr void @lo_and_wo() local_unnamed_addr comdat {
+ ret void
+}
+
+define linkonce_odr void @wo_and_lo() local_unnamed_addr comdat {
+ ret void
+}
diff --git a/lld/test/ELF/lto/internalize-exportdyn.ll b/lld/test/ELF/lto/internalize-exportdyn.ll
index f02d3b375dad50..da3bf8e884505b 100644
--- a/lld/test/ELF/lto/internalize-exportdyn.ll
+++ b/lld/test/ELF/lto/internalize-exportdyn.ll
@@ -1,14 +1,22 @@
; REQUIRES: x86
-; RUN: llvm-as %s -o %t.o
-; RUN: llvm-as %p/Inputs/internalize-exportdyn.ll -o %t2.o
-; RUN: ld.lld %t.o %t2.o -o %t2 --export-dynamic -save-temps
-; RUN: llvm-dis < %t2.0.2.internalize.bc | FileCheck %s
-; RUN: ld.lld %t.o %t2.o -o %t3 -shared -save-temps
-; RUN: llvm-dis < %t3.0.2.internalize.bc | FileCheck %s --check-prefix=DSO
+; RUN: rm -rf %t && split-file %s %t && cd %t
+; RUN: llvm-as a.ll -o a.bc
+; RUN: llvm-as %p/Inputs/internalize-exportdyn.ll -o b.bc
+; RUN: llvm-mc -filetype=obj -triple=x86_64 lib.s -o lib.o
+; RUN: ld.lld a.bc b.bc lib.o -o out --export-dynamic -save-temps
+; RUN: llvm-dis < out.0.2.internalize.bc | FileCheck %s
+; RUN: ld.lld a.bc b.bc lib.o -o out2 -shared -save-temps
+; RUN: llvm-dis < out2.0.2.internalize.bc | FileCheck %s --check-prefix=DSO
+;--- a.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"
+$ext_and_ext = comdat any
+$lo_and_ext = comdat any
+$lo_and_wo = comdat any
+$wo_and_lo = comdat any
+
@c = linkonce_odr constant i32 1
@g = linkonce_odr global i32 1
@u_c = linkonce_odr unnamed_addr constant i32 1
@@ -16,6 +24,8 @@ target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16
@lu_c = linkonce_odr local_unnamed_addr constant i32 1
@lu_g = linkonce_odr local_unnamed_addr global i32 1
+declare void @lib(i64)
+
define void @_start() {
ret void
}
@@ -46,6 +56,24 @@ define linkonce_odr void @baz() {
@use_baz = global ptr @baz
+define void @ext_and_ext() local_unnamed_addr comdat {
+ call void @foo(i64 1)
+ ret void
+}
+
+define linkonce_odr void @lo_and_ext() local_unnamed_addr comdat {
+ call void @foo(i64 1)
+ ret void
+}
+
+define linkonce_odr void @lo_and_wo() local_unnamed_addr comdat {
+ ret void
+}
+
+define weak_odr void @wo_and_lo() local_unnamed_addr comdat {
+ ret void
+}
+
; Check what gets internalized.
; CHECK: @c = weak_odr dso_local constant i32 1
; CHECK: @g = weak_odr dso_local global i32 1
@@ -60,6 +88,12 @@ define linkonce_odr void @baz() {
; CHECK: define internal void @zed2()
; CHECK: define weak_odr dso_local void @bah()
; CHECK: define weak_odr dso_local void @baz()
+; CHECK: define dso_local void @ext_and_ext() comdat
+; CHECK-NEXT: call void @foo(i64 1)
+; CHECK: define internal void @lo_and_ext() comdat
+; CHECK-NEXT: call void @foo(i64 1)
+; CHECK: define weak_odr dso_local void @lo_and_wo() comdat
+; CHECK: define weak_odr dso_local void @wo_and_lo() comdat
; DSO: @c = weak_odr constant i32 1
; DSO: @g = weak_odr global i32 1
@@ -74,3 +108,11 @@ define linkonce_odr void @baz() {
; DSO: define internal void @zed2()
; DSO: define weak_odr void @bah()
; DSO: define weak_odr void @baz()
+; DSO: define void @ext_and_ext() comdat
+; DSO: define internal void @lo_and_ext() comdat
+; DSO: define weak_odr void @lo_and_wo() comdat
+; DSO: define weak_odr void @wo_and_lo() comdat
+
+;--- lib.s
+.globl lib
+lib:
|
There was a problem hiding this comment.
I found this quite difficult to review, although I think that's down to ELF and bitcode having subtle differences. It may have helped to review c650880 as that has some related changes.
Just to make sure I've understood.
A weak_odr, according to https://llvm.org/docs/LangRef.html#linkage-types , may not be discarded. A linkonce_odr is permitted to be discarded.
If an ELF group is prevailing then all is well as the ELF group will have all the definitions.
If the weak_odr group prevails then all is well as the symbol is not permitted to be discarded.
If the linkonce_odr group prevails over a weak_odr group then we must in effect make the prevailing definition weak_odr so that LTO does not remove it.
We do this by creating a new definition for all non-prevailing groups, which may clear the ltoCanOmit flag. This via Symbol::includeInDynsym(Ctx &ctx) communicates to LTO that the symbol is preserved. As these symbols are bitcode symbols in non-prevailing groups they do not generate multiple definition errors.
Assuming I've understood the code changes look good.
| 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" | ||
|
|
||
| $ext_and_ext = comdat any |
There was a problem hiding this comment.
It took me a while to work out what these meant, as I didn't realise that the definitions in Inputs were different. Could be worth a quick comment explaining the naming convention.
|
Thanks for taking a look. Yes, this is complex... --export-dynamic and -shared links exported all definitions, even linkonce_odr ones. For a symbol in a non-prevailing COMDAT, we can now use This patch is like the bitcode counterpart of the ObjectFile part of https://reviews.llvm.org/D120626 . |
Created using spr 1.3.5-bogner
MaskRay
deleted the
users/MaskRay/spr/elf-respect-ltocanomit-for-symbols-in-non-prevailing-comdat
branch
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 fromsymbols 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
ltoCanOmitbit, so thatVisibleToRegularObjcould be false inLTO.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).