[clang-repl] Add a command to load dynamic libraries

[argentite]

Summary

This commit adds the %lib <file> command to load a dynamic library to be
used by the currently running interpreted code.
For example %lib libSDL2.so.

[sgraenitz]

Thanks for working on this! It looks like this is going to be the clang-repl equivalent for Cling's .L command. Do you think we can get test coverage for it?

So far, the only tests I found for libclangInterpreter are unit tests in https://github.com/llvm/llvm-project/tree/main/clang/unittests/Interpreter -- e.g. there is a test for the %undo command. I guess the %lib command would be a good candidate for a LIT test, because it requires a dynamic library to load from disk. The tests for .L in Cling may give you a hint on how this could look like: https://github.com/root-project/cling/tree/master/test/LibraryCall

clang/include/clang/Interpreter/Interpreter.h

62–63

Do we really want these functions capitalized? IIRC in the clang code-base they are used to highlight core API functions. It looks like these two functions are only used as helpers inside Interpreter itself.

78

Typo dynamic

clang/lib/Interpreter/Interpreter.cpp

221

Factoring out this function looks like an independent change. Is it related to the load-library command in any way?

clang/tools/clang-repl/ClangRepl.cpp

126

I see this is analog to existing code, but is there a good reason for using the multi-line string syntax here?

[argentite]

clang/include/clang/Interpreter/Interpreter.h

62–63

I guess we can keep them without capitalizing.

clang/lib/Interpreter/Interpreter.cpp

221

The IncrementalExecutor was being created on the first call to Execute(). It is created lazily for -fsyntax-only support. It holds the Execution Engine that we need for loading the library. So we need to be able to create it in getExecutionEngine() as well because a library can be loaded before any code is executed.

clang/tools/clang-repl/ClangRepl.cpp

126

Yeah I am confused as well. But I tried to maintain the style.

[sgraenitz]

clang/lib/Interpreter/Interpreter.cpp

221

Of course, right. I had missed the second use.

[argentite]

Added a simple test and removed the automatic OS specific library filenames

Full name/path of the library must be provided. This makes the command more
flexible especially when dealing with multiple versions of a library or SDK.

[sgraenitz]

Thanks for the update

clang/test/Interpreter/dynamic-library.cpp

2

The use of head and tail here is a creative solution, but I wonder how well it scales for similar cases in the future in case this becomes a role model. We have this situation a lot in LLDB: compile an input file and use the output in a RUN line. It typically uses separate input files for such purposes (note that "Inputs" folders must be excluded from test discovery somewhere in lit config), e.g.:
https://github.com/llvm/llvm-project/blob/release/16.x/lldb/test/Shell/Breakpoint/jit-loader_jitlink_elf.test

What do you think?

10

This requirement applies to the entire file right? The arrangement here may imply the opposite.

[v.g.vassilev]

Thanks for working on this, looks like we are heading in a good direction!

clang/test/Interpreter/dynamic-library.cpp

2

I agree with that. I've seen that some clang tests surround the code in question with #ifdef LIB and then in the invocation they add -DLIB.

clang/tools/clang-repl/ClangRepl.cpp

126

Independently, I think these commands should be available via pragma directives as well. Maybe in a separate revision we should create something like pragma clang-repl/clang-interpreter load library name. That would allow people to use that in their "scripts" which can be quite useful.

[argentite]

Changed test to use a separate Input file for the dynamic library generation

clang/test/Interpreter/dynamic-library.cpp

2

The #ifdef approach does not seem to work with clang-repl yet. It does not like unterminated #ifdef. So I am going with separate input file. "Inputs" folder seems to be excluded globally in clang/test/lit.cfg.py.

10

Moved it to the top of the file.

[sgraenitz]

Thanks! LGTM

[v.g.vassilev]

LGTM!

[dyung]

Hi @argentite, the test you added is failing on the PS4 linux builder because the test seems to require linking and the PS4 target requires an external linker which is not present on the system. Can the test be rewritten to not require linking?

https://lab.llvm.org/buildbot/#/builders/139/builds/38153

clang: error: unable to execute command: Executable "orbis-ld" doesn't exist!
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[argentite]

