[FEA]: Debugger pretty-printers: cuda::mr::shared_resource - #10229
[FEA]: Debugger pretty-printers: cuda::mr::shared_resource#10229ecgang wants to merge 2 commits into
cuda::mr::shared_resource#10229Conversation
Summarize shared ownership instead of the raw control block: the printers report the public type, the reference count read from the control block's atomic, and the address of the wrapped resource, so two handles to the same resource are distinguishable at a glance. A moved-from handle renders as empty. Registers the new module in both entry points and adds a pretty-printer scenario covering the unique, shared, aliased, moved-into, and moved-from states. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAdds GDB and LLDB pretty-printers for shared_resource pretty-printers
Assessment against linked issues
Possibly related PRs
Suggested reviewers: Comment |
|
/ok to test 6a96e8a |
🥳 CI Workflow Results🟩 Finished in 2h 51m: Pass: 100%/71 | Total: 16h 53m | Max: 2h 20m | Hits: 99%/338114See results here. |
|
Note for reviewers: the two red Both fail before any compilation, at environment creation: The available This PR adds Python pretty-printer modules and a debugging lit test; it touches no headers RAPIDS compiles against. The rest of the matrix is green (100%/71, 85 successful checks). |
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
Thanks a lot for improving the debugging experience 🎉
Jacobfaib
left a comment
Jacobfaib
left a comment
There was a problem hiding this comment.
Thank you for the patch! Just a few minor fixups required
| inspect_shared(second_reference); | ||
| inspect_alias(aliased_resource); | ||
| inspect_moved_into(moved_resource); | ||
| inspect_moved_from(donor_resource); |
There was a problem hiding this comment.
Can you include a scenario where a shared resource is inspected before and after it is copied? I.e.
auto resource = ...;
inspect(resource);
auto copy = resource;
inspect(resource);
inspect(copy);and the same for move? I realize you have similar scenarios already, but we want to make sure that the debuggers update the information for the same object before and after. LLDB pretty printers for example cache the output.
There was a problem hiding this comment.
Added, for both operations. inspect_before_copy and inspect_after_copy print the same variable, copy_source, on either side of copy_target = copy_source, so its use count has to read 1 then 2; the second stop also prints copy_target. The move pair does the same for move_source, which goes from owning to empty, and prints move_target at the second stop.
The old shared, moved_into, and moved_from cases fold into these pairs, since they pinned the same states without showing that the debuggers refresh them.
| return _SHARED_RESOURCE_PATTERN.fullmatch(type_name) is not None | ||
|
|
||
|
|
||
| def shared_resource_summary( |
There was a problem hiding this comment.
This should be a proper pretty printer, not just a summary. Its output should be modeled like std::shared_ptr.
This is probably based on the summary printer for the regular resource, but that needed a summary because gdb/lldb don't play nice when sub-printers produce newlines (so e.g. in buffer, we show the memory resource but need to do so without newlines).
There was a problem hiding this comment.
Done. LLDB now registers a synthetic provider next to the summary, and the GDB printer grew a matching children(). Both expose the owned resource as a resource child, so a handle can be expanded to reach the resource it owns, while the summary keeps carrying the use count and the address the way std::shared_ptr keeps strong=/weak= in its summary and the pointer in its child.
The child is the address rather than the resource itself, deliberately: yielding the payload object made GDB print the whole resource inline, which put cuda::mr::__4::, static __available_flags, and the resource private members into the golden file. That is exactly what public_type_name() exists to keep out, and it would break the test on an ABI namespace bump. One extra expansion step gets you the same drill-down without pinning any of it.
The summary is registered with --expand so frame variable shows the child, and update() returns False so LLDB re-reads after a copy or a move instead of serving what it cached.
| if address == lldb.LLDB_INVALID_ADDRESS: | ||
| return f"{type_name} use_count={use_count}" |
There was a problem hiding this comment.
We should print the address nonetheless, something like <invalid address>.
There was a problem hiding this comment.
Now prints resource=<invalid address>. Both printers carry a comment noting the branch guards an unreadable frame rather than any state this scenario reaches, since the harness always builds at -O0 -g and a control block reached through a live pointer always has an address.
| if control_block.GetValueAsUnsigned(0) == 0: | ||
| return f"{type_name} empty" |
There was a problem hiding this comment.
We should still print the information in the same format as normal. So type_name={type_name} use_count=0, resource=nullptr
There was a problem hiding this comment.
Both printers now report an empty handle as cuda::mr::shared_resource<...> use_count=0, resource=nullptr.
I read "the same format as normal" as keeping the bare type name that the non-empty branch already prints, so there is no literal type_name= label in front of it. Say the word if you meant the label and I will add it to both branches in both printers.
Make the LLDB formatter a real pretty printer: a synthetic provider now sits next to the summary, and the GDB printer grew a matching children(). Both expose the owned resource as a `resource` child, the way std::shared_ptr exposes its pointer, so a handle can be expanded to reach the resource it owns. The child is the address rather than the resource itself, which keeps the ABI inline namespace and the resource's private members out of the output. LLDB's provider reports no caching, so a copy or a move is picked up on the next stop instead of being served from what LLDB already computed. Report an empty handle in the normal format, `use_count=0, resource=nullptr`, rather than as `empty`, and print `<invalid address>` when the owned resource has no load address, so the fields are always in the same places. Cover copying and moving on the same variable across two stops: the copy pair prints `copy_source` on either side of the copy, so its use count has to go 1 -> 2, and the move pair prints `move_source` going from owning to empty. Both stops also print the new handle. These replace the old shared, moved-into, and moved-from cases, which pinned the same states without showing that the debuggers refresh them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ff11c9d8-8d21-4bff-83d1-a1863aed1692
📒 Files selected for processing (6)
libcudacxx/share/libcudacxx/gdb/shared_resource.pylibcudacxx/share/libcudacxx/lldb/shared_resource.pylibcudacxx/test/debugging/shared_resource/CMakeLists.txtlibcudacxx/test/debugging/shared_resource/gdb.expectedlibcudacxx/test/debugging/shared_resource/lldb.expectedlibcudacxx/test/debugging/shared_resource/source.cu
🚧 Files skipped from review as they are similar to previous changes (3)
- libcudacxx/test/debugging/shared_resource/CMakeLists.txt
- libcudacxx/test/debugging/shared_resource/lldb.expected
- libcudacxx/share/libcudacxx/gdb/shared_resource.py
| const shared_resource_type copy_source = cuda::mr::make_shared_resource<managed_resource_type>(); | ||
| inspect_before_copy(copy_source); | ||
| const shared_resource_type copy_target = copy_source; | ||
| inspect_after_copy(copy_source); | ||
|
|
||
| // Moving exchanges the control-block pointer to null, so move_source renders | ||
| // as empty afterwards. The header documents a moved-from shared_resource only | ||
| // as "valid but unspecified", so this case pins the current implementation | ||
| // rather than a documented guarantee. | ||
| shared_resource_type move_source = cuda::mr::make_shared_resource<managed_resource_type>(); | ||
| inspect_before_move(move_source); | ||
| const shared_resource_type move_target = cuda::std::move(move_source); | ||
| inspect_after_move(move_source); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
critical: Add inspections for the copied and moved targets.
Lines 61 and 70-72 inspect only copy_source and move_source; neither copy_target nor move_target is passed to an inspection wrapper. This conflicts with libcudacxx/test/debugging/shared_resource/gdb.expected Lines 21-25 and 34-38. Add inspect_after_copy(copy_target) and inspect_after_move(move_target), or remove those expected sections.
| return False | ||
|
|
||
| def num_children(self) -> int: | ||
| return 1 if self.resource.IsValid() else 0 |
There was a problem hiding this comment.
Nit: can reuse has_children() here, or rework has_children() to reuse num_children()
| cuda::mr::shared_resource<cuda::mr::legacy_managed_memory_resource> use_count=1, resource=<address> = { | ||
| resource = <address> |
There was a problem hiding this comment.
We don't need to print resource=... in the summary now that we have it in the brackets
| (const shared_resource_type) cuda::mr::shared_resource<cuda::mr::legacy_managed_memory_resource> use_count=1, resource=<address> { | ||
| resource = <address> |
There was a problem hiding this comment.
Ditto printing resource twice
|
/ok to test 0a2020e |
3 participants
Closes #10101
Description
closes #10101
Adds GDB and LLDB pretty-printers for
cuda::mr::shared_resource, mirroring the existingmemory_resourceprinters. Each reports the public type name, the reference count read from the control block's atomic, and the address of the wrapped resource, so two handles to the same resource are distinguishable at a glance. A moved-from handle renders asempty. Both debugger entry points register the new module, and alibcudacxx_add_pretty_printer_test()scenario covers the unique, shared, aliased, moved-into, and moved-from states.Verification: both debugger tests were run for real (dev machine has no GPU) in an aarch64 Linux container — nvcc 13.0.88, gdb 15.0.50, lldb 18.1.3 — invoking
run_pretty_printer_test.pywith the argument shapelibcudacxx_add_pretty_printer_test()generates, withlibcuda.so.1stubbed (the binaries call onlycuInitandcuDeviceGetCountthroughcuGetProcAddress; no compared byte originates from the driver — the harness normalizes addresses). All four checked-in.expectedfiles matched observed output byte-for-byte, and the existingmemory_resourcetest passes identically under the same setup as a control. The one path not exercised locally is the CMake registration itself (a full configure needs lit and a GPU-runner environment), so a CI run is what proves that wiring.Checklist
docs/cccl/development/debugger_setup.rstenumerates the printer directories, not individual types, so no doc edit is needed.)This contribution was developed with AI assistance via terminalhire. The author has reviewed the change and takes responsibility for its content.