[PATCH] D130175: [JITLink][COFF] Implement dllimport stubs.

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

Otherwise LGTM.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:147-153
+    MemProt Prot = MemProt::Read;
     if ((*Sec)->Characteristics & COFF::IMAGE_SCN_MEM_EXECUTE)
       Prot |= MemProt::Exec;
     if ((*Sec)->Characteristics & COFF::IMAGE_SCN_MEM_READ)
       Prot |= MemProt::Read;
     if ((*Sec)->Characteristics & COFF::IMAGE_SCN_MEM_WRITE)
       Prot |= MemProt::Write;
----------------
What prompted this change?

I assume `IMAGE_SCN_MEM_READ` is specified on most sections, or you'd have seen this blow up already. Should we turn `Read` on by default for all sections, or only for some subset?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:227
     else if (Sym->isUndefined()) {
-      LLVM_DEBUG({
-        dbgs() << "    " << SymIndex
-               << ": Creating external graph symbol for COFF symbol \""
-               << SymbolName << "\" in "
-               << getCOFFSectionName(SectionIndex, Sec, *Sym)
-               << " (index: " << SectionIndex << ") \n";
-      });
-      if (!ExternalSymbols.count(SymbolName))
-        ExternalSymbols[SymbolName] =
-            &G->addExternalSymbol(SymbolName, Sym->getValue(), Linkage::Strong);
-      GSym = ExternalSymbols[SymbolName];
+      auto CreateExternalSymbol = [&](StringRef SymbolName) {
+        if (!ExternalSymbols.count(SymbolName))
----------------
This might be worth lifting out into its own method.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130175/new/

https://reviews.llvm.org/D130175