Hi, @dyung ! It seems the dynamic library generation is failing from the lack of a linker. We need a test library to load at runtime in clang-repl. Do you have any idea how we can create one without a linker? Otherwise we can disable the test on PS4 as it is probably an unlikely use case.

[dyung]

In D141824#4229693, @argentite wrote:

Hi, @dyung ! It seems the dynamic library generation is failing from the lack of a linker. We need a test library to load at runtime in clang-repl. Do you have any idea how we can create one without a linker? Otherwise we can disable the test on PS4 as it is probably an unlikely use case.

If you need to generate a library at test time, then the test won't work on the PS4 platform since the external linker won't be present. (Note that it should work on PS5 as that uses LLD for the linker). I think you should probably just mark the test as unsupported on PS4.

[argentite]

Just to confirm, UNSUPPORTED: target=x86_64-scei-ps4 should be enough, right?

[probinson]

In D141824#4229953, @argentite wrote:

Just to confirm, UNSUPPORTED: target=x86_64-scei-ps4 should be enough, right?

UNSUPPORTED: target={{.*-(ps4|ps5)}} please.

[probinson]

In D141824#4231372, @probinson wrote:

In D141824#4229953, @argentite wrote:

Just to confirm, UNSUPPORTED: target=x86_64-scei-ps4 should be enough, right?

UNSUPPORTED: target={{.*-(ps4|ps5)}} please.

Oh wait, @dyung says this should work on PS5. Yes, your original suggestion is good.

[dyung]

In D141824#4231374, @probinson wrote:

In D141824#4231372, @probinson wrote:

In D141824#4229953, @argentite wrote:

Just to confirm, UNSUPPORTED: target=x86_64-scei-ps4 should be enough, right?

UNSUPPORTED: target={{.*-(ps4|ps5)}} please.

Oh wait, @dyung says this should work on PS5. Yes, your original suggestion is good.

Upon actually trying to run the test with a PS5 target I see that it fails in a similar way, so the test should be marked unsupported for both platforms. I'll check in a fix to do that.

[zhuhan0]

Hi @argentite, we build llvm in a custom distro which is missing some C startup code at the default paths, and this fails with errors like:

/abc/bin/ld: cannot find crti.o: No such file or directory
/abc/bin/ld: cannot find crtbeginS.o: No such file or directory
/abc/bin/ld: cannot find -lgcc
/abc/bin/ld: cannot find -lgcc_s
/abc/bin/ld: cannot find -lc
/abc/bin/ld: cannot find -lgcc
/abc/bin/ld: cannot find -lgcc_s
/abc/bin/ld: cannot find crtendS.o: No such file or directory
/abc/bin/ld: cannot find crtn.o: No such file or directory

This seems to be the first clang test that involves actual linking. If the intention is to test loading a dynamic library, could you commit the library in the Inputs directory instead of building it from source?
cc @ayermolo

[sgraenitz]

In D141824#4247891, @zhuhan0 wrote:

This seems to be the first clang test that involves actual linking.

Interesting, I think we all didn't have this on our radars.

If the intention is to test loading a dynamic library, could you commit the library in the Inputs directory instead of building it from source? cc @ayermolo

Is that something other tests do? Otherwise, I am not sure it's a practical solution. We could also consider another requirement like host-supports-linking for the test. What do you think?

[zhuhan0]

In D141824#4250011, @sgraenitz wrote:

In D141824#4247891, @zhuhan0 wrote:

This seems to be the first clang test that involves actual linking.

Interesting, I think we all didn't have this on our radars.

If the intention is to test loading a dynamic library, could you commit the library in the Inputs directory instead of building it from source? cc @ayermolo

Is that something other tests do? Otherwise, I am not sure it's a practical solution.

Yes. You can search for .so files under various /test directories. See this and this as some examples. Admittedly you lose some readability if you replace the cpp source input with a compiled library, but you can simply add the source cpp as comments in the test file. After all if the intention is to test loading a shared library, I think it's better to reduce it to just doing that, so you avoid any flakiness that might come from the compilation.

We could also consider another requirement like host-supports-linking for the test. What do you think?

