[lld] [lld][WebAssembly] Fix bitcode LTO order in archive parsing (PR #73095)
Heejin Ahn via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 22 01:01:12 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/2] [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/2] 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;
More information about the llvm-commits
mailing list