[libcu++] Add stream debugger pretty-printers - #10520
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds GDB and LLDB pretty-printers for ChangesCUDA stream debugger support
Assessment against linked issues
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libcudacxx/share/libcudacxx/lldb/stream.py (1)
230-289: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCapture status (and full metadata) is queried twice per print.
stream_summaryqueries_CUDA_STREAM_IS_CAPTURINGand, when not capturing,_unique_id.StreamSyntheticProvider.update()then calls_stream_info, which re-queries capture status independently before fetching device/priority/flags. Since both are registered together (type summary add --expand+type synthetic add), a singleframe variable -Aon a stream triggers this capture-status query at least twice, plus separate malloc/free round-trips for the same logical data.♻️ Suggested approach
Cache the combined
StreamInfo(and handle/unique_id) per(process, load-address, stop-id)the first time either callback runs for a given stop, and have bothstream_summaryandStreamSyntheticProvider.update()read from that cache instead of re-issuing inferior calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f905598-abbe-4273-99d8-de9a178d8b30
📒 Files selected for processing (7)
libcudacxx/share/libcudacxx/gdb/stream.pylibcudacxx/share/libcudacxx/lldb/stream.pylibcudacxx/test/debugging/run_pretty_printer_test.pylibcudacxx/test/debugging/stream/CMakeLists.txtlibcudacxx/test/debugging/stream/gdb.expectedlibcudacxx/test/debugging/stream/lldb.expectedlibcudacxx/test/debugging/stream/source.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- libcudacxx/test/debugging/stream/CMakeLists.txt
…y-printers # Conflicts: # libcudacxx/share/libcudacxx/gdb/__init__.py # libcudacxx/share/libcudacxx/lldb/__init__.py
|
/ok to test 2aa22cd |
This comment has been minimized.
This comment has been minimized.
|
/ok to test 44b233e |
|
/ok to test df63bda |
…nto feat/add-stream-pretty-printers
|
@Jacobfaib, apologies for the delayed response. I live near Naples, close to the area affected by the earthquake a couple of days ago. Fortunately, I am safe, and I have now been able to get back to this PR. I reworked the LLDB printer based on your feedback and replied to the inline comments. In summary:
Across 15 separate LLDB runs, the initial metadata evaluation took 10.5 to 15.7 ms, with a 12.2 ms median. All stream pretty-printer cases pass with both LLDB and GDB after merging the latest When you have time, could you please take another look? |
|
/ok to test 6e4f734 |
…y-printers # Conflicts: # libcudacxx/share/libcudacxx/gdb/__init__.py # libcudacxx/share/libcudacxx/lldb/__init__.py
|
@Jacobfaib, I addressed both follow-up comments in b447d46, added an explicit no-current-context regression for both debuggers, and merged the latest |
This comment has been minimized.
This comment has been minimized.
|
/ok to test b447d46 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jacobfaib
left a comment
Jacobfaib
left a comment
There was a problem hiding this comment.
OK it is still a bit... overengineered, but at this point I feel it is relatively feature complete and the complexity is something we will just have to live with.
Thanks for the contribution!
|
@JJordan0C can you rebase the branch over latest main to resolve the conflicts? Then we can merge |
…y-printers # Conflicts: # libcudacxx/share/libcudacxx/gdb/__init__.py # libcudacxx/share/libcudacxx/lldb/__init__.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
libcudacxx/share/libcudacxx/gdb/stream.py (1)
137-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: When
cudaStreamIsCapturingcannot be called (capture_status is None), line 148 takes the capture branch and drops device, priority, flags, and unique ID. On a runtime or build where only that one query fails, the printer reports nothing instead of the metadata it could still obtain. Separate the two cases:- if capture_status != _CUDA_STREAM_CAPTURE_STATUS_NONE: + if ( + capture_status is not None + and capture_status != _CUDA_STREAM_CAPTURE_STATUS_NONE + ): return StreamInfo(handle, description, None, None, None, is_capturing, None)libcudacxx/share/libcudacxx/lldb/stream.py (2)
172-183: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winsuggestion: The fallback retries the whole expression when the first evaluation fails for any reason, not only for a missing
cudaStreamGetDevice. That doubles the cost of every genuinely failing print (unresolvablecuStreamGetId, no driver symbols, hung expression). Cache the outcome of the device-query probe once per target so that later prints skip the first form after it fails.
142-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winsuggestion: Configure formatter expressions with
SetSuppressPersistentResult(True)to prevent$Nvariables from accumulating, andSetTryAllThreads(False)to prevent LLDB from resuming other threads. Do not add a two-second timeout: LLDB’s default timeout is unlimited, and a fixed limit could interrupt valid CUDA calls.libcudacxx/test/debugging/stream/source.cu (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Add a shared debugging-test header for
keep_for_debuggerand include it from all nine fixtures. No existing common header provides this identical helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c64fc71d-a2c9-4a53-a2e4-a3bf37a1b5ca
📒 Files selected for processing (10)
libcudacxx/share/libcudacxx/gdb/__init__.pylibcudacxx/share/libcudacxx/gdb/stream.pylibcudacxx/share/libcudacxx/lldb/__init__.pylibcudacxx/share/libcudacxx/lldb/stream.pylibcudacxx/test/debugging/CMakeLists.txtlibcudacxx/test/debugging/run_pretty_printer_test.pylibcudacxx/test/debugging/stream/CMakeLists.txtlibcudacxx/test/debugging/stream/gdb.expectedlibcudacxx/test/debugging/stream/lldb.expectedlibcudacxx/test/debugging/stream/source.cu
🚧 Files skipped from review as they are similar to previous changes (5)
- libcudacxx/share/libcudacxx/lldb/init.py
- libcudacxx/share/libcudacxx/gdb/init.py
- libcudacxx/test/debugging/stream/CMakeLists.txt
- libcudacxx/test/debugging/CMakeLists.txt
- libcudacxx/test/debugging/run_pretty_printer_test.py
|
/ok to test dd262c4 |
⏱️ CCCL compile-time benchmark comparison: Public headers compile-time benchResult: 0 regression row(s), 24 improvement row(s) above threshold.
Artifacts: reports and traces TU total compilation
🟢 TU total compilation — Improvements
Direct file processing
🟢 Direct file processing — Improvements
|
🥳 CI Workflow Results🟩 Finished in 1h 23m: Pass: 100%/68 | Total: 1d 18h | Max: 1h 23m | Hits: 63%/663120See results here. |
3 participants
Description
closes #10095
Add GDB and LLDB pretty-printers for
cuda::streamandcuda::stream_ref.The formatters expose the wrapped native handle without invoking CUDA APIs in the inferior, and provide stable descriptions for default, legacy, per-thread, invalid, and moved-from stream states. The debugger test matrix covers owning streams, references, aliases, and each special handle state.
Validation
libcudacxx.test.debuggingtarget with CUDA 13.3, GCC 15.2, and SM 89.Checklist