[lld] [lld][WebAssembly] Fix bitcode LTO order in archive parsing (PR #73095)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 22 01:00:43 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lld
Author: Heejin Ahn (aheejin)
<details>
<summary>Changes</summary>
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)
)
```
---
Full diff: https://github.com/llvm/llvm-project/pull/73095.diff
4 Files Affected:
- (added) lld/test/wasm/lto/Inputs/global_var_init1.ll (+33)
- (added) lld/test/wasm/lto/Inputs/global_var_init2.ll (+30)
- (added) lld/test/wasm/lto/global_var_init.test (+17)
- (modified) lld/wasm/SymbolTable.cpp (+1-1)
``````````diff
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;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/73095
More information about the llvm-commits
mailing list