[lld] [llvm] [LLD] [COFF] Add a test to observe an unwanted effect of 3ab6209. NFC. (PR #72764)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Nov 18 14:31:25 PST 2023
Martin =?utf-8?q?Storsjö?= <martin at martin.st>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/72764 at github.com>
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-platform-windows
Author: Martin Storsjö (mstorsjo)
<details>
<summary>Changes</summary>
The commit 3ab6209a3f93bdbeec8e9b9fcc00a9a4980915ff has the undesired
effect of retaining every `__imp_` symbol, even if it isn't referenced
in any way. Add a testcase to observe this behaviour, to serve as
a reference point if this behaviour were to be improved later.
---
Full diff: https://github.com/llvm/llvm-project/pull/72764.diff
6 Files Affected:
- (modified) lld/COFF/SymbolTable.cpp (+3-1)
- (added) lld/test/COFF/lto-dllimport.ll (+41)
- (modified) lld/test/COFF/lto-imp-prefix.ll (+40-3)
- (modified) llvm/lib/LTO/LTO.cpp (+1-7)
- (removed) llvm/test/LTO/X86/Inputs/dllimport.ll (-17)
- (removed) llvm/test/LTO/X86/dllimport.ll (-30)
``````````diff
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
new file mode 100644
index 000000000000000..ddce0cb88fb562e
--- /dev/null
+++ b/lld/test/COFF/lto-dllimport.ll
@@ -0,0 +1,41 @@
+; REQUIRES: x86
+; RUN: split-file %s %t.dir
+
+; RUN: llvm-as %t.dir/main.ll -o %t.main.bc
+; RUN: llvm-as %t.dir/other.ll -o %t.other.bc
+; RUN: llc %t.dir/main.ll -o %t.main.obj --filetype=obj
+; RUN: llc %t.dir/other.ll -o %t.other.obj --filetype=obj
+
+; 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.obj %t.other.bc -entry:main -out:%t.exe
+; RUN: lld-link %t.main.bc %t.other.obj -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"
+target triple = "x86_64-pc-windows-msvc19.11.0"
+
+define dso_local i32 @main() local_unnamed_addr {
+entry:
+ %call = tail call i32 @foo()
+ ret i32 %call
+}
+
+declare dllimport i32 @foo() local_unnamed_addr
+
+!llvm.module.flags = !{!1}
+
+!1 = !{i32 1, !"ThinLTO", i32 0}
+
+;--- other.ll
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.11.0"
+
+define dso_local i32 @foo() local_unnamed_addr {
+entry:
+ ret i32 42
+}
+
+!llvm.module.flags = !{!1}
+
+!1 = !{i32 1, !"ThinLTO", i32 0}
diff --git a/lld/test/COFF/lto-imp-prefix.ll b/lld/test/COFF/lto-imp-prefix.ll
index 503e4fd0649e41d..5affe73922888a1 100644
--- a/lld/test/COFF/lto-imp-prefix.ll
+++ b/lld/test/COFF/lto-imp-prefix.ll
@@ -3,9 +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.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 %t1.exe | FileCheck %s
+
+; CHECK: __imp_unusedFunc
+
+; RUN: lld-link /entry:entry %t.main.obj %t.other2.obj /out:%t2.exe /subsystem:console
-; RUN: lld-link /entry:entry %t.main.obj %t.other.obj /out:%t.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"
@@ -22,7 +34,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"
@@ -33,6 +45,31 @@ entry:
ret void
}
+ at __imp_unusedFunc = global ptr @unusedFuncReplacement
+
+define internal void @unusedFuncReplacement() {
+entry:
+ ret void
+}
+
+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 &&
diff --git a/llvm/test/LTO/X86/Inputs/dllimport.ll b/llvm/test/LTO/X86/Inputs/dllimport.ll
deleted file mode 100644
index f3daab1df09742d..000000000000000
--- a/llvm/test/LTO/X86/Inputs/dllimport.ll
+++ /dev/null
@@ -1,17 +0,0 @@
-; ModuleID = 'b.obj'
-source_filename = "b.cpp"
-target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-pc-windows-msvc19.11.0"
-
-; Function Attrs: norecurse nounwind readnone sspstrong uwtable
-define dso_local i32 @"?foo@@YAHXZ"() local_unnamed_addr {
-entry:
- ret i32 42
-}
-
-!llvm.module.flags = !{!1}
-
-!1 = !{i32 1, !"ThinLTO", i32 0}
-
-^0 = module: (path: "b.obj", hash: (0, 0, 0, 0, 0))
-^1 = gv: (name: "?foo@@YAHXZ", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 1, live: 0, dsoLocal: 1), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0)))) ; guid = 2709792123250749187
diff --git a/llvm/test/LTO/X86/dllimport.ll b/llvm/test/LTO/X86/dllimport.ll
deleted file mode 100644
index eb9f7df5c8b30a5..000000000000000
--- a/llvm/test/LTO/X86/dllimport.ll
+++ /dev/null
@@ -1,30 +0,0 @@
-; Test requiring LTO to remove the __imp_ prefix for locally imported COFF
-; symbols (mirroring how lld handles these symbols).
-; RUN: llvm-as %s -o %t.obj
-; RUN: llvm-as %S/Inputs/dllimport.ll -o %t2.obj
-; RUN: llvm-lto2 run -r=%t.obj,main,px -r %t.obj,__imp_?foo@@YAHXZ -r %t2.obj,?foo@@YAHXZ,p -o %t3 %t.obj %t2.obj -save-temps
-; RUN: llvm-dis %t3.0.0.preopt.bc -o - | FileCheck %s
-
-; CHECK: define dso_local i32 @"?foo@@YAHXZ"()
-
-; ModuleID = 'a.obj'
-source_filename = "a.cpp"
-target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-pc-windows-msvc19.11.0"
-
-; Function Attrs: norecurse nounwind sspstrong uwtable
-define dso_local i32 @main() local_unnamed_addr {
-entry:
- %call = tail call i32 @"?foo@@YAHXZ"()
- ret i32 %call
-}
-
-declare dllimport i32 @"?foo@@YAHXZ"() local_unnamed_addr
-
-!llvm.module.flags = !{!1}
-
-!1 = !{i32 1, !"ThinLTO", i32 0}
-
-^0 = module: (path: "a.obj", hash: (0, 0, 0, 0, 0))
-^1 = gv: (name: "?foo@@YAHXZ") ; guid = 2709792123250749187
-^2 = gv: (name: "main", summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 1, live: 0, dsoLocal: 1), insts: 2, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0), calls: ((callee: ^1))))) ; guid = 15822663052811949562
``````````
</details>
https://github.com/llvm/llvm-project/pull/72764
More information about the llvm-commits
mailing list