Add error propagation to interpreter creation logic #553
Conversation
anutosh491
changed the title
|
Hence, we had a couple approaches to go ahead here i) Introduce a isValid for ii) What Vipul and I liked was propagating the error all the way to that top, so that a half broken interpreter can't be put to use and a utility function like parse/declare would only work if CreateInterpreter doesn't return null along with an error message saying Interpreter creation failed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #553 +/- ##
==========================================
- Coverage 75.94% 75.93% -0.01%
==========================================
Files 9 9
Lines 3646 3657 +11
==========================================
+ Hits 2769 2777 +8
- Misses 877 880 +3
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -136,11 +136,14 @@ class Interpreter { | |||
| private: | |||
| std::unique_ptr<clang::Interpreter> inner; | |||
|
|
|||
| Interpreter(std::unique_ptr<clang::Interpreter> CI) : inner(std::move(CI)) {} | |||
There was a problem hiding this comment.
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| void* extraLibHandle = 0, bool noRuntime = true) { | ||
| static std::unique_ptr<Interpreter> | ||
| create(int argc, const char* const* argv, const char* llvmdir = 0, | ||
| const std::vector<std::shared_ptr<clang::ModuleFileExtension>>& |
There was a problem hiding this comment.
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| void* extraLibHandle = 0, bool noRuntime = true) { | ||
| static std::unique_ptr<Interpreter> | ||
| create(int argc, const char* const* argv, const char* llvmdir = 0, | ||
| const std::vector<std::shared_ptr<clang::ModuleFileExtension>>& |
There was a problem hiding this comment.
warning: no header providing "std::shared_ptr" is directly included [misc-include-cleaner]
const std::vector<std::shared_ptr<clang::ModuleFileExtension>>&
^| void* extraLibHandle = 0, bool noRuntime = true) { | ||
| static std::unique_ptr<Interpreter> | ||
| create(int argc, const char* const* argv, const char* llvmdir = 0, | ||
| const std::vector<std::shared_ptr<clang::ModuleFileExtension>>& |
There was a problem hiding this comment.
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
| llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), | ||
| "Failed load libcudart.so runtime:"); | ||
| return nullptr; |
There was a problem hiding this comment.
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
which is bound to not work as can be seen here
Description
@Vipul-Cariappa and I were discussing a use case yesterday (especially while building with repl) and this is what we realized
The function responsible for creating
clang::InterpreteriscreateClangInterpreterwhich can return a nullptr, for egCppInterOp/lib/Interpreter/Compatibility.h
Line 277 in c3384fb
This is assigned to
innerwhile the constructor forcompat::Interpreteris calledCppInterOp/lib/Interpreter/CppInterOpInterpreter.h
Line 153 in c3384fb
which means
innercan benullptrCreateInterpreterfrom the top, we're creating thecompat::InterpreterhereCppInterOp/lib/Interpreter/CppInterOp.cpp
Line 2960 in c3384fb
Which means
compat::Interpretercan exist even when the underlyingclang::Interpreteris not existing. So we can say that the Interpreter from the top is half brokenWe would hit the following finally
CppInterOp/lib/Interpreter/CppInterOpInterpreter.h
Lines 174 to 176 in c3384fb
And although the call is made to
compat::Interpreter, we end up using the underlyingclang::Interpreterwithout checking if it is valid.Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist