From d79b51cbf6cea660a80452eb11caa342cad76d3b Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 21 Nov 2023 16:41:33 -0800 Subject: [PATCH 1/7] [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 00000000000000..1478a9070ea0ec --- /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 + +@unused = linkonce_odr global i32 0, comdat, align 4 +@_ZGV6unused = linkonce_odr global i32 0, comdat($unused), align 4 +@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 00000000000000..66f60e1cf3e3bb --- /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 + +@unused = linkonce_odr global i32 0, comdat, align 4 +@_ZGV6unused = linkonce_odr global i32 0, comdat($unused), align 4 +@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 00000000000000..f1b09a092a1fcf --- /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 a00e336118d8c8..6d283c815d4470 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(file)) { - f->parse(symName); bitcodeFiles.push_back(f); + f->parse(symName); return; } From 90af3eb68885777c23b3129a7750e2b8a7ba1cd9 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 22 Nov 2023 01:00:53 -0800 Subject: [PATCH 2/7] 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 6d283c815d4470..76370525c37199 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(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 Date: Wed, 22 Nov 2023 01:07:56 -0800 Subject: [PATCH 3/7] 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 1478a9070ea0ec..913c278ecf4a1e 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 66f60e1cf3e3bb..8d59f2a7224cef 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 Date: Wed, 22 Nov 2023 01:08:52 -0800 Subject: [PATCH 4/7] 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 913c278ecf4a1e..fdc1f7d505b39f 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 8d59f2a7224cef..6a1fd0828ed172 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 Date: Wed, 22 Nov 2023 13:19:56 -0800 Subject: [PATCH 5/7] 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 f1b09a092a1fcf..01d627a436a114 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 Date: Tue, 28 Nov 2023 12:48:21 -0800 Subject: [PATCH 6/7] 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 fdc1f7d505b39f..b866c6efeba10e 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 6a1fd0828ed172..58ab5122bad881 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 01d627a436a114..9bea3d351dc92a 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 From c21e792863ff0d6fffc0c24f498c442b1364c069 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 28 Nov 2023 14:32:52 -0800 Subject: [PATCH 7/7] Missing ; --- lld/test/wasm/lto/comdat_ordering.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lld/test/wasm/lto/comdat_ordering.test b/lld/test/wasm/lto/comdat_ordering.test index 9bea3d351dc92a..12a0efc39aff34 100644 --- a/lld/test/wasm/lto/comdat_ordering.test +++ b/lld/test/wasm/lto/comdat_ordering.test @@ -1,5 +1,5 @@ ; Check if we handle a variable (here __cxx_global_var_init) in different LTO -bitcode modules sharing a comdat. +; bitcode modules sharing a comdat. ; RUN: llvm-as %S/Inputs/comdat_ordering1.ll -o %t1.o ; RUN: llvm-as %S/Inputs/comdat_ordering2.ll -o %t2.o