[PATCH] D130479: [ORC_RT][COFF] Initial platform support for COFF/x86_64.

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Thanks for your patience with the review @sunho!

This is great stuff -- I'm very excited to see these features coming to Windows.



================
Comment at: compiler-rt/lib/orc/CMakeLists.txt:3-20
+# ORC runtime library common implementation files.
+set(ORC_COMMON_SOURCES
   debug.cpp
   extensible_rtti.cpp
   log_error_to_stderr.cpp
+  run_program_wrapper.cpp
+  dlfcn_wrapper.cpp
----------------
What's the distinction between `ORC_COMMON_SOURCES` sources and `ORC_ALL_SOURCES`?

looks like `dlfcn_wrapper.cpp` is included in both? Was that intentional?

It looks like the other edits to `CMakeLists.txt` include cleanup that's tangential to COFF support. How hard would it be to break out this cleanup from the rest of the patch? (Definitely nice to see this being cleaned up either way!)


================
Comment at: compiler-rt/lib/orc/adt.h:90-127
+    return LHS._compare(RHS, std::min(LHS.Size, RHS.Size)) == 0;
   }
 
   friend bool operator!=(const string_view &LHS, const string_view &RHS) {
     return !(LHS == RHS);
   }
 
----------------
These are nice extensions, but the switch to c++17 in https://github.com/llvm/llvm-project/commit/b1356504e63ae821cccf1e051a0d2526bdfef2b0 means that we can now drop `__orc_rt::string_view` entirely in favor of `std::string_view`. I will land a patch for that tomorrow.


================
Comment at: compiler-rt/lib/orc/coff_platform.cpp:58
+  // name.
+  struct XtorSection {
+    void Register(char SubsectionChar, span<void (*)(void)> Xtors) {
----------------
Where did the `Xtor` prefix come from? I really like that. Much cleaner than `CtorDtor`.


================
Comment at: compiler-rt/lib/orc/coff_platform.cpp:536
+                                                   ExecutorAddrRange Range) {
+  assert(!this->BlockRanges.count(Range.Start.toPtr<void *>()) &&
+         "Block range address already registered");
----------------
Is the explicit `this->` necessary here?


================
Comment at: compiler-rt/lib/orc/coff_platform.h:20-24
+ORC_RT_INTERFACE const char *__orc_rt_coff_jit_dlerror();
+ORC_RT_INTERFACE void *__orc_rt_coff_jit_dlopen(const char *path, int mode);
+ORC_RT_INTERFACE int __orc_rt_coff_jit_dlclose(void *header);
+ORC_RT_INTERFACE void *__orc_rt_coff_jit_dlsym(void *header,
+                                               const char *symbol);
----------------
Does Windows support the POSIX `dl*` APIs?

I *think* these should probably be renamed to match the corresponding Windows function names, e.g. `__orc_rt_LoadLibrary`, `__orc_rt_GetProcAddress` and `__orc_rt_FreeLibrary`. The high-level `__orc_rt_coff_run_program` function can then be written in terms of them very similarly to the way that `__orc_rt_macho_run_program` uses the dlfcn APIs.

The idea here is that we could alias `LoadLibrary -> __orc_rt_LoadLibrary` for JIT'd code, and then windows code that calls LoadLibrary will work under the JIT*.

All that said -- I'm fine with this landing as-is, and these comments being addressed in follow-up patches.

*We have a long way to go before this is true in general, but we want to move in that direction.



================
Comment at: compiler-rt/lib/orc/error.h:116
 
-  void setChecked(bool Checked) {
-    ErrPtr = (reinterpret_cast<uintptr_t>(ErrPtr) & ~uintptr_t(1)) | Checked;
-  }
+  void setChecked(bool Checked) { ErrPtr = (ErrPtr & ~uintptr_t(1)) | Checked; }
 
----------------
Nice cleanup! It seems unrelated to the rest of this patch -- can you commit it separately? (No further review needed)


================
Comment at: compiler-rt/test/orc/TestCases/Windows/x86-64/lit.local.cfg.py:1-4
+if config.root.host_arch not in ['AMD64','x86_64']:
+  config.unsupported = True
+
+if config.target_arch not in ['AMD64','x86_64']:
----------------
Can this just be `config.target_arch != "x86_64"`? Do we not canonicalize AMD64 -> x86_64?


================
Comment at: compiler-rt/test/orc/lit.cfg.py:32-34
+if config.host_os == 'Windows':
+  config.substitutions.append(
+      ('%llvm_jitlink', (llvm_jitlink + ' -orc-runtime=' + orc_rt_path + ' -no-process-syms=true -slab-allocate=64MB')))
----------------
Should be formatted for 80-columns.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/COFFPlatform.h:189
+  std::map<JITDylib *, JDBootstrapState> JDBootstrapStates;
+  std::atomic<bool> Bootstraping;
+
----------------
Typo: Bootstraping -> Bootstrapping.


================
Comment at: llvm/lib/ExecutionEngine/Orc/COFFPlatform.cpp:217-218
+
+  auto IsBootstraping = Bootstraping.load();
+  if (!IsBootstraping) {
+    std::vector<std::string> ImportedLibs;
----------------
`IsBootstrapping` is unused outside the condition -- could you just change the condition to `(!Bootstrapping)`?


================
Comment at: llvm/lib/ExecutionEngine/Orc/COFFPlatform.cpp:386-425
+Expected<COFFPlatform::JITDylibDepMap>
+COFFPlatform::buildJDDepMap(JITDylib &JD) {
+  return ES.runSessionLocked([&]() -> Expected<JITDylibDepMap> {
+    JITDylibDepMap JDDepMap;
+
+    SmallVector<JITDylib *, 16> Worklist({&JD});
+    while (!Worklist.empty()) {
----------------
This is the same algorithm as MachO, right? We should look for a way to factor this out, but that can happen in a followup patch.


================
Comment at: llvm/lib/ExecutionEngine/Orc/ObjectFileInterface.cpp:220-225
+    if (auto SecName = Sec.getName()) {
+      if (COFFPlatform::isInitializerSection(*SecName)) {
+        addInitSymbol(I, ES, Obj.getFileName());
+        break;
+      }
+    }
----------------
This is missing a return on the error path:
```
} else
  return SecName.takeError();
```


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1069-1073
+  else
+    ExitOnErr(MainJD->define(absoluteSymbols(
+        {{ES.intern("llvm_jitlink_setTestResultOverride"),
+          {pointerToJITTargetAddress(llvm_jitlink_setTestResultOverride),
+           JITSymbolFlags::Exported}}})));
----------------
Maybe add a brief comment here that we always want JIT'd code to be able to call this, even when we don't expose other processes symbols?


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1222-1223
 
+Expected<JITDylib *> Session::getOrLoadDynamicLibrary(StringRef LibName,
+                                                      StringRef LibPath) {
+  auto It = DynLibJDs.find(LibName.str());
----------------
Is there any reason to make a distinction between `LibName` and `LibPath` here? Could we just use the path as the name?


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.h:56
 
+  using DynLibJDMap = std::map<std::string, orc::JITDylib *>;
+  using KnownDynLibPathMap = std::map<std::string, std::string>;
----------------
Is this just a map of JITDylib names to JITDylibs? If so I think you could use `ExecutionSession::getJITDylibByName` instead.


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

https://reviews.llvm.org/D130479