Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error propagation to interpreter creation logic #553

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+39 −13
Open

Add error propagation to interpreter creation logic #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8cb3ec
Fixing creation of Interpreter
anutosh491 Apr 4, 2025
eb0116d
Update environment-wasm.yml
anutosh491 Apr 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions environment-wasm.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ channels:
- https://repo.prefix.dev/emscripten-forge-dev
dependencies:
- emscripten-abi==3.1.73
- nlohmann_json
- nlohmann_json=3.11.3
- xeus-lite
- xeus
- cpp-argparse
- pugixml
- doctest
- doctest
11 changes: 9 additions & 2 deletions lib/Interpreter/CXCppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,15 @@
}

CXInterpreter clang_createInterpreter(const char* const* argv, int argc) {
auto* I = new CXInterpreterImpl(); // NOLINT(*-owning-memory)
auto I = std::make_unique<CXInterpreterImpl>();

Check warning on line 274 in lib/Interpreter/CXCppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/Interpreter/CXCppInterOp.cpp#L274 

Added line #L274 was not covered by tests
#ifdef CPPINTEROP_USE_CLING
I->Interp = std::make_unique<compat::Interpreter>(argc, argv);
#else
I->Interp = compat::Interpreter::create(argc, argv);
if (!I->Interp) {
return nullptr;

Check warning on line 280 in lib/Interpreter/CXCppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/Interpreter/CXCppInterOp.cpp#L278-L280 

Added lines #L278 - L280 were not covered by tests
}
#endif
// create a bridge between CXTranslationUnit and clang::Interpreter
// auto AU = std::make_unique<ASTUnit>(false);
// AU->FileMgr = I->Interp->getCompilerInstance().getFileManager();
Expand All @@ -281,7 +288,7 @@
// AU->Ctx = &I->Interp->getSema().getASTContext();
// I->TU.reset(MakeCXTranslationUnit(static_cast<CIndexer*>(clang_createIndex(0,
// 0)), AU));
return I;
return I.release();

Check warning on line 291 in lib/Interpreter/CXCppInterOp.cpp

View check run for this annotation

Codecov / codecov/patch

lib/Interpreter/CXCppInterOp.cpp#L291 

Added line #L291 was not covered by tests
}

