[ORC][COFF] Handle COFF import files of static archive.

[sunho]

Summary

Handles COFF import files of static archive. Changes static library genrator to build up object file map keyed by symbol name that excludes the symbols from dllimported symbols so that static generator will not be responsible for them. It exposes the list of dynamic libraries that need to be imported. Client should properly load the libraries in this list beforehand. Object file map is also an improvment from the past in terms of performance. Archive.findSym does a slow O(n) linear serach of symbol list to find the symbol. (we call findSym O(n) times, thus full time complexity is O(n^2); we were the only user of findSym function in fact)

There is a room for improvements in how to load the libraries in the list. We currently just hand the responsibility over to the clinet. A better way would be let ORC read this list and hand them over to JITLink side that would also help validation (e.g. not trying to generate stub for non dllimported targets) Nevertheless, we will have to exclude the symbols from COFF import object file list and need a way to access this list, which this patch offers.

[sunho]

Use symbolstringptr.

[lhames]

Does obj2yaml work for archives? If so it'd be cool to have a testcase for this. If obj2yaml doesn't support this I'm not sure we could count on having available tools that would produce an archive with COFF imports in it, could we?

llvm/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h

290–295 ↗

(On Diff #445799)

Eventually I think we'll want this to be a separate structure returned by the Create, rather than a method on StaticLibraryDefinitionGenerator, but it's early days for this API -- I don't mind if it lands like this initially.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

1665–1667 ↗

(On Diff #445799)

This is always going to load the library as a real dylib (via EPCDynamicLibrarySearchGenerator), right?

Could we have it treated as if it were a -L argument instead?

[sunho]

obj2yaml tries to dump the archive files but support for lib file is broken. I think we can use other tools to genrate test cases though. (we could just use raw lib file if we can't get anything working)

[sunho]

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

1665–1667 ↗

(On Diff #445799)

We want to search the dll file that is not inside specified -L serach directories as well. One way is extend existing -l flag to also look up system dynamic library if everything else fails in search directorires and print warning message. How does that sound to you?

[lhames]

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

1665–1667 ↗

(On Diff #445799)

How is MSVC / LINK.EXE handling this? Do they just have a fallback list of search paths to search on? I'd be fine with including those as if they were -L arguments by default (maybe with a switch to turn them off explicitly where they're not wanted)

[sunho]

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

1665–1667 ↗

(On Diff #445799)

It would just rely on windows kernel to do the dll search which is described in https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order. However, common use cases of llvm-jitlink wouldn't really want to look pathes other than MSVC installations and probably some windows system path. So, I think it's fine to go with adding some default path to -L way.

[sunho]

New approach in llvm-jitlink

[sunho]

Optimize code a bit. I don't think this part of code needs optimization but I couldn't help it.

[lhames]

See individual comments, but otherwise LGTM!

llvm/lib/ExecutionEngine/Orc/ExecutionUtils.cpp

426 ↗

(On Diff #447786)

Following the usual error handling idioms you should add an ErrorAsOutParameter _(&Err); variable to the start of this function body -- this will clear the checked flag on exit from the constructor, forcing the caller to check it. (The original version did not need this, as Err was never modified in the body.)

In this case it's not necessary at the moment because the constructor is private and the only caller returns the Error (which will also clear the checked flag), but it would become important if we ever made the constructor public.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

97–98 ↗

(On Diff #447786)

"dynlib" isn't used anywhere else -- I think this should either be "dylib", or just plain "search-system-lib-paths".

1272–1275 ↗

(On Diff #447786)

Could you add an TODO to move this logic into ObjectFile::getTriple().

1536–1545 ↗

(On Diff #447786)

We probably want to add API to the ORC runtime to retrieve the remote environment, rather than grabbing the local one. Could you add a TODO for this too?

1595 ↗

(On Diff #447786)

What's the rationale behind the switch to a priority queue? Unless there's a good reason for the switch this should stay as a std::vector, just to minimize the differences introduced in this patch.

1681–1682 ↗

(On Diff #447786)

This should generate an llvm::Error rather than assert -- we can't assume that object contents are well-formed.

1803–1808 ↗

(On Diff #447786)

Rather than hoisting the loop into a lambda, you could use LLVM's handy concat-iterator utility to iterate over both sequences in one go:

for (StringRef SearchPath : concat<StringRef>(JDSearchPaths[&JD], SystemSearchPaths)) {
    ..
}

[sunho]

llvm/tools/llvm-jitlink/llvm-jitlink.cpp

1595 ↗

(On Diff #447786)

We still need another type of data structure to "push_front" We could reverse the order of sorting -- but that makes it more confusing in my opinion. I changed it to deque to keep it similar in terms of code and performance while enabling to push load job in the middle.

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

Expected<Symbol *> COFFLinkGraphBuilder::createCOMDATExportRequest(
   case COFF::IMAGE_COMDAT_SELECT_EXACT_MATCH:
   case COFF::IMAGE_COMDAT_SELECT_SAME_SIZE: {
     // FIXME: Implement size/content validation when LinkGraph is able to
     // handle this.
     L = Linkage::Weak;
     break;
   }
   case COFF::IMAGE_COMDAT_SELECT_LARGEST: {
-    // FIXME: Support IMAGE_COMDAT_SELECT_LARGEST when LinkGraph is able to
-    // handle this.
-    return make_error<JITLinkError>(
-        "IMAGE_COMDAT_SELECT_LARGEST is not supported.");
+    // FIXME: Support IMAGE_COMDAT_SELECT_LARGEST properly when LinkGraph is
+    // able to handle this.
+    LLVM_DEBUG({
+      dbgs() << "    " << SymIndex
+             << ": Partially supported IMAGE_COMDAT_SELECT_LARGEST was used"
+                " in section "
+             << Symbol.getSectionNumber() << "\n";
+    });
+    L = Linkage::Weak;
+    break;
   }
   case COFF::IMAGE_COMDAT_SELECT_NEWEST: {
     // Even link.exe doesn't support this selection properly.
     return make_error<JITLinkError>(
         "IMAGE_COMDAT_SELECT_NEWEST is not supported.");
   }
   default: {
     return make_error<JITLinkError>("Invalid comdat selection type: " +
LLVM_DEBUG({
     dbgs() << "      " << *Target << "\n";
   });
   PendingComdatExport = None;
   return Target;
 }
 
 } // namespace jitlink
 } // namespace llvm
 No newline at end of file

llvm/test/ExecutionEngine/JITLink/X86/COFF_comdat_largest.test

 # REQUIRES: asserts
-# XFAIL: *
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-jitlink -noexec --debug-only=jitlink -noexec %t 2>&1
 # 
-# Check jitlink return an error when IMAGE_COMDAT_SELECT_LARGEST selection type is encountered.
+# Check jitlink handles largest selection type as plain weak symbol.
 #
+# CHECK: Creating graph symbols...
+# CHECK:      2: Creating defined graph symbol for COFF symbol ".text" in .text (index: 2)
+# CHECK-NEXT:   0x0 (block + 0x00000000): size: 0x00000001, linkage: strong, scope: local, dead  -   <anonymous symbol>
+# CHECK-NEXT: 4: Exporting COMDAT graph symbol for COFF symbol "func" in section 2
+# CHECK-NEXT:   0x0 (block + 0x00000000): size: 0x00000001, linkage: weak, scope: default, dead  -   func
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: [  ]
 sections:
   - Name:            .text
     Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]