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

[Tom Stellard via Phabricator via llvm-commits]

tstellar marked an inline comment as done.
tstellar added inline comments.


================
Comment at: llvm/tools/llvm-shlib/libllvm.cpp:21
+
+  snprintf(SharedObjectName, 100, "LLVM%sBackend.so", Target);
+  Handle = dlopen(SharedObjectName, RTLD_LAZY | RTLD_NOLOAD);
----------------
stellaraccident wrote:
> 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.
Maybe we should add a test to check for a long target name?  Otherwise I'm not sure exactly how to improve this.


================
Comment at: llvm/tools/llvm-shlib/libllvm.cpp:40
+    if (!InitFunc) { \
+      return; \
+    } \
----------------
serge-sans-paille wrote:
> Not all targets have all these features, so I guess it's fine to ignore the errors when loading fails (?) But some are mandatory, maybe split the mandatory and non-mandatory components?
So do you think we should print an error message if a mandatory feature fails to load but do nothing if a non-mandatory feature fails?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93351