[PATCH] D41269: [COFF] Warn for locally imported symbols

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I think I'm fine with this patch.



================
Comment at: COFF/SymbolTable.cpp:102
         LocalImportChunks.push_back(cast<DefinedLocalImport>(Sym)->getChunk());
+        UndefsToLocalImports.try_emplace(Sym, D);
         continue;
----------------
smeenai wrote:
> ruiu wrote:
> > smeenai wrote:
> > > ruiu wrote:
> > > > Can you just call warn() here?
> > > 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.
> > 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.
> 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.
Ah, my bad. They are different symbols indeed.


================
Comment at: COFF/SymbolTable.cpp:131
         errorOrWarn(toString(File) + ": undefined symbol: " + Sym->getName());
+      if (Symbol *LI = UndefsToLocalImports.lookup(Sym))
+        warn(toString(File) + ": locally defined symbol imported: " +
----------------
I'd name this Imp instead of LI (which is an unfamiliar name in lld.)


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41269