[PATCH] D93351: [llvm-shlib] Build backend libraries as loadable modules

[Stella Laurenzo via Phabricator via llvm-commits]

stellaraccident added inline comments.


================
Comment at: llvm/tools/llvm-shlib/CMakeLists.txt:34
+      list(REMOVE_ITEM LIB_NAMES "LLVM${component}")
+      list(APPEND objects "$<TARGET_OBJECTS:LLVM${component}>")
+    endforeach()
----------------
I am somewhat surprised this works -- I usually try to avoid this kind of layering issue, because while it can work, it usually has weird edge cases.

In this case, each of your target backends will directly embed (and export!) its dependencies, which, are probably also embedded and exported by the libLLVM that loads it. Aside from bloat, this can cause issues with module statics being duplicated depending on how the child library references things in the module (tends to manifest with cl conflicts in the support library at runtime). Based on the sizes you report, I suspect this is adding ~a megabyte to each of your backends (completely guessing based on what I have seen).

There are two ways to later this better:
- arrange for the dependencies to always exist in libLLVM and then just don't include them here. Because you are dlopen'ing from there, this should work on Linux, even without an ld-level link. Might not work on osx. Can't work on Windows.
- my rework of the cmake bits for libLLVM layers this properly but will take some time/consensus. I feel like that is the better approach, but imo, of this works for the cases your targeting and stems some issues, I'm not opposed.


================
Comment at: llvm/tools/llvm-shlib/CMakeLists.txt:36
+    endforeach()
+    add_llvm_library("LLVM${target}Backend"
+                     MODULE
----------------
Is there a reason why we are using the "backend" nomenclature here for each target? I had looked into doing this with my cmake upgrades and had been thinking that a name like LLVMX86Target was more consistent.


================
Comment at: llvm/tools/llvm-shlib/libllvm.cpp:21
+
+  snprintf(SharedObjectName, 100, "LLVM%sBackend.so", Target);
+  Handle = dlopen(SharedObjectName, RTLD_LAZY | RTLD_NOLOAD);
----------------
serge-sans-paille wrote:
> serge-sans-paille wrote:
> > It's probably safer to just use an ``llvm::raw_string_ostream`` here, as your code breaks if a Target has a very long name.
> Or `llvm::format` btw
I don't think you want to be depending on anything from support in this kind of shim loader, so I would stick with libc string manipulation, but +1 on making it safer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93351