[clang-repl] : Fix clang-repl crash with --cuda flag #136404
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang Author: Anutosh Bhat (anutosh491) Changes
The underlying issue was that the Unlike the host-side To fix this, I refactored the Full diff: https://github.com/llvm/llvm-project/pull/136404.diff 3 Files Affected:
diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h
index b1b63aedf86ab..20d22c4437a97 100644
--- a/clang/include/clang/Interpreter/Interpreter.h
+++ b/clang/include/clang/Interpreter/Interpreter.h
@@ -129,7 +129,8 @@ class Interpreter {
public:
virtual ~Interpreter();
static llvm::Expected<std::unique_ptr<Interpreter>>
- create(std::unique_ptr<CompilerInstance> CI);
+ create(std::unique_ptr<CompilerInstance> CI,
+ std::unique_ptr<CompilerInstance> DeviceCI = nullptr);
static llvm::Expected<std::unique_ptr<Interpreter>>
createWithCUDA(std::unique_ptr<CompilerInstance> CI,
std::unique_ptr<CompilerInstance> DCI);
diff --git a/clang/lib/Interpreter/DeviceOffload.cpp b/clang/lib/Interpreter/DeviceOffload.cpp
index 1999d63d1aa04..9a7be006250a0 100644
--- a/clang/lib/Interpreter/DeviceOffload.cpp
+++ b/clang/lib/Interpreter/DeviceOffload.cpp
@@ -34,14 +34,15 @@ IncrementalCUDADeviceParser::IncrementalCUDADeviceParser(
TargetOpts(HostInstance.getTargetOpts()) {
if (Err)
return;
- DeviceCI = std::move(DeviceInstance);
StringRef Arch = TargetOpts.CPU;
if (!Arch.starts_with("sm_") || Arch.substr(3).getAsInteger(10, SMVersion)) {
+ DeviceInstance.release();
Err = llvm::joinErrors(std::move(Err), llvm::make_error<llvm::StringError>(
"Invalid CUDA architecture",
llvm::inconvertibleErrorCode()));
return;
}
+ DeviceCI = std::move(DeviceInstance);
}
llvm::Expected<TranslationUnitDecl *>
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index f8c8d0a425659..1af3f24774028 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -451,13 +451,44 @@ const char *const Runtimes = R"(
)";
llvm::Expected<std::unique_ptr<Interpreter>>
-Interpreter::create(std::unique_ptr<CompilerInstance> CI) {
+Interpreter::create(std::unique_ptr<CompilerInstance> CI,
+ std::unique_ptr<CompilerInstance> DeviceCI) {
llvm::Error Err = llvm::Error::success();
auto Interp =
std::unique_ptr<Interpreter>(new Interpreter(std::move(CI), Err));
if (Err)
return std::move(Err);
+ if (DeviceCI) {
+ // auto DeviceLLVMCtx = std::make_unique<llvm::LLVMContext>();
+ // auto DeviceTSCtx =
+ // std::make_unique<llvm::orc::ThreadSafeContext>(std::move(DeviceLLVMCtx));
+
+ // llvm::Error DeviceErr = llvm::Error::success();
+ // llvm::ErrorAsOutParameter EAO(&DeviceErr);
+
+ // auto DeviceAct = std::make_unique<IncrementalAction>(
+ // *DeviceCI, *DeviceTSCtx->getContext(), DeviceErr, *Interp);
+
+ // if (DeviceErr)
+ // return std::move(DeviceErr);
+
+ //DeviceCI->ExecuteAction(*DeviceAct);
+ DeviceCI->ExecuteAction(*Interp->Act);
+
+ llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> IMVFS =
+ std::make_unique<llvm::vfs::InMemoryFileSystem>();
+
+ auto DeviceParser = std::make_unique<IncrementalCUDADeviceParser>(
+ std::move(DeviceCI), *Interp->getCompilerInstance(), IMVFS, Err,
+ Interp->PTUs);
+
+ if (Err)
+ return std::move(Err);
+
+ Interp->DeviceParser = std::move(DeviceParser);
+ }
+
// Add runtime code and set a marker to hide it from user code. Undo will not
// go through that.
auto PTU = Interp->Parse(Runtimes);
@@ -481,20 +512,7 @@ Interpreter::createWithCUDA(std::unique_ptr<CompilerInstance> CI,
OverlayVFS->pushOverlay(IMVFS);
CI->createFileManager(OverlayVFS);
- auto Interp = Interpreter::create(std::move(CI));
- if (auto E = Interp.takeError())
- return std::move(E);
-
- llvm::Error Err = llvm::Error::success();
- auto DeviceParser = std::make_unique<IncrementalCUDADeviceParser>(
- std::move(DCI), *(*Interp)->getCompilerInstance(), IMVFS, Err,
- (*Interp)->PTUs);
- if (Err)
- return std::move(Err);
-
- (*Interp)->DeviceParser = std::move(DeviceParser);
-
- return Interp;
+ return Interpreter::create(std::move(CI), std::move(DCI));
}
const CompilerInstance *Interpreter::getCompilerInstance() const {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This fixes this discussion y'all had #107737 (comment) Unfortunately I am on an Apple m1 so I can just justify seeing this on my side But actually this error is more justified that what we are seeing with clang-repl-19 So everything spawns from here llvm-project/clang/tools/clang-repl/ClangRepl.cpp Lines 207 to 212 in 3bcb724 But for The first one comes from here llvm-project/clang/lib/Interpreter/DeviceOffload.cpp Lines 38 to 43 in 3bcb724 ii) clang-repl-19: Passes that, successfully creates the interpreter and ends up failing at the Loading step (although we should expect it to fail on the top itself isn't it ?) |
anutosh491
commented
Apr 19, 2025
|
With these changes, I am positive we atleast have the design in place to run any of these Cuda tests (https://github.com/llvm/llvm-project/tree/main/clang/test/Interpreter/CUDA) For example running the sanity.cu file The only thing which I am kinda struggling to figure out is how to put the virtual filesystem to use. So to get the above tests working, I had to just comment out what we do through VFS and use the real filesystem So hopefully the VFS used here and here llvm-project/clang/lib/CodeGen/CGCUDANV.cpp Lines 791 to 792 in bea110d Should be the same, but my logs tell me otherwise. I added some simple logs to verify the same and I see this So if you see both VFS point to different locations. So the current changes would just fail with Once we start fetching these files from the correct location, this should be ready ! |
|
Was able to address the issue here. Making a commit soon ! |
c4c00cb to
6c64e64
Compare
|
Making a note of the major changes below ! |
anutosh491
commented
Apr 24, 2025
anutosh491
commented
Apr 24, 2025
anutosh491
commented
Apr 24, 2025
vgvassilev
reviewed
Apr 24, 2025
vgvassilev
reviewed
Apr 24, 2025
vgvassilev
reviewed
Apr 24, 2025
bbf41f4 to
d10dab4
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/15088 Here is the relevant piece of the build log for the reference |
|
/cherry-pick 21fb19f |
|
Failed to cherry-pick: 21fb19f https://github.com/llvm/llvm-project/actions/runs/14705725537 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
|
/cherry-pick 21fb19f |
|
/pull-request #137615 |
swift-ci
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 29, 2025
`clang-repl --cuda` was previously crashing with a segmentation fault, instead of reporting a clean error ``` (base) anutosh491@Anutoshs-MacBook-Air bin % ./clang-repl --cuda #0 0x0000000111da4fbc llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/opt/local/libexec/llvm-20/lib/libLLVM.dylib+0x150fbc) #1 0x0000000111da31dc llvm::sys::RunSignalHandlers() (/opt/local/libexec/llvm-20/lib/libLLVM.dylib+0x14f1dc) #2 0x0000000111da5628 SignalHandler(int) (/opt/local/libexec/llvm-20/lib/libLLVM.dylib+0x151628) #3 0x000000019b242de4 (/usr/lib/system/libsystem_platform.dylib+0x180482de4) #4 0x0000000107f638d0 clang::IncrementalCUDADeviceParser::IncrementalCUDADeviceParser(std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>, clang::CompilerInstance&, llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>, llvm::Error&, std::__1::list<clang::PartialTranslationUnit, std::__1::allocator<clang::PartialTranslationUnit>> const&) (/opt/local/libexec/llvm-20/lib/libclang-cpp.dylib+0x216b8d0) #5 0x0000000107f638d0 clang::IncrementalCUDADeviceParser::IncrementalCUDADeviceParser(std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>, clang::CompilerInstance&, llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>, llvm::Error&, std::__1::list<clang::PartialTranslationUnit, std::__1::allocator<clang::PartialTranslationUnit>> const&) (/opt/local/libexec/llvm-20/lib/libclang-cpp.dylib+0x216b8d0) #6 0x0000000107f6bac8 clang::Interpreter::createWithCUDA(std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>, std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>) (/opt/local/libexec/llvm-20/lib/libclang-cpp.dylib+0x2173ac8) #7 0x000000010206f8a8 main (/opt/local/libexec/llvm-20/bin/clang-repl+0x1000038a8) #8 0x000000019ae8c274 Segmentation fault: 11 ``` The underlying issue was that the `DeviceCompilerInstance` (used for device-side CUDA compilation) was never initialized with a `Sema`, which is required before constructing the `IncrementalCUDADeviceParser`. https://github.com/llvm/llvm-project/blob/89687e6f383b742a3c6542dc673a84d9f82d02de/clang/lib/Interpreter/DeviceOffload.cpp#L32 https://github.com/llvm/llvm-project/blob/89687e6f383b742a3c6542dc673a84d9f82d02de/clang/lib/Interpreter/IncrementalParser.cpp#L31 Unlike the host-side `CompilerInstance` which runs `ExecuteAction` inside the Interpreter constructor (thereby setting up Sema), the device-side CI was passed into the parser uninitialized, leading to an assertion or crash when accessing its internals. To fix this, I refactored the `Interpreter::create` method to include an optional `DeviceCI` parameter. If provided, we know we need to take care of this instance too. Only then do we construct the `IncrementalCUDADeviceParser`. (cherry picked from commit 21fb19f)
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
`clang-repl --cuda` was previously crashing with a segmentation fault, instead of reporting a clean error ``` (base) anutosh491@Anutoshs-MacBook-Air bin % ./clang-repl --cuda #0 0x0000000111da4fbc llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/opt/local/libexec/llvm-20/lib/libLLVM.dylib+0x150fbc) llvm#1 0x0000000111da31dc llvm::sys::RunSignalHandlers() (/opt/local/libexec/llvm-20/lib/libLLVM.dylib+0x14f1dc) llvm#2 0x0000000111da5628 SignalHandler(int) (/opt/local/libexec/llvm-20/lib/libLLVM.dylib+0x151628) llvm#3 0x000000019b242de4 (/usr/lib/system/libsystem_platform.dylib+0x180482de4) llvm#4 0x0000000107f638d0 clang::IncrementalCUDADeviceParser::IncrementalCUDADeviceParser(std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>, clang::CompilerInstance&, llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>, llvm::Error&, std::__1::list<clang::PartialTranslationUnit, std::__1::allocator<clang::PartialTranslationUnit>> const&) (/opt/local/libexec/llvm-20/lib/libclang-cpp.dylib+0x216b8d0) llvm#5 0x0000000107f638d0 clang::IncrementalCUDADeviceParser::IncrementalCUDADeviceParser(std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>, clang::CompilerInstance&, llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem>, llvm::Error&, std::__1::list<clang::PartialTranslationUnit, std::__1::allocator<clang::PartialTranslationUnit>> const&) (/opt/local/libexec/llvm-20/lib/libclang-cpp.dylib+0x216b8d0) llvm#6 0x0000000107f6bac8 clang::Interpreter::createWithCUDA(std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>, std::__1::unique_ptr<clang::CompilerInstance, std::__1::default_delete<clang::CompilerInstance>>) (/opt/local/libexec/llvm-20/lib/libclang-cpp.dylib+0x2173ac8) llvm#7 0x000000010206f8a8 main (/opt/local/libexec/llvm-20/bin/clang-repl+0x1000038a8) llvm#8 0x000000019ae8c274 Segmentation fault: 11 ``` The underlying issue was that the `DeviceCompilerInstance` (used for device-side CUDA compilation) was never initialized with a `Sema`, which is required before constructing the `IncrementalCUDADeviceParser`. https://github.com/llvm/llvm-project/blob/89687e6f383b742a3c6542dc673a84d9f82d02de/clang/lib/Interpreter/DeviceOffload.cpp#L32 https://github.com/llvm/llvm-project/blob/89687e6f383b742a3c6542dc673a84d9f82d02de/clang/lib/Interpreter/IncrementalParser.cpp#L31 Unlike the host-side `CompilerInstance` which runs `ExecuteAction` inside the Interpreter constructor (thereby setting up Sema), the device-side CI was passed into the parser uninitialized, leading to an assertion or crash when accessing its internals. To fix this, I refactored the `Interpreter::create` method to include an optional `DeviceCI` parameter. If provided, we know we need to take care of this instance too. Only then do we construct the `IncrementalCUDADeviceParser`.
6 tasks
anutosh491
added a commit
that referenced
this pull request
Aug 26, 2025
…alParser (#137458) Read discussion : #136404 (comment) and the following comments for context Motivation 1) `IncrementalAction` is designed to keep Frontend statealive across inputs. As per the docstring: “IncrementalAction ensures it keeps its underlying action's objects alive as long as the IncrementalParser needs them.” 2) To align responsibilities with that contract, the parser layer (host: `IncrementalParser`, device: `IncrementalCUDADeviceParser`) should manage PTU registration and module generation, while the interpreter orchestrates at a higher level. What this PR does 1) Moves CodeGen surfaces behind IncrementalAction: GenModule(), getCodeGen(), and the cached “first CodeGen module” now live in IncrementalAction. 2) Moves PTU ownership to the parser layer: Adds IncrementalParser::RegisterPTU(…) (and device counterpart) 3) Add device-side registration in IncrementalCUDADeviceParser. 4) Remove Interpreter::{getCodeGen, GenModule, RegisterPTU}.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Aug 26, 2025
…o IncrementalParser (#137458) Read discussion : llvm/llvm-project#136404 (comment) and the following comments for context Motivation 1) `IncrementalAction` is designed to keep Frontend statealive across inputs. As per the docstring: “IncrementalAction ensures it keeps its underlying action's objects alive as long as the IncrementalParser needs them.” 2) To align responsibilities with that contract, the parser layer (host: `IncrementalParser`, device: `IncrementalCUDADeviceParser`) should manage PTU registration and module generation, while the interpreter orchestrates at a higher level. What this PR does 1) Moves CodeGen surfaces behind IncrementalAction: GenModule(), getCodeGen(), and the cached “first CodeGen module” now live in IncrementalAction. 2) Moves PTU ownership to the parser layer: Adds IncrementalParser::RegisterPTU(…) (and device counterpart) 3) Add device-side registration in IncrementalCUDADeviceParser. 4) Remove Interpreter::{getCodeGen, GenModule, RegisterPTU}.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
clang-repl --cudawas previously crashing with a segmentation fault, instead of reporting a clean errorThe underlying issue was that the
DeviceCompilerInstance(used for device-side CUDA compilation) was never initialized with aSema, which is required before constructing theIncrementalCUDADeviceParser.llvm-project/clang/lib/Interpreter/DeviceOffload.cpp
Line 32 in 89687e6
llvm-project/clang/lib/Interpreter/IncrementalParser.cpp
Line 31 in 89687e6
Unlike the host-side
CompilerInstancewhich runsExecuteActioninside the Interpreter constructor (thereby setting up Sema), the device-side CI was passed into the parser uninitialized, leading to an assertion or crash when accessing its internals.To fix this, I refactored the
Interpreter::createmethod to include an optionalDeviceCIparameter. If provided, we know we need to take care of this instance too. Only then do we construct theIncrementalCUDADeviceParser.