[PATCH] D129952: [ORC][COFF] Handle COFF import files of static archive.

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.

See individual comments, but otherwise LGTM!



================
Comment at: llvm/lib/ExecutionEngine/Orc/ExecutionUtils.cpp:426
       Archive(std::make_unique<object::Archive>(*this->ArchiveBuffer, Err)) {
 
   if (!this->GetObjFileInterface)
----------------
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.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:97-98
 
+static cl::opt<bool> SearchSystemDynLibrary(
+    "search-sys-dynlib",
+    cl::desc("Add system library paths to dynamic library search paths"),
----------------
"dynlib" isn't used anywhere else -- I think this should either be "dylib", or just plain "search-system-lib-paths".


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1272-1275
+        if (Magic == file_magic::coff_object) {
+          TT.setObjectFormat(Triple::COFF);
           TT.setOS(Triple::OSType::Win32);
+        }
----------------
Could you add an `TODO` to move this logic into `ObjectFile::getTriple()`.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1536-1545
+static SmallVector<StringRef, 5> getSearchPathsFromEnvVar(Session &S) {
+  SmallVector<StringRef, 5> PathVec;
+  auto TT = S.ES.getExecutorProcessControl().getTargetTriple();
+  if (TT.isOSBinFormatCOFF())
+    StringRef(getenv("PATH")).split(PathVec, ";");
+  else if (TT.isOSBinFormatELF())
+    StringRef(getenv("LD_LIBRARY_PATH")).split(PathVec, ":");
----------------
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?


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1595
+  };
+  llvm::PriorityQueue<LibraryLoad, SmallVector<LibraryLoad, 2>, decltype(Comp)>
+      LibraryLoadQueue(Comp);
----------------
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.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1681-1682
+      auto FileNameRef = StringRef(FileName);
+      assert(FileNameRef.endswith_insensitive(".dll") &&
+             "COFF Imported library not ending with dll extension?");
+      NewLL.LibName = FileNameRef.drop_back(strlen(".dll")).str();
----------------
This should generate an `llvm::Error` rather than assert -- we can't assume that object contents are well-formed.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1803-1808
+    if (auto Err = TraverseSearchPathList(JDSearchPaths[&JD]))
+      return Err;
+    if (LibFound)
+      continue;
+    if (auto Err = TraverseSearchPathList(SystemSearchPaths))
+      return Err;
----------------
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)) {
        ..
    }
    
```


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

https://reviews.llvm.org/D129952