[LTO] [LLD] Don't alias the __imp_func and func symbol resolutions (#71376)

Commit b963c0b 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 #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
(d766653). However that referenced
commit had mistaken how this logic works, which was corrected later in
SVN r240622 (88e0f92); 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
b963c0b, 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
3ab6209) completes LLD to work with
mingw-w64-crt files (the base glue code for a mingw-w64 toolchain) built
with LTO.

Author: mstorsjo

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;

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"

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"
+
+@__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
+}

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 &&