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

[Sunho Kim via Phabricator via llvm-commits]

sunho added inline comments.


================
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;
----------------
lhames wrote:
> 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?
I was getting "Illegal memory protection flag specified" error in getWindowsProtectionFlags. I thought it was None flag just supplied as it is but it might be with some interesting combination like W + X. I'll check again.


================
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))
----------------
lhames wrote:
> This might be worth lifting out into its own method.
I reallized the same when I was writing SECREL patch and splitted off into additional method in that patch. 


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

https://reviews.llvm.org/D130175