[COFF] Warn for locally imported symbols

[smeenai]

Summary

Locally imported symbols are a very surprising linker feature. link.exe
warns for them, and we should warn too.

[ruiu]

COFF/SymbolTable.cpp

102

Can you just call warn() here?

test/COFF/locally-imported.test

6

I wouldn't repeat the filename twice in the same line.

[smeenai]

COFF/SymbolTable.cpp

102

I definitely want to report the object file which is importing the symbol (link.exe does even better and reports the function that's doing the import, but I believe reporting the object file is the best we can do). As far as I can tell, the only way to figure out the object file(s) that are doing the import is to loop through the symbols in Config->GCRoot and every object file, as is done below (in the loops I've modified). I could duplicate those loops here to inline the warning, but it seemed better to avoid the duplication and reuse the existing loops.

test/COFF/locally-imported.test

6

The file in which the symbol is defined and the file in which it's imported would normally be different.

If you want, I can change the test so that the symbol is defined and imported in different object files (or perhaps add a different test case which does that), to demonstrate the different filenames.

[ruiu]

COFF/SymbolTable.cpp

66

I'd name this LocalImports.

102

I see. But the way you are using the map is not correct. Sym and D are always the same pointer value. You can use a set instead of map.

[smeenai]

COFF/SymbolTable.cpp

102

I thought Sym would be the original (undefined) symbol, and D would be the corresponding defined local symbol. The pointer values are definitely different in my testing, and e.g. Sym->getFile() is NULL even after the replaceSymbol call.

[ruiu]

I think I'm fine with this patch.

COFF/SymbolTable.cpp

102

Ah, my bad. They are different symbols indeed.

131

I'd name this Imp instead of LI (which is an unfamiliar name in lld.)

[smeenai]

Rui's comments; add test for multiple input files

[ruiu]

COFF/SymbolTable.cpp

102

Why don't you do LocalImports[Sym] = D?

[smeenai]

Use [] for dictionary insertion

[smeenai]

COFF/SymbolTable.cpp

102

I forgot about that form. Updated.

[ruiu]

LGTM

COFF/SymbolTable.cpp

static void errorOrWarn(const Twine &S) {
   if (Config->Force)
     warn(S);
   else
     error(S);
 }
 
 void SymbolTable::reportRemainingUndefines() {
   SmallPtrSet<Symbol *, 8> Undefs;
+  DenseMap<Symbol *, Symbol *> LocalImports;
 
   for (auto &I : SymMap) {
     Symbol *Sym = I.second;
     auto *Undef = dyn_cast<Undefined>(Sym);
     if (!Undef)
       continue;
     if (!Sym->IsUsedInRegularObj)
       continue;
for (auto &I : SymMap) {
     // If we can resolve a symbol by removing __imp_ prefix, do that.
     // This odd rule is for compatibility with MSVC linker.
     if (Name.startswith("__imp_")) {
       Symbol *Imp = find(Name.substr(strlen("__imp_")));
       if (Imp && isa<Defined>(Imp)) {
         auto *D = cast<Defined>(Imp);
         replaceSymbol<DefinedLocalImport>(Sym, Name, D);
         LocalImportChunks.push_back(cast<DefinedLocalImport>(Sym)->getChunk());
+        LocalImports[Sym] = D;
         continue;
       }
     }
 
     // Remaining undefined symbols are not fatal if /force is specified.
     // They are replaced with dummy defined symbols.
     if (Config->Force)
       replaceSymbol<DefinedAbsolute>(Sym, Name, 0);
     Undefs.insert(Sym);
   }
 
-  if (Undefs.empty())
+  if (Undefs.empty() && LocalImports.empty())
     return;
 
-  for (Symbol *B : Config->GCRoot)
+  for (Symbol *B : Config->GCRoot) {
     if (Undefs.count(B))
       errorOrWarn("<root>: undefined symbol: " + B->getName());
+    if (Symbol *Imp = LocalImports.lookup(B))
+      warn("<root>: locally defined symbol imported: " + Imp->getName() +
+           " (defined in " + toString(Imp->getFile()) + ")");
+  }
 
-  for (ObjFile *File : ObjFile::Instances)
-    for (Symbol *Sym : File->getSymbols())
-      if (Sym && Undefs.count(Sym))
+  for (ObjFile *File : ObjFile::Instances) {
+    for (Symbol *Sym : File->getSymbols()) {
+      if (!Sym)
+        continue;
+      if (Undefs.count(Sym))
         errorOrWarn(toString(File) + ": undefined symbol: " + Sym->getName());
+      if (Symbol *Imp = LocalImports.lookup(Sym))
+        warn(toString(File) + ": locally defined symbol imported: " +
+             Imp->getName() + " (defined in " + toString(Imp->getFile()) + ")");
+    }
+  }
 }
 
 std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name) {
   Symbol *&Sym = SymMap[CachedHashStringRef(Name)];
   if (Sym)
     return {Sym, false};
   Sym = (Symbol *)make<SymbolUnion>();
   Sym->IsUsedInRegularObj = false;

test/COFF/Inputs/locally-imported-def.s

+.text
+.globl f
+f:
+  ret

test/COFF/Inputs/locally-imported-imp.s

+.text
+  call  __imp_f

test/COFF/locally-imported-warn-multiple.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc -o %T/locally-imported-def.obj %S/Inputs/locally-imported-def.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc -o %T/locally-imported-imp1.obj %S/Inputs/locally-imported-imp.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc -o %T/locally-imported-imp2.obj %S/Inputs/locally-imported-imp.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc -o %t.obj %s
+# RUN: lld-link /entry:main %T/locally-imported-def.obj %T/locally-imported-imp1.obj %T/locally-imported-imp2.obj %t.obj 2>&1 | FileCheck %s
+
+# CHECK: warning: [[TESTDIR:[^:]+]]locally-imported-imp1.obj: locally defined symbol imported: f (defined in [[TESTDIR]]locally-imported-def.obj)
+# CHECK-NEXT: warning: [[TESTDIR:[^:]+]]locally-imported-imp2.obj: locally defined symbol imported: f (defined in [[TESTDIR]]locally-imported-def.obj)
+
+.globl main
+main:
+  ret

test/COFF/locally-imported.test

 # RUN: yaml2obj < %s > %t.obj
-# RUN: lld-link /out:%t.exe /entry:main %t.obj
+# RUN: lld-link /out:%t.exe /entry:main %t.obj 2>&1 | FileCheck -check-prefix=WARN %s
 # RUN: llvm-objdump -s %t.exe | FileCheck %s
 # RUN: llvm-readobj -coff-basereloc %t.exe | FileCheck -check-prefix=BASEREL %s
 
+# WARN: warning: [[INPUT:[^:]+]]: locally defined symbol imported: main (defined in [[INPUT]])
+
 # CHECK:      Contents of section .text:
 # CHECK-NEXT: 1000 00200000
 # CHECK:      Contents of section .rdata:
 # CHECK-NEXT: 2000 04100040 01000000
 
 # BASEREL:      BaseReloc [
 # BASEREL-NEXT:   Entry {
 # BASEREL-NEXT:     Type: DIR64