LTO builds fail when dllimport functions are called locally

[llvmbot]

Bugzilla Link	38105
Resolution	FIXED
Resolved on	Jul 23, 2018 15:35
Version	unspecified
OS	Windows NT
Reporter	LLVM Bugzilla Contributor
CC	@pcc,@rnk

Extended Description

When a function is declared dllimport but called directly (i.e. in the same binary), a non-LTO build will issue a warning but still succeed (same as MSVC). LTO and ThinLTO builds fail with linkage complaints.

This is preventing Firefox's use of LTO on Windows: https://bugzilla.mozilla.org/show_bug.cgi?id=1448976

clang version 7.0.0 (trunk 336407)

$ cat a.cpp
__declspec(dllimport) int foo();
int main() { return foo(); }

$ cat b.cpp
int foo() { return 42; }

$ cat test.sh
#!/bin/bash
rm -rf *.obj *.exe
clang-cl $FLAG -O2 -c a.cpp b.cpp
lld-link -nodefaultlib -entry:main a.obj b.obj

$ ./test.sh
lld-link.exe: warning: a.obj: locally defined symbol imported: ?foo@@yahxz (defined in b.obj) [LNK4217]

$ FLAG=-flto ./test.sh
Global is external, but doesn't have external or weak linkage!
i32 ()* @"?foo@@yahxz"
LLVM ERROR: Broken module found, compilation aborted!

$ FLAG=-flto=thin ./test.sh
lld-link.exe: error: undefined symbol: ?foo@@yahxz

referenced by lto.tmp:(main)

[llvmbot]

Hopefully Peter who I cc'ed on this can take a look. It is the index-based computeDeadSymbols that is causing the issue for both regular and thin LTO. Disabling it via /mllvm:-compute-dead=false makes the issue go away.

The problem is that LTO::run is getting 2 GlobalResolutions for foo, one prevailing from b.obj and one non-prevailing from a.obj. It processes the prevailing first and this line sets the GUID to prevailing:

GUIDPrevailingResolutions[GUID] =
    Res.second.Prevailing ? PrevailingType::Yes : PrevailingType::No;

When the non-prevailing a.obj copy is processed, it is overwritten with No.

Peter - what should be happening here? Should we never reset to No after setting to Yes?

[pcc]

The issue is that there are two separate symbols involved, which leads to there being two entries in GlobalResolutions for the same IR symbol:

$ ~/src2/llvm-project/ra/bin/llvm-nm a.obj
U _imp?foo@@yahxz
---------------- T main
$ ~/src2/llvm-project/ra/bin/llvm-nm b.obj
---------------- T ?foo@@yahxz

The same root cause was probably also causing this code: http://llvm-cs.pcc.me.uk/lib/LTO/LTO.cpp#858 to be run twice for the same symbol in the -flto case, causing an incorrect linkage to be set on the dllimport symbol (although I couldn't reproduce this myself).

I'm not sure what the right fix is yet.

[llvmbot]

lld has special handling to disregard the _imp prefix if necessary: llvm-mirror/lld@add5dd0#diff-e248c0b838e4ca91656fc436f8f5e9ac

That even allows for a case of calling an _imp function directly without dllimport keywords. (yuck! but compat, I guess?)

Should the LTO code do something similar?

[llvmbot]

Indeed, the following patch fixes the issue:

diff --git a/lib/LTO/LTO.cpp b/lib/LTO/LTO.cpp
index 6077b3b1ba4..05c75b77b31 100644
--- a/lib/LTO/LTO.cpp
+++ b/lib/LTO/LTO.cpp
@@ -428,7 +428,10 @@ void LTO::addModuleToGlobalRes(ArrayRefInputFile::Symbol Syms,
assert(ResI != ResE);
SymbolResolution Res = *ResI++;

• auto &GlobalRes = GlobalResolutions[Sym.getName()];

• StringRef Name = Sym.getName();
• if (Name.startswith("_imp"))

•  Name = Name.substr(strlen("__imp_"));
  

• auto &GlobalRes = GlobalResolutions[Name];
  GlobalRes.UnnamedAddr &= Sym.isUnnamedAddr();
  if (Res.Prevailing) {
  assert(!GlobalRes.Prevailing &&

I can send a patch with test later today or tomorrow.

[llvmbot]

Fix mailed in D49138, plus lld test in D49140.

[llvmbot]

Thank you Teresa! This patch fixes all but one of the undefined symbols in the Firefox LTO build.

I'll try to reduce the one remaining failure (may take some time) and file a new bug if it turns out to be unrelated.

[llvmbot]

Fixed in r337762.