[lld] 89efffd - [LTO] [LLD] Don't alias the __imp_func and func symbol resolutions (#71376)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 21 05:06:05 PST 2023
Author: Martin Storsjö
Date: 2023-11-21T15:06:00+02:00
New Revision: 89efffd463ecc39cb17440ef7bb66d26141322aa
URL: https://github.com/llvm/llvm-project/commit/89efffd463ecc39cb17440ef7bb66d26141322aa
DIFF: https://github.com/llvm/llvm-project/commit/89efffd463ecc39cb17440ef7bb66d26141322aa.diff
LOG: [LTO] [LLD] Don't alias the __imp_func and func symbol resolutions (#71376)
Commit b963c0b658cc54b370832df4f5a3d63fd69da334 fixed LTO compilation of
cases where one translation unit is calling a function with the
dllimport attribute, and another translation unit provides this function
locally within the same linked module (i.e. not actually dllimported);
see https://github.com/llvm/llvm-project/issues/37453 or
https://bugs.llvm.org/show_bug.cgi?id=38105 for full context.
This was fixed by aliasing their GlobalResolution structs, for the
`__imp_` prefixed and non prefixed symbols.
I believe this fix to be wrong.
This patch reverts that fix, and fixes the same issue differently,
within LLD instead.
The fix assumed that one can treat the `__imp_` prefixed and unprefixed
symbols as equal, referencing SVN r240620
(d766653534e0cff702e42a43b44d3057b6094fea). However that referenced
commit had mistaken how this logic works, which was corrected later in
SVN r240622 (88e0f9206b4dccb56dee931adab08f89ff80525a); those symbols
aren't direct aliases for each other - but if there's a need for the
`__imp_` prefixed one and the other one exists, the `__imp_` prefixed
one is created, as a pointer to the other one.
However this fix only works if both translation units are compiled as
LTO; if the caller is compiled as a regular object file and the callee
is compiled as LTO, the fix fails, as the LTO compilation doesn't know
that the unprefixed symbol is needed.
The only level that knows of the potential relationship between the
`__imp_` prefixed and unprefixed symbol, across regular and bitcode
object files, is LLD itself.
Therefore, revert the original fix from
b963c0b658cc54b370832df4f5a3d63fd69da334, and fix the issue differently
- when concluding that we can fulfill an undefined symbol starting with
`__imp_`, mark the corresponding non prefixed symbol as used in a
regular object for the LTO compilation, to make sure that this non
prefixed symbol exists after the LTO compilation, to let LLD do the
fixup of the local import.
Extend the testcase to test a regular object file calling an LTO object
file, which previously failed.
This change also fixes another issue; an object file can provide both
unprefixed and prefixed versions of the same symbol, like this:
void importedFunc(void) {
}
void (*__imp_importedFunc)(void) = importedFunc;
That allows the function to be called both with and without dllimport
markings. (The concept of automatically resolving a reference to
`__imp_func` to a locally defined `func` only is done in MSVC style
linkers, but not in GNU ld, therefore MinGW mode code often uses this
construct.)
Previously, the aliasing of global resolutions at the LTO level would
trigger a failed assert with "Multiple prevailing defs are not allowed"
for this case, as both `importedFunc` and `__imp_importedFunc` could be
prevailing. Add a case to the existing LLD test case lto-imp-prefix.ll
to test this as well.
This change (together with previous change in
3ab6209a3f93bdbeec8e9b9fcc00a9a4980915ff) completes LLD to work with
mingw-w64-crt files (the base glue code for a mingw-w64 toolchain) built
with LTO.
Added:
Modified:
lld/COFF/SymbolTable.cpp
lld/test/COFF/lto-dllimport.ll
lld/test/COFF/lto-imp-prefix.ll
llvm/lib/LTO/LTO.cpp
Removed:
################################################################################
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index 15c76461a84a92c..44aa506d2c35da9 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -462,8 +462,10 @@ void SymbolTable::reportUnresolvable() {
StringRef name = undef->getName();
if (name.starts_with("__imp_")) {
Symbol *imp = find(name.substr(strlen("__imp_")));
- if (imp && isa<Defined>(imp))
+ if (Defined *def = dyn_cast_or_null<Defined>(imp)) {
+ def->isUsedInRegularObj = true;
continue;
+ }
}
if (name.contains("_PchSym_"))
continue;
diff --git a/lld/test/COFF/lto-dllimport.ll b/lld/test/COFF/lto-dllimport.ll
index 74f3f43d8c2810f..3bef2779ebdef7f 100644
--- a/lld/test/COFF/lto-dllimport.ll
+++ b/lld/test/COFF/lto-dllimport.ll
@@ -9,10 +9,7 @@
; RUN: lld-link %t.main.obj %t.other.obj -entry:main -out:%t.exe
; RUN: lld-link %t.main.bc %t.other.bc -entry:main -out:%t.exe
; RUN: lld-link %t.main.bc %t.other.obj -entry:main -out:%t.exe
-
-;; This test currently fails if combining the regular object file main.obj
-;; with the bitcode object file other.bc.
-; RUN-skipped: lld-link %t.main.obj %t.other.bc -entry:main -out:%t.exe
+; RUN: lld-link %t.main.obj %t.other.bc -entry:main -out:%t.exe
;--- main.ll
target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
diff --git a/lld/test/COFF/lto-imp-prefix.ll b/lld/test/COFF/lto-imp-prefix.ll
index c0b3bea7841c435..56a7c48cc9d168a 100644
--- a/lld/test/COFF/lto-imp-prefix.ll
+++ b/lld/test/COFF/lto-imp-prefix.ll
@@ -3,18 +3,21 @@
; RUN: rm -rf %t.dir
; RUN: split-file %s %t.dir
; RUN: llvm-as %t.dir/main.ll -o %t.main.obj
-; RUN: llvm-as %t.dir/other.ll -o %t.other.obj
+; RUN: llvm-as %t.dir/other1.ll -o %t.other1.obj
+; RUN: llvm-as %t.dir/other2.ll -o %t.other2.obj
-; RUN: lld-link /entry:entry %t.main.obj %t.other.obj /out:%t.exe /subsystem:console /debug:symtab
+; RUN: lld-link /entry:entry %t.main.obj %t.other1.obj /out:%t1.exe /subsystem:console /debug:symtab
;; The current implementation for handling __imp_ symbols retains all of them.
;; Observe that this currently produces __imp_unusedFunc even if nothing
;; references unusedFunc in any form.
-; RUN: llvm-nm %t.exe | FileCheck %s
+; RUN: llvm-nm %t1.exe | FileCheck %s
; CHECK: __imp_unusedFunc
+; RUN: lld-link /entry:entry %t.main.obj %t.other2.obj /out:%t2.exe /subsystem:console
+
;--- main.ll
target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-w64-windows-gnu"
@@ -30,7 +33,7 @@ declare dllimport void @importedFunc()
declare void @other()
-;--- other.ll
+;--- other1.ll
target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-w64-windows-gnu"
@@ -52,3 +55,21 @@ define void @other() {
entry:
ret void
}
+
+;--- other2.ll
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-w64-windows-gnu"
+
+ at __imp_importedFunc = global ptr @importedFunc
+
+; Test with two external symbols with the same name, with/without the __imp_
+; prefix.
+define void @importedFunc() {
+entry:
+ ret void
+}
+
+define void @other() {
+entry:
+ ret void
+}
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index e111e09681178e2..05836fd28f52882 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -610,13 +610,7 @@ void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
assert(ResI != ResE);
SymbolResolution Res = *ResI++;
- StringRef Name = Sym.getName();
- // Strip the __imp_ prefix from COFF dllimport symbols (similar to the
- // way they are handled by lld), otherwise we can end up with two
- // global resolutions (one with and one for a copy of the symbol without).
- if (TT.isOSBinFormatCOFF() && Name.startswith("__imp_"))
- Name = Name.substr(strlen("__imp_"));
- auto &GlobalRes = GlobalResolutions[Name];
+ auto &GlobalRes = GlobalResolutions[Sym.getName()];
GlobalRes.UnnamedAddr &= Sym.isUnnamedAddr();
if (Res.Prevailing) {
assert(!GlobalRes.Prevailing &&
More information about the llvm-commits
mailing list