[lld] [lld][WebAssembly] Fix bitcode LTO order in archive parsing (PR #73095)
Heejin Ahn via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 28 14:21:02 PST 2023
https://github.com/aheejin updated https://github.com/llvm/llvm-project/pull/73095
>From d79b51cbf6cea660a80452eb11caa342cad76d3b Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Tue, 21 Nov 2023 16:41:33 -0800
Subject: [PATCH 1/6] [lld][WebAssembly] Fix bitcode LTO order in archive
parsing
tl;dr:
When doing LTO on multiple archives, the order with which bitcodes are
linked to the LTO module is hard to control, given that processing
undefined symbols can lead to parsing of an object file, which in turn
lead to parsing of another object file before finishing parsing of the
previous file. This can result in encountering a non-prevailing comdat
first when linking, which can make the the symbol undefined, and the
real definition is added later with an additional prefix to avoid
duplication (e.g. `__cxx_global_var_init` and `__cxx_global_var_init.2`)
So this one-line fix ensures we compile bitcodes in the order that we
process comdats, so that when multiple archived bitcode files have the
same variable with the same comdat, we make sure that the prevailing
comdat will be linked first in the LTO.
Fixes #62243.
---
- Long version, feel free to skip:
When linking (non-archived) bitcode files directly in LTO, we add files
in `bitcodeFiles` vector in the order they are given in the command
line:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/Driver.cpp#L1201-L1204 ->
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/SymbolTable.cpp#L54
The order they are added in `bitcodeFiles` is the order they are
compiled and linked into the LTO module later.
While parsing these bitcode files, we also add symbols to the symbol
table. Depending on their status, they are added as defined symbols or
undefined symbols.
---
On the other hand, when linking archives that contain bitcode files, the
order bitcode files are added to `bitcodeFiles` vector is hard to
predict.
When parsing archives, firstly, symbols are parsed as lazy symbols:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/InputFiles.cpp#L734-L739
And then we handle undefined symbols here:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/Driver.cpp#L1208-L1210
where we try to fetch lazy symbols' original objects:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/Driver.cpp#L708 ->
https://github.com/llvm/llvm-project/blob/907ed77ad1d7a154317e5f8398d17d441711dc38/lld/wasm/Symbols.cpp#L428
which causes the corresponding object file to be added to the symbol
table:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/InputFiles.cpp#L763
and the object file is parsed:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/SymbolTable.cpp#L53
But this `parse` call can lead to the parsing of other files before it
is added to `bitcodeFiles` in the next line. For example, we have two
bitcode files `a.o` and `b.o`, each of which is archived as `liba.a` and
`libb.a`. Both files have a variable with the same name and the same
comdat. When handling undefined symbols, suppose we start from a symbol
from `a.o`. We arrive here and all comdats in `a.o` are added to
`keptComdats`:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/InputFiles.cpp#L844-L845
And we call `createBitCodeSymbol` on all symbols in `a.o`:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/InputFiles.cpp#L847-L848
But this can cause the loading of another object in case some of those
symbols are undefined functions or data, like here:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/InputFiles.cpp#L791-L792
->
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/SymbolTable.cpp#L534
So effectively, in the middle of parsing `a.o`, we proceed to parse
`b.o`. And when parsing `b.o`, we encounter the comdat variable. But
because we already added that comdat in `keptComdats` when parsing
`a.o`, so `b.o`'s comdat variable ends up being excluded, so it is added
as an undefined data, whereas `a.o`'s comdat variable will be added as a
defined data:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/InputFiles.cpp#L786-L798
For a lazy symbol, `addUndefinedData` does not replace the symbol unless
it is the first time being inserted, but `addDefinedData` does:
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/SymbolTable.cpp#L382-L385
And because of this `a.o`'s comdat is to be the prevailing one, because
the symbol's `getFile()` will return `a.o`:
https://github.com/llvm/llvm-project/blob/907ed77ad1d7a154317e5f8398d17d441711dc38/lld/wasm/LTO.cpp#L104
But because `b.o`'s parsing started in the middle of `a.o`s parsing, it
ends first and gets added to `bitcodeFiles` first.
https://github.com/llvm/llvm-project/blob/fb57f4e0e0b302ec1b3181e952a4bd4b3c57a286/lld/wasm/SymbolTable.cpp#L54
So to sum up, `bitcodeFiles` now contains [`b.o`, `a.o`] but `a.o` has
the prevailing comdat. This does not happen when directly linking
bitcodes (without archives) because symbols are added in one-pass in the
order specified in the command line. This also does not happen in
non-LTO (even with archives), because object parsing uses
`ObjFile::parse` which is a separate process from this bitcode
processing.
---
I'm not sure if there is a rule that the prevailing comdat has to exist
in the first file having that comdat within `bitcodeFiles`, but my
observation says this is likely the case.
When adding bitcodes to the LTO, if a symbol's comdat is not a
prevailing one, this code demotes its linkage to `available_externally`:
https://github.com/llvm/llvm-project/blob/23c84fb362849865990c0e160158b19f54742147/llvm/lib/LTO/LTO.cpp#L869-L895
and subsequently all other variables that are associated with the same
comdat in that object file are demoted in the same way.
https://github.com/llvm/llvm-project/blob/23c84fb362849865990c0e160158b19f54742147/llvm/lib/LTO/LTO.cpp#L788-L792
In out case, because of the orders of `bitcodeFiles`, we start from
`b.o`, and the comdat symbols there become `available_externally`. This
is how `__cxx_global_var_init` in `global_var_init2.ll` in the attached
test becomes `available_externally`.
When linking those bitcodes in the LTO, if we traverse symbols in
the order of prevailing->non-prevailing, we end up keeping only one
global variable, but if we traverse in the other way around (i.e.,
non-prevailing first), we end up adding two symbols, because it lets us
add a `available_externally` symbol when we don't already have a
definition for it:
https://github.com/llvm/llvm-project/blob/23c84fb362849865990c0e160158b19f54742147/llvm/lib/LTO/LTO.cpp#L974-L986
Also when it is an `available_externally` symbol, it becomes a
declaration, not a definition. So we end up with a module like this:
```ll
declare dso_local void @__cxx_global_var_init()
define internal void @__cxx_global_var_init.2() comdat($unused) {
...
}
```
---
So this one-line fix ensures we compile bitcodes in the order that we
process comdats, so that when multiple archived bitcode files have the
same variable with the same comdat, we make sure that the prevailing
comdat will be linked first in the LTO.
---
This crash is said to be happening after https://github.com/llvm/llvm-project/commit/12050a3fb7344694cfd7527d4cca0033729bcfc5
but even reverting this is not really a solution for us because we end
up with two definitions if we do that:
```ll
define internal void @__cxx_global_var_init() comdat($unused) {
...
}
define internal void @__cxx_global_var_init.2() comdat($unused) {
...
}
```
And the wasm's `__wasm_call_ctors` will be like
```wast
(func $__wasm_call_ctors
(call $emscripten_stack_init)
(call $__cxx_global_var_init)
(call $__cxx_global_var_init.2)
)
```
---
lld/test/wasm/lto/Inputs/global_var_init1.ll | 33 ++++++++++++++++++++
lld/test/wasm/lto/Inputs/global_var_init2.ll | 30 ++++++++++++++++++
lld/test/wasm/lto/global_var_init.test | 17 ++++++++++
lld/wasm/SymbolTable.cpp | 2 +-
4 files changed, 81 insertions(+), 1 deletion(-)
create mode 100644 lld/test/wasm/lto/Inputs/global_var_init1.ll
create mode 100644 lld/test/wasm/lto/Inputs/global_var_init2.ll
create mode 100644 lld/test/wasm/lto/global_var_init.test
diff --git a/lld/test/wasm/lto/Inputs/global_var_init1.ll b/lld/test/wasm/lto/Inputs/global_var_init1.ll
new file mode 100644
index 000000000000000..1478a9070ea0ec8
--- /dev/null
+++ b/lld/test/wasm/lto/Inputs/global_var_init1.ll
@@ -0,0 +1,33 @@
+target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+$unused = comdat any
+
+ at unused = linkonce_odr global i32 0, comdat, align 4
+ at _ZGV6unused = linkonce_odr global i32 0, comdat($unused), align 4
+ at llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__cxx_global_var_init, ptr @unused }]
+
+define internal void @__cxx_global_var_init() comdat($unused) {
+entry:
+ %0 = load i8, ptr @_ZGV6unused, align 4
+ %1 = and i8 %0, 1
+ %guard.uninitialized = icmp eq i8 %1, 0
+ br i1 %guard.uninitialized, label %init.check, label %init.end
+
+init.check: ; preds = %entry
+ store i8 1, ptr @_ZGV6unused, align 4
+ %call = call i32 @foo()
+ store i32 %call, ptr @unused, align 4
+ br label %init.end
+
+init.end: ; preds = %init.check, %entry
+ ret void
+}
+
+declare i32 @foo()
+
+define i32 @main() {
+entry:
+ %call = call i32 @foo()
+ ret i32 %call
+}
diff --git a/lld/test/wasm/lto/Inputs/global_var_init2.ll b/lld/test/wasm/lto/Inputs/global_var_init2.ll
new file mode 100644
index 000000000000000..66f60e1cf3e3bbf
--- /dev/null
+++ b/lld/test/wasm/lto/Inputs/global_var_init2.ll
@@ -0,0 +1,30 @@
+target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+$unused = comdat any
+
+ at unused = linkonce_odr global i32 0, comdat, align 4
+ at _ZGV6unused = linkonce_odr global i32 0, comdat($unused), align 4
+ at llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__cxx_global_var_init, ptr @unused }]
+
+define internal void @__cxx_global_var_init() comdat($unused) {
+entry:
+ %0 = load i8, ptr @_ZGV6unused, align 4
+ %1 = and i8 %0, 1
+ %guard.uninitialized = icmp eq i8 %1, 0
+ br i1 %guard.uninitialized, label %init.check, label %init.end
+
+init.check: ; preds = %entry
+ store i8 1, ptr @_ZGV6unused, align 4
+ %call = call i32 @foo()
+ store i32 %call, ptr @unused, align 4
+ br label %init.end
+
+init.end: ; preds = %init.check, %entry
+ ret void
+}
+
+define i32 @foo() {
+entry:
+ ret i32 42
+}
diff --git a/lld/test/wasm/lto/global_var_init.test b/lld/test/wasm/lto/global_var_init.test
new file mode 100644
index 000000000000000..f1b09a092a1fcfb
--- /dev/null
+++ b/lld/test/wasm/lto/global_var_init.test
@@ -0,0 +1,17 @@
+; Check if we handle __cxx_global_var_init in different LTO bitcode modules
+; sharing a comdat.
+
+; RUN: llvm-as %S/Inputs/global_var_init1.ll -o %t1.o
+; RUN: llvm-as %S/Inputs/global_var_init2.ll -o %t2.o
+; RUN: llvm-ar rcs %t1.a %t1.o
+; RUN: llvm-ar rcs %t2.a %t2.o
+; RUN: wasm-ld %t1.a %t2.a -o %t.wasm --no-entry --export=main --export=__wasm_call_ctors
+; RUN: obj2yaml %t.wasm | FileCheck %s
+
+; CHECK: - Type: CUSTOM
+; CHECK-NEXT: Name: name
+; CHECK-NEXT: FunctionNames:
+; CHECK-NEXT: - Index: 0
+; CHECK-NEXT: Name: __wasm_call_ctors
+; CHECK-NEXT: - Index: 1
+; CHECK-NEXT: Name: __cxx_global_var_init
diff --git a/lld/wasm/SymbolTable.cpp b/lld/wasm/SymbolTable.cpp
index a00e336118d8c84..6d283c815d44701 100644
--- a/lld/wasm/SymbolTable.cpp
+++ b/lld/wasm/SymbolTable.cpp
@@ -50,8 +50,8 @@ void SymbolTable::addFile(InputFile *file, StringRef symName) {
// LLVM bitcode file
if (auto *f = dyn_cast<BitcodeFile>(file)) {
- f->parse(symName);
bitcodeFiles.push_back(f);
+ f->parse(symName);
return;
}
>From 90af3eb68885777c23b3129a7750e2b8a7ba1cd9 Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Wed, 22 Nov 2023 01:00:53 -0800
Subject: [PATCH 2/6] Add comment with the PR link
---
lld/wasm/SymbolTable.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lld/wasm/SymbolTable.cpp b/lld/wasm/SymbolTable.cpp
index 6d283c815d44701..76370525c371995 100644
--- a/lld/wasm/SymbolTable.cpp
+++ b/lld/wasm/SymbolTable.cpp
@@ -50,6 +50,8 @@ void SymbolTable::addFile(InputFile *file, StringRef symName) {
// LLVM bitcode file
if (auto *f = dyn_cast<BitcodeFile>(file)) {
+ // This order, first adding to `bitcodeFiles` and then parsing is necessary.
+ // See https://github.com/llvm/llvm-project/pull/73095
bitcodeFiles.push_back(f);
f->parse(symName);
return;
>From f880bfc9a9bc18386e609d5eb93c570813c14dc5 Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Wed, 22 Nov 2023 01:07:56 -0800
Subject: [PATCH 3/6] Paste source C++ code as comments
---
lld/test/wasm/lto/Inputs/global_var_init1.ll | 9 +++++++++
lld/test/wasm/lto/Inputs/global_var_init2.ll | 9 +++++++++
2 files changed, 18 insertions(+)
diff --git a/lld/test/wasm/lto/Inputs/global_var_init1.ll b/lld/test/wasm/lto/Inputs/global_var_init1.ll
index 1478a9070ea0ec8..913c278ecf4a1e5 100644
--- a/lld/test/wasm/lto/Inputs/global_var_init1.ll
+++ b/lld/test/wasm/lto/Inputs/global_var_init1.ll
@@ -1,6 +1,15 @@
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
+; Generated from this C++ code
+;
+; int foo();
+; inline int unused = foo();
+;
+; int main() {
+; return foo();
+; }
+
$unused = comdat any
@unused = linkonce_odr global i32 0, comdat, align 4
diff --git a/lld/test/wasm/lto/Inputs/global_var_init2.ll b/lld/test/wasm/lto/Inputs/global_var_init2.ll
index 66f60e1cf3e3bbf..8d59f2a7224cef6 100644
--- a/lld/test/wasm/lto/Inputs/global_var_init2.ll
+++ b/lld/test/wasm/lto/Inputs/global_var_init2.ll
@@ -1,6 +1,15 @@
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
+; Generated from this C++ code
+;
+; int foo();
+; inline int unused = foo();
+;
+; int foo() {
+; return 42;
+; }
+
$unused = comdat any
@unused = linkonce_odr global i32 0, comdat, align 4
>From 194372295fbf301c5b11ed648c9d6d029829e59e Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Wed, 22 Nov 2023 01:08:52 -0800
Subject: [PATCH 4/6] add :
---
lld/test/wasm/lto/Inputs/global_var_init1.ll | 2 +-
lld/test/wasm/lto/Inputs/global_var_init2.ll | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/lld/test/wasm/lto/Inputs/global_var_init1.ll b/lld/test/wasm/lto/Inputs/global_var_init1.ll
index 913c278ecf4a1e5..fdc1f7d505b39f1 100644
--- a/lld/test/wasm/lto/Inputs/global_var_init1.ll
+++ b/lld/test/wasm/lto/Inputs/global_var_init1.ll
@@ -1,7 +1,7 @@
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
-; Generated from this C++ code
+; Generated from this C++ code:
;
; int foo();
; inline int unused = foo();
diff --git a/lld/test/wasm/lto/Inputs/global_var_init2.ll b/lld/test/wasm/lto/Inputs/global_var_init2.ll
index 8d59f2a7224cef6..6a1fd0828ed1721 100644
--- a/lld/test/wasm/lto/Inputs/global_var_init2.ll
+++ b/lld/test/wasm/lto/Inputs/global_var_init2.ll
@@ -1,7 +1,7 @@
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
-; Generated from this C++ code
+; Generated from this C++ code:
;
; int foo();
; inline int unused = foo();
>From 72804e9d89d4aecf79de22f3fcd9a3970862a217 Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Wed, 22 Nov 2023 13:19:56 -0800
Subject: [PATCH 5/6] Add `CHECK-NOT` to make sure we don't have duplicate defs
---
lld/test/wasm/lto/global_var_init.test | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lld/test/wasm/lto/global_var_init.test b/lld/test/wasm/lto/global_var_init.test
index f1b09a092a1fcfb..01d627a436a114e 100644
--- a/lld/test/wasm/lto/global_var_init.test
+++ b/lld/test/wasm/lto/global_var_init.test
@@ -15,3 +15,5 @@
; CHECK-NEXT: Name: __wasm_call_ctors
; CHECK-NEXT: - Index: 1
; CHECK-NEXT: Name: __cxx_global_var_init
+
+; CHECK-NOT: Name: __cxx_global_var_init.2
>From a5591082441c7e9489c758883f1d194ffd8433d8 Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Tue, 28 Nov 2023 12:48:21 -0800
Subject: [PATCH 6/6] Rename tests
---
.../Inputs/{global_var_init1.ll => comdat_ordering1.ll} | 2 +-
.../Inputs/{global_var_init2.ll => comdat_ordering2.ll} | 2 +-
.../lto/{global_var_init.test => comdat_ordering.test} | 8 ++++----
3 files changed, 6 insertions(+), 6 deletions(-)
rename lld/test/wasm/lto/Inputs/{global_var_init1.ll => comdat_ordering1.ll} (95%)
rename lld/test/wasm/lto/Inputs/{global_var_init2.ll => comdat_ordering2.ll} (94%)
rename lld/test/wasm/lto/{global_var_init.test => comdat_ordering.test} (71%)
diff --git a/lld/test/wasm/lto/Inputs/global_var_init1.ll b/lld/test/wasm/lto/Inputs/comdat_ordering1.ll
similarity index 95%
rename from lld/test/wasm/lto/Inputs/global_var_init1.ll
rename to lld/test/wasm/lto/Inputs/comdat_ordering1.ll
index fdc1f7d505b39f1..b866c6efeba10ef 100644
--- a/lld/test/wasm/lto/Inputs/global_var_init1.ll
+++ b/lld/test/wasm/lto/Inputs/comdat_ordering1.ll
@@ -1,7 +1,7 @@
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
-; Generated from this C++ code:
+; Generated from this C++ code and simplified manually:
;
; int foo();
; inline int unused = foo();
diff --git a/lld/test/wasm/lto/Inputs/global_var_init2.ll b/lld/test/wasm/lto/Inputs/comdat_ordering2.ll
similarity index 94%
rename from lld/test/wasm/lto/Inputs/global_var_init2.ll
rename to lld/test/wasm/lto/Inputs/comdat_ordering2.ll
index 6a1fd0828ed1721..58ab5122bad8813 100644
--- a/lld/test/wasm/lto/Inputs/global_var_init2.ll
+++ b/lld/test/wasm/lto/Inputs/comdat_ordering2.ll
@@ -1,7 +1,7 @@
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
-; Generated from this C++ code:
+; Generated from this C++ code and simplified manually:
;
; int foo();
; inline int unused = foo();
diff --git a/lld/test/wasm/lto/global_var_init.test b/lld/test/wasm/lto/comdat_ordering.test
similarity index 71%
rename from lld/test/wasm/lto/global_var_init.test
rename to lld/test/wasm/lto/comdat_ordering.test
index 01d627a436a114e..9bea3d351dc92a3 100644
--- a/lld/test/wasm/lto/global_var_init.test
+++ b/lld/test/wasm/lto/comdat_ordering.test
@@ -1,8 +1,8 @@
-; Check if we handle __cxx_global_var_init in different LTO bitcode modules
-; sharing a comdat.
+; Check if we handle a variable (here __cxx_global_var_init) in different LTO
+bitcode modules sharing a comdat.
-; RUN: llvm-as %S/Inputs/global_var_init1.ll -o %t1.o
-; RUN: llvm-as %S/Inputs/global_var_init2.ll -o %t2.o
+; RUN: llvm-as %S/Inputs/comdat_ordering1.ll -o %t1.o
+; RUN: llvm-as %S/Inputs/comdat_ordering2.ll -o %t2.o
; RUN: llvm-ar rcs %t1.a %t1.o
; RUN: llvm-ar rcs %t2.a %t2.o
; RUN: wasm-ld %t1.a %t2.a -o %t.wasm --no-entry --export=main --export=__wasm_call_ctors
More information about the llvm-commits
mailing list