[LTO] Handle __imp_ (dllimport) symbols consistently with lld

[tejohnson]

Summary

Similar to what lld already does for dllimport symbols which are
prefaced with imp_ (see lld patch r240620), strip off the imp_
prefix in LTO. Otherwise we can get 2 separate GlobalResolution for
a single symbol, the dllimport declaration, and the definition, which
leads to incorrect LTO handling.

Tests coming in separate tools/lld patch.

Fixes PR38105.

[dexonsmith]

Is it impossible to also test this locally (in the LLVM repo)? It will be hard to hack on this file if there aren't local tests.

[tejohnson]

In D49138#1157483, @dexonsmith wrote:

Is it impossible to also test this locally (in the LLVM repo)? It will be hard to hack on this file if there aren't local tests.

Yeah I can just invoke llvm-lto2 with the resolutions lld gives LTO in this case and confirm that the symbol is kept...will add test shortly

[tejohnson]

Add llvm-lto2 test (confirmed fails without fix)

[dexonsmith]

Thanks! (I'm not comfortable signing off though, since I don't know much about dllimport.)

[pcc]

This is a COFF-specific behaviour so it should be conditioned on the object file format being COFF.

[tejohnson]

Restrict to COFF.

[tejohnson]

ping

[pcc]

LGTM

Sorry, I reviewed this and left one comment but I forgot to send it. Aside from that, this looks good.

lib/LTO/LTO.cpp

528 ↗

(On Diff #154843)

You don't need to pass a triple here, you can get it from RegularLTO.CombinedModule->getTargetTriple().

[tejohnson]

Address comment

llvm/trunk/lib/LTO/LTO.cpp

void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
                                unsigned Partition, bool InSummary) {
   auto *ResI = Res.begin();
   auto *ResE = Res.end();
   (void)ResE;
   for (const InputFile::Symbol &Sym : Syms) {
     assert(ResI != ResE);
     SymbolResolution Res = *ResI++;
 
-    auto &GlobalRes = GlobalResolutions[Sym.getName()];
+    StringRef Name = Sym.getName();
+    Triple TT(RegularLTO.CombinedModule->getTargetTriple());
+    // 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];
     GlobalRes.UnnamedAddr &= Sym.isUnnamedAddr();
     if (Res.Prevailing) {
       assert(!GlobalRes.Prevailing &&
              "Multiple prevailing defs are not allowed");
       GlobalRes.Prevailing = true;
       GlobalRes.IRName = Sym.getIRName();
     } else if (!GlobalRes.Prevailing && GlobalRes.IRName.empty()) {
       // Sometimes it can be two copies of symbol in a module and prevailing

llvm/trunk/test/LTO/X86/Inputs/dllimport.ll

+; ModuleID = 'b.obj'
+source_filename = "b.cpp"
+target datalayout = "e-m:w-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

llvm/trunk/test/LTO/X86/dllimport.ll

+; 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-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