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

[Sunho Kim via Phabricator via llvm-commits]

sunho marked 8 inline comments as done.
sunho added inline comments.


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


================
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);
----------------
lhames wrote:
> 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.
> 
The plan of me is that still keep internal implementation dl* api friendly and implement loadlibrary using dl* api. This allows us to reuse LLJIT platform support class which uses dl* api as it is. Notice that we don't alias dlopen -> __orc_rt_dl_... yet.


================
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']:
----------------
lhames wrote:
> Can this just be `config.target_arch != "x86_64"`? Do we not canonicalize AMD64 -> x86_64?
Not exactly sure what's going on here. On my local machine, only AMD64 works.


================
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()) {
----------------
lhames wrote:
> 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.
I thought the same while doing this. What's more urgent in my opinion is refactor the orc runtime side code to share more code. A lot of code is duplicated across multiple platforms there. 


================
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());
----------------
lhames wrote:
> Is there any reason to make a distinction between `LibName` and `LibPath` here? Could we just use the path as the name?
It's separated because same DLL can exist in multiple include pathes. All the path stuffs is done to make sure we don't double import the same DLL.


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

https://reviews.llvm.org/D130479