CXInterpreter clang_createInterpreterFromRawPtr(TInterp_t I) {
Expand Down
4 changes: 3 additions & 1 deletion lib/Interpreter/Compatibility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,11 @@ createClangInterpreter(std::vector<const char*>& args) {
return nullptr;
}
if (CudaEnabled) {
if (auto Err = (*innerOrErr)->LoadDynamicLibrary("libcudart.so"))
if (auto Err = (*innerOrErr)->LoadDynamicLibrary("libcudart.so")) {
llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(),
"Failed load libcudart.so runtime:");
return nullptr;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was some inconsistency that I saw along the way. We return a nullptr whenever clang-repl uses ExitOnErr except this case (https://github.com/llvm/llvm-project/blob/main/clang/tools/clang-repl/ClangRepl.cpp#L212)

So clang-repl would just exit out ... through something like

(clang-repl-env-19) anutosh491@Anutoshs-MacBook-Air bin % ./clang-repl --cuda
clang-repl: dlopen(libcudart.so, 0x0009): tried: 'libcudart.so' (no such file), '/System/Volumes/Preboot/Cryptexes/OSlibcudart.so' (no such file), '/Users/anutosh491/micromamba/envs/clang-repl-env-19/lib/../lib/libcudart.so' (no such file), '/Users/anutosh491/micromamba/envs/clang-repl-env-19/lib/libcudart.so' (no such file), '/Users/anutosh491/micromamba/envs/clang-repl-env-19/bin/../lib/libcudart.so' (no such file), '/Users/anutosh491/micromamba/envs/clang-repl-env-19/bin/../lib/libcudart.so' (no such file), '/usr/lib/libcudart.so' (no such file, not in dyld cache), 'libcudart.so' (no such file), '/usr/local/lib/libcudart.so' (no such file), '/usr/lib/libcudart.so' (no such file, not in dyld cache)

But in our case we are continuing to use a half broken interpreter without the above working and we run declare on top of this

CppInterOp/unittests/CppInterOp/CUDATest.cpp

Lines 17 to 18 in c3384fb

return Cpp::Declare("__global__ void test_func() {}"
"test_func<<<1,1>>>();") == 0;

which is bound to not work as can be seen here

}
}

return std::move(*innerOrErr);
Expand Down
8 changes: 8 additions & 0 deletions lib/Interpreter/CppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2957,7 +2957,15 @@ namespace Cpp {
std::back_inserter(ClingArgv),
[&](const std::string& str) { return str.c_str(); });

#ifdef CPPINTEROP_USE_CLING
auto I = new compat::Interpreter(ClingArgv.size(), &ClingArgv[0]);
#else
auto Interp = compat::Interpreter::create(
static_cast<int>(ClingArgv.size()), ClingArgv.data());
if (!Interp)
return nullptr;
auto* I = Interp.release();
#endif

// Honor -mllvm.
//
Expand Down
19 changes: 14 additions & 5 deletions lib/Interpreter/CppInterOpInterpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,14 @@ class Interpreter {
private:
std::unique_ptr<clang::Interpreter> inner;

Interpreter(std::unique_ptr<clang::Interpreter> CI) : inner(std::move(CI)) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "std::move" is directly included [misc-include-cleaner]

lib/Interpreter/CppInterOpInterpreter.h:24:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <utility>
+ #if CLANG_VERSION_MAJOR >= 19


public:
Interpreter(int argc, const char* const* argv, const char* llvmdir = 0,
const std::vector<std::shared_ptr<clang::ModuleFileExtension>>&
moduleExtensions = {},
void* extraLibHandle = 0, bool noRuntime = true) {
static std::unique_ptr<Interpreter>
create(int argc, const char* const* argv, const char* llvmdir = nullptr,
const std::vector<std::shared_ptr<clang::ModuleFileExtension>>&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "clang::ModuleFileExtension" is directly included [misc-include-cleaner]

lib/Interpreter/CppInterOpInterpreter.h:24:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <clang/Serialization/ModuleFileExtension.h>
+ #if CLANG_VERSION_MAJOR >= 19

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "std::shared_ptr" is directly included [misc-include-cleaner]

         const std::vector<std::shared_ptr<clang::ModuleFileExtension>>&
                                ^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "std::vector" is directly included [misc-include-cleaner]

lib/Interpreter/CppInterOpInterpreter.h:24:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <vector>
+ #if CLANG_VERSION_MAJOR >= 19

moduleExtensions = {},
void* extraLibHandle = nullptr, bool noRuntime = true) {
// Initialize all targets (required for device offloading)
llvm::InitializeAllTargetInfos();
llvm::InitializeAllTargets();
Expand All @@ -150,7 +153,13 @@ class Interpreter {
std::vector<const char*> vargs(argv + 1, argv + argc);
vargs.push_back("-include");
vargs.push_back("new");
inner = compat::createClangInterpreter(vargs);
auto CI = compat::createClangInterpreter(vargs);
if (!CI) {
llvm::errs() << "Interpreter creation failed\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "llvm::errs" is directly included [misc-include-cleaner]

lib/Interpreter/CppInterOpInterpreter.h:24:

- #if CLANG_VERSION_MAJOR >= 19
+ #include <llvm/Support/raw_ostream.h>
+ #if CLANG_VERSION_MAJOR >= 19

return nullptr;
}

return std::unique_ptr<Interpreter>(new Interpreter(std::move(CI)));
}

~Interpreter() {}
Expand Down
6 changes: 3 additions & 3 deletions unittests/CppInterOp/CUDATest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ static bool HasCudaSDK() {
// FIXME: Enable this for cling.
return false;
#endif // CLANG_VERSION_MAJOR < 16
Cpp::CreateInterpreter({}, {"--cuda"});
if(!Cpp::CreateInterpreter({}, {"--cuda"})) return false;
return Cpp::Declare("__global__ void test_func() {}"
"test_func<<<1,1>>>();") == 0;
};
Expand All @@ -30,7 +30,7 @@ static bool HasCudaRuntime() {
if (!HasCudaSDK())
return false;

Cpp::CreateInterpreter({}, {"--cuda"});
if(!Cpp::CreateInterpreter({}, {"--cuda"})) return false;
if (Cpp::Declare("__global__ void test_func() {}"
"test_func<<<1,1>>>();"))
return false;
Expand Down Expand Up @@ -74,4 +74,4 @@ TEST(CUDATest, CUDARuntime) {
GTEST_SKIP() << "Skipping CUDA tests as CUDA runtime not found";

EXPECT_TRUE(HasCudaRuntime());
}
}
Loading