I don't think this will work. Our platform supports linking. It's just the C startup/finish libraries are at a different path. It might be overly complicated to plumb that down to the test through cmake and llvm-lit.

[zhuhan0]

I've put up https://reviews.llvm.org/D147823.

[bcain]

Can you depend on cstdio in an LLVM toolchain test case like this? clang doesn't install cstdio -- does it? This is a c or c++ library file, so I think it should be out of scope for a clang test, right? AFAICT no other clang test uses this header file.

clang/test/Interpreter/dynamic-library.cpp

6

This test fails for me like below.

FAIL: Clang :: Interpreter/dynamic-library.cpp (1 of 17751)
******************** TEST 'Clang :: Interpreter/dynamic-library.cpp' FAILED ********************
Script:
--
: 'RUN: at line 4';   /local/mnt/workspace/upstream/obj_ubuntu/bin/clang -xc++ -o /local/mnt/workspace/upstream/obj_ubuntu/tools/clang/test/Interpreter/Output/libdynamic-library-test.so -fPIC -shared -DLIBRARY /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/Inputs/dynamic-library-test.cpp
: 'RUN: at line 5';   cat /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp | env LD_LIBRARY_PATH=/local/mnt/workspace/upstream/obj_ubuntu/tools/clang/test/Interpreter/Output:$LD_LIBRARY_PATH /local/mnt/workspace/upstream/obj_ubuntu/bin/clang-repl | /local/mnt/workspace/upstream/obj_ubuntu/bin/FileCheck /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp
--
Exit Code: 1

Command Output (stderr):
--
In file included from <<< inputs >>>:1:
input_line_6:1:10: fatal error: 'cstdio' file not found
#include <cstdio>
         ^~~~~~~~
error: Parsing failed.
input_line_12:1:1: error: use of undeclared identifier 'printf'
printf("Return value: %d\n", calculate_answer());
^
error: Parsing failed.
input_line_15:1:1: error: use of undeclared identifier 'printf'
printf("Variable: %d\n", ultimate_answer);
^
error: Parsing failed.
/local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp:15:11: error: CHECK: expected string not found in input
// CHECK: Return value: 5
          ^
<stdin>:1:1: note: scanning from here
clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
^
<stdin>:1:231: note: possible intended match here
clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> 
                                                                                                                                                                                                                                      ^

Input file: <stdin>
Check file: /local/mnt/workspace/upstream/llvm-project/clang/test/Interpreter/dynamic-library.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            1: clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl> clang-repl>  
check:15'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:15'1                                                                                                                                                                                                                                           ?           possible intended match
>>>>>>

--

********************
********************
Failed Tests (1):
  Clang :: Interpreter/dynamic-library.cpp

[sgraenitz]

clang/test/Interpreter/dynamic-library.cpp

6

Thanks for your note, sounds reasonable. @argentite It should be sufficient to forward declare printfright? While we are here, can we somehow check whether the symbol is non-null before calling it?

[argentite]

We should probably also address the lack of linker issue as well. Should we go for a precompiled dynamic library file? There seems to be some "precedent" of this in other tests.

clang/test/Interpreter/dynamic-library.cpp

6

Yes I think I can do that.

[sgraenitz]

In D141824#4274461, @argentite wrote:

We should probably also address the lack of linker issue as well. Should we go for a precompiled dynamic library file? There seems to be some "precedent" of this in other tests.

Yes, since yaml2obj can't do it and the test only runs on Linux anyway, it seems reasonable.

[zhuhan0]

Hi, I have just got the time to come back to https://reviews.llvm.org/D147823, but I wanna check here first if you guys have any fix in flight? In addition to the linking issue, we also encountered the "cstdio" not found issue as mentioned above.

[argentite]

I made patch with both precompiled library and without cstdio. D148992

@dyung We can also enable this now on PS4/5 if you are interested.

[dyung]

In D141824#4289752, @argentite wrote:

I made patch with both precompiled library and without cstdio. D148992

@dyung We can also enable this now on PS4/5 if you are interested.

Sure. Please try it out and see if it works. More coverage is always good.

clang/include/clang/Interpreter/Interpreter.h

 
 #include <memory>
 #include <vector>
 
 namespace llvm {
 namespace orc {
 class LLJIT;
 class ThreadSafeContext;
-}
+} // namespace orc
 } // namespace llvm
 
 namespace clang {
 
 class CompilerInstance;
 class IncrementalExecutor;
 class IncrementalParser;
 
 /// Create a pre-configured \c CompilerInstance for incremental processing.
 class IncrementalCompilerBuilder {
 public:
   static llvm::Expected<std::unique_ptr<CompilerInstance>>
   create(std::vector<const char *> &ClangArgv);
 };
 
 /// Provides top-level interfaces for incremental compilation and execution.
 class Interpreter {
   std::unique_ptr<llvm::orc::ThreadSafeContext> TSCtx;
   std::unique_ptr<IncrementalParser> IncrParser;
   std::unique_ptr<IncrementalExecutor> IncrExecutor;
 
   Interpreter(std::unique_ptr<CompilerInstance> CI, llvm::Error &Err);
 
+  llvm::Error CreateExecutor();
+
 public:
   ~Interpreter();
   static llvm::Expected<std::unique_ptr<Interpreter>>
   create(std::unique_ptr<CompilerInstance> CI);
   const CompilerInstance *getCompilerInstance() const;
-  const llvm::orc::LLJIT *getExecutionEngine() const;
+  llvm::Expected<llvm::orc::LLJIT &> getExecutionEngine();
+
   llvm::Expected<PartialTranslationUnit &> Parse(llvm::StringRef Code);
   llvm::Error Execute(PartialTranslationUnit &T);
   llvm::Error ParseAndExecute(llvm::StringRef Code) {
     auto PTU = Parse(Code);
     if (!PTU)
       return PTU.takeError();
     if (PTU->TheModule)
       return Execute(*PTU);
     return llvm::Error::success();
   }
 
   /// Undo N previous incremental inputs.
   llvm::Error Undo(unsigned N = 1);
 
+  /// Link a dynamic library
+  llvm::Error LoadDynamicLibrary(const char *name);
+
   /// \returns the \c JITTargetAddress of a \c GlobalDecl. This interface uses
   /// the CodeGenModule's internal mangling cache to avoid recomputing the
   /// mangled name.
   llvm::Expected<llvm::JITTargetAddress> getSymbolAddress(GlobalDecl GD) const;
 
   /// \returns the \c JITTargetAddress of a given name as written in the IR.
   llvm::Expected<llvm::JITTargetAddress>
   getSymbolAddress(llvm::StringRef IRName) const;

clang/lib/Interpreter/IncrementalExecutor.h

public:
   ~IncrementalExecutor();
 
   llvm::Error addModule(PartialTranslationUnit &PTU);
   llvm::Error removeModule(PartialTranslationUnit &PTU);
   llvm::Error runCtors() const;
   llvm::Error cleanUp();
   llvm::Expected<llvm::JITTargetAddress>
   getSymbolAddress(llvm::StringRef Name, SymbolNameKind NameKind) const;
-  llvm::orc::LLJIT *getExecutionEngine() const { return Jit.get(); }
+
+  llvm::orc::LLJIT &GetExecutionEngine() { return *Jit; }
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_INTERPRETER_INCREMENTALEXECUTOR_H

clang/lib/Interpreter/Interpreter.cpp

 #include "clang/Driver/Driver.h"
 #include "clang/Driver/Job.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
 #include "clang/Lex/PreprocessorOptions.h"
 
+#include "llvm/ExecutionEngine/Orc/LLJIT.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/TargetParser/Host.h"
 
 using namespace clang;
 
 // FIXME: Figure out how to unify with namespace init_convenience from
 //        tools/clang-import-test/clang-import-test.cpp
if (Err)
     return std::move(Err);
   return std::move(Interp);
 }
 
 const CompilerInstance *Interpreter::getCompilerInstance() const {
   return IncrParser->getCI();
 }
 
-const llvm::orc::LLJIT *Interpreter::getExecutionEngine() const {
-  if (IncrExecutor)
-    return IncrExecutor->getExecutionEngine();
-  return nullptr;
+llvm::Expected<llvm::orc::LLJIT &> Interpreter::getExecutionEngine() {
+  if (!IncrExecutor) {
+    if (auto Err = CreateExecutor())
+      return Err;
+  }
+
+  return IncrExecutor->GetExecutionEngine();
 }
 
 llvm::Expected<PartialTranslationUnit &>
 Interpreter::Parse(llvm::StringRef Code) {
   return IncrParser->Parse(Code);
 }
 
-llvm::Error Interpreter::Execute(PartialTranslationUnit &T) {
-  assert(T.TheModule);
-  if (!IncrExecutor) {
-    const clang::TargetInfo &TI =
-        getCompilerInstance()->getASTContext().getTargetInfo();
-    llvm::Error Err = llvm::Error::success();
-    IncrExecutor = std::make_unique<IncrementalExecutor>(*TSCtx, Err, TI);
+llvm::Error Interpreter::CreateExecutor() {
+  auto Executor = std::make_unique<IncrementalExecutor>(*TSCtx, Err, TI);
+  if (!Err)
+    IncrExecutor = std::move(Executor);
 
+  return Err;
+}
+
+llvm::Error Interpreter::Execute(PartialTranslationUnit &T) {
+  assert(T.TheModule);
+  if (!IncrExecutor) {
+    auto Err = CreateExecutor();
     if (Err)
       return Err;
   }
   // FIXME: Add a callback to retain the llvm::Module once the JIT is done.
   if (auto Err = IncrExecutor->addModule(T))
     return Err;
 
   if (auto Err = IncrExecutor->runCtors())
if (IncrExecutor) {
         return Err;
     }
 
     IncrParser->CleanUpPTU(PTUs.back());
     PTUs.pop_back();
   }
   return llvm::Error::success();
 }
+
+llvm::Error Interpreter::LoadDynamicLibrary(const char *name) {
+  auto EE = getExecutionEngine();
+  if (!EE)
+    return EE.takeError();
+
+  auto &DL = EE->getDataLayout();
+
+  if (auto DLSG = llvm::orc::DynamicLibrarySearchGenerator::Load(
+          name, DL.getGlobalPrefix()))
+    EE->getMainJITDylib().addGenerator(std::move(*DLSG));
+  else
+    return DLSG.takeError();
+
+  return llvm::Error::success();
+}

clang/test/Interpreter/Inputs/dynamic-library-test.cpp

+int ultimate_answer = 0;
+
+int calculate_answer() {
+  ultimate_answer = 42;
+  return 5;
+}

clang/test/Interpreter/dynamic-library.cpp

+// REQUIRES: host-supports-jit, system-linux
+
+// RUN: %clang -xc++ -o %T/libdynamic-library-test.so -fPIC -shared -DLIBRARY %S/Inputs/dynamic-library-test.cpp
+// RUN: cat %s | env LD_LIBRARY_PATH=%T:$LD_LIBRARY_PATH clang-repl | FileCheck %s
+
+#include <cstdio>
+
+extern int ultimate_answer;
+int calculate_answer();
+
+%lib libdynamic-library-test.so
+
+printf("Return value: %d\n", calculate_answer());
+// CHECK: Return value: 5
+
+printf("Variable: %d\n", ultimate_answer);
+// CHECK-NEXT: Variable: 42
+
+%quit

clang/tools/clang-repl/ClangRepl.cpp

while (std::optional<std::string> Line = LE.readLine()) {
         break;
       if (*Line == R"(%undo)") {
         if (auto Err = Interp->Undo()) {
           llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
           HasError = true;
         }
         continue;
       }
+      if (Line->rfind("%lib ", 0) == 0) {
+        if (auto Err = Interp->LoadDynamicLibrary(Line->data() + 5)) {
+          llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
+          HasError = true;
+        }
+        continue;
+      }
 
       if (auto Err = Interp->ParseAndExecute(*Line)) {
         llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
         HasError = true;
       }
     }
   }
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any
   // later errors use the default handling behavior instead.
   llvm::remove_fatal_error_handler();
 
   return checkDiagErrors(Interp->getCompilerInstance(), HasError);
 }