gdb pretty printers #6
Conversation
WalkthroughThis pull request introduces enhancements to the decodeless library's debugging capabilities and documentation. Key changes include the addition of Python pretty printers for Changes
Sequence DiagramsequenceDiagram
participant Developer
participant CMake
participant GDB
participant PrettyPrinter
Developer->>CMake: Configure with pretty printer option
CMake->>PrettyPrinter: Embed Python scripts
PrettyPrinter-->>CMake: Generate debug script objects
Developer->>GDB: Start debugging
GDB->>PrettyPrinter: Load pretty printer
PrettyPrinter-->>GDB: Enhanced type visualization
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(2 hunks)README.md(3 hunks)debugging/offset_ptr_pretty_printer.py(1 hunks)debugging/offset_span_pretty_printer.py(1 hunks)debugging/pretty_printer_embed.cmake(1 hunks)debugging/pretty_printer_embed.cpp(1 hunks)test/CMakeLists.txt(0 hunks)
💤 Files with no reviewable changes (1)
- test/CMakeLists.txt
🧰 Additional context used
📓 Learnings (1)
README.md (2)
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:37-39
Timestamp: 2024-11-12T07:43:50.881Z
Learning: The `offset_span::subspan()` function already ensures that `offset` is within bounds, and no additional validation is necessary.
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:64-64
Timestamp: 2024-11-12T07:43:50.881Z
Learning: In `offset_span`, the `size()` function returns `const size_type&` intentionally to allow taking the address of `m_size` for binary stability.
🪛 LanguageTool
README.md
[style] ~10-~10: Only include ‘own’ if you want to emphasize that something belongs to a person or thing or was done by a person or thing.
Context: ...fset pointer is a relative address from its own address in memory rather than an absolu...
(PRP_OWN)
[style] ~12-~12: Consider using the typographical ellipsis character here instead.
Context: ... will still work. Quite useful if you're... - Saving a data structure to disk - Be...
(ELLIPSIS)
[style] ~18-~18: A comma is missing here.
Context: ...ing the raw memory of a data structure, e.g. a low overhead deep copy A tiny offset...
(EG_NO_COMMA)
[typographical] ~82-~82: To join two clauses or introduce examples, consider using an em dash.
Context: ...pointer behaves like a regular C pointer - you can assign to it, dereference etc....
(DASH_RULE)
[typographical] ~84-~84: To join two clauses or introduce examples, consider using an em dash.
Context: ...izes to nullptr (perhaps controversial - the glm library does not zero-initiali...
(DASH_RULE)
[style] ~93-~93: Consider using a more formal/concise alternative here.
Context: ...eader-only library with no dependencies other than C++20. CMake integration is provided. Y...
(OTHER_THAN)
[style] ~126-~126: Consider using the typographical ellipsis character here instead.
Context: ... library, a config file is provided for find_package(... CONFIG ...), which trivially includes ...
(ELLIPSIS)
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ...g cmake debugging should be seamless on windows without any setup and near-seamless on ...
(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (see auto-load safe-path below). Without debug visua...
(EN_COMPOUNDS_AUTO_LOAD)
[uncategorized] ~143-~143: Possible missing comma found.
Context: ...-load safe-pathbelow). Without debug visualizers you'd just seem_offset` in bytes when...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~145-~145: Consider using the typographical ellipsis character here instead.
Context: ...char*)&offsetPtr + offsetPtr.m_offset)` ... lol. Instead, it'd be nicer if a debugg...
(ELLIPSIS)
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...
(EN_COMPOUNDS_AUTO_LOAD)
🪛 markdownlint-cli2 (0.17.2)
README.md
26-26: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
97-97: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
124-124: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
150-150: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
156-156: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
🔇 Additional comments (2)
debugging/offset_span_pretty_printer.py (1)
26-29: Verify the special case of offset == 1.For
offset == 1, the code resets bothbaseandoffsetto zero. This strongly depends on the upstream logic guaranteeing that an offset of 1 is a sentinel scenario. Verify that negative offsets or other edge cases won't appear. Otherwise, consider adding explicit checks for out-of-range offsets or documenting why 1 signifies a null-equivalent pointer.debugging/offset_ptr_pretty_printer.py (1)
28-36: LGTM!The lookup function correctly matches the type pattern and returns the appropriate printer instance.
| function(decodeless_embed_pretty_printers target) | ||
| foreach(source IN LISTS ARGN) | ||
| # Generate a *.o filename | ||
| get_filename_component(source_name "${source}" NAME_WLE) | ||
| set(object "${CMAKE_CURRENT_BINARY_DIR}/${source_name}.o") | ||
|
|
||
| # Create a .bin file with a .debug_gdb_scripts section and generate a .o file from it | ||
| add_custom_command( | ||
| OUTPUT "${object}" | ||
| COMMAND | ||
| "$<TARGET_FILE:decodeless_pretty_printer_embed>" "${source}" "${object}.bin" && | ||
| ${CMAKE_OBJCOPY} -I binary -O elf64-x86-64 --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}" | ||
| DEPENDS decodeless_pretty_printer_embed "${source}" | ||
| COMMENT "Embedding GDB pretty-printer ${source}") | ||
|
|
||
| # Add the object file as a target dependency | ||
| # TODO: avoid the custom target dance | ||
| target_link_libraries(${target} INTERFACE "${object}") | ||
| add_custom_target("generate_${source_name}" DEPENDS "${object}") | ||
| add_dependencies(${target} "generate_${source_name}") | ||
| endforeach() | ||
| endfunction() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider potential portability on non-ELF platforms and streamline custom target usage.
The approach of invoking objcopy with -I binary -O elf64-x86-64 and then renaming .data to .debug_gdb_scripts may not be portable to non-ELF platforms (e.g., Windows). Additionally, the TODO comment indicates there's an interest in avoiding the extra custom targets. Investigate dynamically generating these object files in a more cross-platform manner and simplifying or automating the custom target logic.
There was a problem hiding this comment.
objcopy is already gnu binutils thing. I.e. we're already in the linux part of the cross platform support and this file is not used with MSVC. I already dislike the seemingly architecture dependent nature of this command, but again I haven't found an alternative way to add to an ELF section without specifying the architecture. Maybe by getting gcc or ld to first create an empty .o with the existing architecture but that's getting really long winded for something that should be simple.
Yep. If you could suggest a way around the custom_target that works I'll totally add it, but I've spent a long time on this already and have come up empty.
There was a problem hiding this comment.
@pknowles Thank you for the detailed explanation. I understand now that this is specifically for Linux/GNU toolchain support, and the architecture-specific nature is an inherent limitation of the ELF tooling. I agree that alternatives would likely introduce more complexity. I'll withdraw my concern about portability, and I appreciate you considering the custom target suggestion despite the current limitations.
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:6-27
Timestamp: 2025-02-01T01:52:50.022Z
Learning: The pretty printer embedding in `debugging/pretty_printer_embed.cmake` is specifically for Linux/GNU toolchain support and is not used with MSVC. The architecture-specific nature of the objcopy command is a known limitation when working with ELF sections.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Adding the necessary headers to the .debug_gdb_scripts section is not easy | ||
| # with cmake. Much nicer to build a little C++ executable to do the generation | ||
| # with no other dependencies. | ||
| add_executable(decodeless_pretty_printer_embed "${CMAKE_CURRENT_LIST_DIR}/pretty_printer_embed.cpp") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Provide an option to disable adding this executable if not needed.
Some projects may not require pretty-printer embedding on certain platforms or build configurations. Consider wrapping this target in an option guard to reduce build overhead when no GDB integration is needed.
There was a problem hiding this comment.
Already done - this file doesn't get included if the option is disabled.
There was a problem hiding this comment.
@pknowles I apologize for the oversight. You're absolutely right - I can see the option DECODELESS_EMBED_PRETTY_PRINTERS and the conditional inclusion in the main CMakeLists.txt.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and display the relevant parts of CMakeLists.txt that show the option and inclusion
rg -A 5 -B 5 "pretty_printer" CMakeLists.txt
Length of output: 605
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:29-32
Timestamp: 2025-02-01T01:53:30.925Z
Learning: The pretty printer embedding functionality in offset_ptr is controlled by the CMake option DECODELESS_EMBED_PRETTY_PRINTERS (default: ON) and is only available on non-MSVC platforms.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| offset_span_lookup_pattern = re.compile("^decodeless::offset_span<.*>$") | ||
| def offset_span_lookup(val): | ||
| global offset_span_lookup_pattern | ||
| lookup_tag = val.type.strip_typedefs().unqualified().tag | ||
| if lookup_tag is None: | ||
| return None | ||
| if offset_span_lookup_pattern.match(lookup_tag): | ||
| return OffsetSpanPrinter(val) | ||
| return None |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure broad coverage of type detection.
offset_span_lookup_pattern matches ^decodeless::offset_span<.*>$. For template specializations with nested namespaces or extra qualifiers, this pattern may fail. Consider expanding or refining the pattern if you expect more complex usage.
There was a problem hiding this comment.
Qualifiers are covered by val.type.strip_typedefs().unqualified().tag. Yes, I too am unhappy that the pretty printer docs don't discuss templates or a way to be explicit about the captured type. It would be nice to remove "decodeless" so it's not always relative to the global namespace, but how? It doesn't trivially work without as is. If you have a specific suggestion, please say.
There was a problem hiding this comment.
@pknowles After looking at libstdc++'s pretty printer implementations, I can confirm that your current approach is actually following best practices. The libstdc++ implementation also uses explicit namespaces in their patterns and relies on strip_typedefs().unqualified().tag to handle qualifiers, just like your implementation.
The explicit namespace in the pattern is actually beneficial because:
- It prevents false matches with types from other namespaces that might have the same name
- It maintains clarity about which types the printer handles
- It's consistent with how standard library implementations handle their pretty printers
/**
(__/)
(='.'=)
(")_(")
Thoughtful bunny appreciates the discussion
**/
🔗 Analysis chain
@pknowles Thank you for the clarification. After investigating other GDB pretty printers, I agree that the current approach is the most reliable. While not ideal, keeping the explicit namespace is safer than attempting namespace-agnostic matching which could lead to false positives or missed matches.
/**
(__/)
(='.'=)
(")_(")
Thoughtful bunny is taking notes
**/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GDB pretty printer regex patterns in popular C++ projects
rg -A 5 'lookup_pattern.*=.*re\.compile' --type py
Length of output: 1021
Script:
#!/bin/bash
# Clone and search some popular C++ projects known to have GDB pretty printers
git clone --depth 1 https://github.com/llvm/llvm-project
git clone --depth 1 https://github.com/gcc-mirror/gcc
git clone --depth 1 https://github.com/facebook/folly
echo "=== LLVM Pretty Printers ==="
rg -A 5 'lookup_pattern.*=.*re\.compile' --type py llvm-project
echo "=== GCC Pretty Printers ==="
rg -A 5 'lookup_pattern.*=.*re\.compile' --type py gcc
echo "=== Folly Pretty Printers ==="
rg -A 5 'lookup_pattern.*=.*re\.compile' --type py folly
Length of output: 3797
Script:
#!/bin/bash
# Search in LLDB's Python directory for pretty printer patterns
echo "=== LLDB Pretty Printers ==="
rg -A 5 'lookup_pattern.*=.*re\.compile' llvm-project/lldb/examples/synthetic --type py
rg -A 5 'lookup_pattern.*=.*re\.compile' llvm-project/lldb/scripts --type py
# Search in GCC's Python directory for pretty printer patterns
echo "=== GCC Pretty Printers ==="
rg -A 5 'lookup_pattern.*=.*re\.compile' gcc/libstdc++-v3/python --type py
# Let's also look for alternative pattern matching approaches
echo "=== Alternative Pattern Matching ==="
rg "def lookup_type" llvm-project/lldb/scripts --type py
rg "def lookup_type" gcc/libstdc++-v3/python --type py
Length of output: 704
Script:
#!/bin/bash
# Clone libstdc++ and search for pretty printer patterns
git clone --depth 1 https://gcc.gnu.org/git/gcc.git libstdcxx
echo "=== LibStdC++ Pretty Printers ==="
find libstdcxx -name "*.py" -type f -exec grep -l "lookup" {} \;
echo "=== Pattern Examples ==="
for f in $(find libstdcxx -name "*.py" -type f); do
if grep -l "lookup" "$f" > /dev/null; then
echo "=== $f ==="
cat "$f" | grep -A 5 "class.*Printer"
fi
done
Length of output: 31553
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.285Z
Learning: GDB pretty printers for C++ templates should keep explicit namespaces in their patterns (e.g., "decodeless::offset_ptr<.*>") to prevent false matches, while using `strip_typedefs().unqualified().tag` to handle qualifiers.
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.285Z
Learning: In GDB pretty printers for C++ templates, explicit namespace matching (e.g., "decodeless::offset_span<.*>") is required for reliable type detection, even though qualifiers are handled by `strip_typedefs().unqualified().tag`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
debugging/pretty_printer_embed.cpp
Outdated
| if(argc != 3) | ||
| return EXIT_FAILURE; | ||
| std::string name = std::filesystem::path(argv[1]).filename().string(); | ||
| std::ifstream src(argv[1]); | ||
| std::ofstream dst(argv[2], std::ios::binary); | ||
| dst.write("\x04", 1); // "python text" identifier byte | ||
| dst << name << "\n"; // first line is the name of the script | ||
| dst << src.rdbuf(); // inline the script | ||
| dst.write("\0", 1); // null terminated | ||
| return EXIT_SUCCESS; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file I/O failures.
While the argument count check is good, consider verifying file open success and providing user-friendly error messages. Failing to open either the source or output file currently goes unchecked, which can silently produce a corrupted or empty .bin file.
if(argc != 3)
return EXIT_FAILURE;
+// Check if the input file can be opened
std::ifstream src(argv[1]);
+if (!src.is_open()) {
+ std::cerr << "Error: Could not open source file: " << argv[1] << std::endl;
+ return EXIT_FAILURE;
+}
std::ofstream dst(argv[2], std::ios::binary);
+if (!dst.is_open()) {
+ std::cerr << "Error: Could not open output file: " << argv[2] << std::endl;
+ return EXIT_FAILURE;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(argc != 3) | |
| return EXIT_FAILURE; | |
| std::string name = std::filesystem::path(argv[1]).filename().string(); | |
| std::ifstream src(argv[1]); | |
| std::ofstream dst(argv[2], std::ios::binary); | |
| dst.write("\x04", 1); // "python text" identifier byte | |
| dst << name << "\n"; // first line is the name of the script | |
| dst << src.rdbuf(); // inline the script | |
| dst.write("\0", 1); // null terminated | |
| return EXIT_SUCCESS; | |
| } | |
| if(argc != 3) | |
| return EXIT_FAILURE; | |
| std::string name = std::filesystem::path(argv[1]).filename().string(); | |
| // Check if the input file can be opened | |
| std::ifstream src(argv[1]); | |
| if (!src.is_open()) { | |
| std::cerr << "Error: Could not open source file: " << argv[1] << std::endl; | |
| return EXIT_FAILURE; | |
| } | |
| std::ofstream dst(argv[2], std::ios::binary); | |
| if (!dst.is_open()) { | |
| std::cerr << "Error: Could not open output file: " << argv[2] << std::endl; | |
| return EXIT_FAILURE; | |
| } | |
| dst.write("\x04", 1); // "python text" identifier byte | |
| dst << name << "\n"; // first line is the name of the script | |
| dst << src.rdbuf(); // inline the script | |
| dst.write("\0", 1); // null terminated | |
| return EXIT_SUCCESS; | |
| } |
| """Pretty printer for decodeless::offset_ptr<T>""" | ||
| def __init__(self, val): | ||
| self.val = val | ||
| #base_type = self.val.type.strip_typedefs().unqualified() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented code.
The commented line appears to be unused and can be safely removed.
- #base_type = self.val.type.strip_typedefs().unqualified()| def display_hint(self): | ||
| return 'array' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider returning 'pointer' instead of 'array'.
The display_hint method returns 'array', but since this is a pointer type, returning 'pointer' would be more appropriate for GDB's display formatting.
- return 'array'
+ return 'pointer'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def display_hint(self): | |
| return 'array' | |
| def display_hint(self): | |
| - return 'array' | |
| + return 'pointer' |
There was a problem hiding this comment.
'pointer' is not a thing: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Pretty-Printing-API.html#Pretty-Printing-API
There was a problem hiding this comment.
@pknowles I apologize for my incorrect suggestion. You are absolutely right - 'pointer' is not a valid display hint. The current implementation using 'array' is correct according to GDB's documentation. Thank you for the correction.
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_ptr_pretty_printer.py:25-26
Timestamp: 2025-02-01T02:03:25.089Z
Learning: GDB pretty printers only support 'string', 'array', 'map' display hints, or None, as documented in https://sourceware.org/gdb/current/onlinedocs/gdb.html/Pretty-Printing-API.html#Pretty-Printing-API. 'pointer' is not a valid display hint.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| option(DECODELESS_EMBED_PRETTY_PRINTERS | ||
| "Embed python pretty printers in .debug_gdb_scripts sections" ON) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation for the CMake option.
Consider adding a more detailed description for the DECODELESS_EMBED_PRETTY_PRINTERS option to help users understand its purpose and impact.
option(DECODELESS_EMBED_PRETTY_PRINTERS
- "Embed python pretty printers in .debug_gdb_scripts sections" ON)
+ "Embed python pretty printers in .debug_gdb_scripts sections for enhanced GDB debugging experience. \
+ When enabled, the pretty printers will be embedded in the object files and automatically loaded by GDB." ON)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| option(DECODELESS_EMBED_PRETTY_PRINTERS | |
| "Embed python pretty printers in .debug_gdb_scripts sections" ON) | |
| option(DECODELESS_EMBED_PRETTY_PRINTERS | |
| "Embed python pretty printers in .debug_gdb_scripts sections for enhanced GDB debugging experience. \ | |
| When enabled, the pretty printers will be embedded in the object files and automatically loaded by GDB." ON) |
README.md
Outdated
| If you're using cmake debugging should be seamless on windows without any setup | ||
| and near-seamless on linux (see `auto-load safe-path` below). |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix capitalization and compound words.
Improve readability with proper capitalization and compound words:
-If you're using cmake debugging should be seamless on windows without any setup
-and near-seamless on linux (see `auto-load safe-path` below).
+If you're using CMake, debugging should be seamless on Windows without any setup
+and near-seamless on Linux (see `autoload safe-path` below).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If you're using cmake debugging should be seamless on windows without any setup | |
| and near-seamless on linux (see `auto-load safe-path` below). | |
| If you're using CMake, debugging should be seamless on Windows without any setup | |
| and near-seamless on Linux (see `autoload safe-path` below). |
🧰 Tools
🪛 LanguageTool
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ...g cmake debugging should be seamless on windows without any setup and near-seamless on ...
(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (see auto-load safe-path below). Without debug visua...
(EN_COMPOUNDS_AUTO_LOAD)
| You may need to set an [auto-load | ||
| safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) | ||
| before gdb will read the inlined scripts. Add this to `~/.gdbinit`: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix compound word and improve sentence structure.
Improve readability:
-You may need to set an [auto-load
-safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html)
-before gdb will read the inlined scripts. Add this to `~/.gdbinit`:
+You may need to set an [autoload
+safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html)
+before GDB will read the inlined scripts. Add this to `~/.gdbinit`:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| You may need to set an [auto-load | |
| safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) | |
| before gdb will read the inlined scripts. Add this to `~/.gdbinit`: | |
| You may need to set an [autoload | |
| safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) | |
| before GDB will read the inlined scripts. Add this to `~/.gdbinit`: |
🧰 Tools
🪛 LanguageTool
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...
(EN_COMPOUNDS_AUTO_LOAD)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(2 hunks)README.md(3 hunks)debugging/offset_ptr_pretty_printer.py(1 hunks)debugging/offset_span_pretty_printer.py(1 hunks)debugging/pretty_printer_embed.cmake(1 hunks)debugging/pretty_printer_embed.cpp(1 hunks)test/CMakeLists.txt(0 hunks)
💤 Files with no reviewable changes (1)
- test/CMakeLists.txt
🧰 Additional context used
📓 Learnings (4)
debugging/offset_span_pretty_printer.py (1)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.408Z
Learning: GDB pretty printers for C++ templates should keep explicit namespaces in their patterns (e.g., "decodeless::offset_ptr<.*>") to prevent false matches, while using `strip_typedefs().unqualified().tag` to handle qualifiers.
README.md (2)
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:37-39
Timestamp: 2024-11-12T07:43:50.881Z
Learning: The `offset_span::subspan()` function already ensures that `offset` is within bounds, and no additional validation is necessary.
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:64-64
Timestamp: 2024-11-12T07:43:50.881Z
Learning: In `offset_span`, the `size()` function returns `const size_type&` intentionally to allow taking the address of `m_size` for binary stability.
debugging/pretty_printer_embed.cmake (1)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:29-32
Timestamp: 2025-02-01T01:53:31.064Z
Learning: The pretty printer embedding functionality in offset_ptr is controlled by the CMake option DECODELESS_EMBED_PRETTY_PRINTERS (default: ON) and is only available on non-MSVC platforms.
debugging/offset_ptr_pretty_printer.py (1)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_ptr_pretty_printer.py:25-26
Timestamp: 2025-02-01T02:03:25.205Z
Learning: GDB pretty printers only support 'string', 'array', 'map' display hints, or None, as documented in https://sourceware.org/gdb/current/onlinedocs/gdb.html/Pretty-Printing-API.html#Pretty-Printing-API. 'pointer' is not a valid display hint.
🪛 LanguageTool
README.md
[style] ~10-~10: Only include ‘own’ if you want to emphasize that something belongs to a person or thing or was done by a person or thing.
Context: ...fset pointer is a relative address from its own address in memory rather than an absolu...
(PRP_OWN)
[style] ~12-~12: Consider using the typographical ellipsis character here instead.
Context: ... will still work. Quite useful if you're... - Saving a data structure to disk - Be...
(ELLIPSIS)
[style] ~18-~18: A comma is missing here.
Context: ...ing the raw memory of a data structure, e.g. a low overhead deep copy A tiny offset...
(EG_NO_COMMA)
[typographical] ~82-~82: To join two clauses or introduce examples, consider using an em dash.
Context: ...pointer behaves like a regular C pointer - you can assign to it, dereference etc....
(DASH_RULE)
[typographical] ~84-~84: To join two clauses or introduce examples, consider using an em dash.
Context: ...izes to nullptr (perhaps controversial - the glm library does not zero-initiali...
(DASH_RULE)
[style] ~93-~93: Consider using a more formal/concise alternative here.
Context: ...eader-only library with no dependencies other than C++20. CMake integration is provided. Y...
(OTHER_THAN)
[style] ~126-~126: Consider using the typographical ellipsis character here instead.
Context: ... library, a config file is provided for find_package(... CONFIG ...), which trivially includes ...
(ELLIPSIS)
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ... CMake, debugging should be seamless on windows without any setup and near-seamless on ...
(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (see auto-load safe-path below). Without debug visua...
(EN_COMPOUNDS_AUTO_LOAD)
[uncategorized] ~143-~143: Possible missing comma found.
Context: ...-load safe-pathbelow). Without debug visualizers you'd just seem_offset` in bytes when...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~145-~145: Consider using the typographical ellipsis character here instead.
Context: ...char*)&offsetPtr + offsetPtr.m_offset)` ... lol. Instead, it'd be nicer if a debugg...
(ELLIPSIS)
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...
(EN_COMPOUNDS_AUTO_LOAD)
🪛 markdownlint-cli2 (0.17.2)
README.md
26-26: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
97-97: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
124-124: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
150-150: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
156-156: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
🔇 Additional comments (18)
debugging/offset_ptr_pretty_printer.py (5)
1-3: Imports look correct and minimal.
They provide the essential GDB and regex functionality for a pretty printer without unnecessary overhead.
19-21: Ensure pointer validity before dereferencing.
If offset is invalid or very large, dereferencing could be undefined behavior. Although this is only for debugging, you may want to safeguard against invalid offsets or at least add a comment clarifying the assumption that offset is valid.
25-26: Returning 'array' is correct.
Since GDB does not accept 'pointer' as a valid display hint, using 'array' complies with GDB’s Pretty-Printing API.
28-38: Global registration of pretty printer looks good.
Appending to gdb.pretty_printers ensures the offset pointer printer is recognized automatically. No issues found.
8-8: 🧹 Nitpick (assertive)Remove or justify commented-out code.
The commented line “base_type = …” might be confusing to future maintainers. If it’s no longer needed, please remove it; otherwise, add a comment clarifying its purpose.- #base_type = self.val.type.strip_typedefs().unqualified() + # Remove the line or add a clarifying commentLikely invalid or redundant comment.
debugging/pretty_printer_embed.cpp (3)
15-19: Argument check is straightforward.
Throwing an error and exiting is reasonable if the argument count is incorrect.
21-25: Source file checks are valid.
Failing fast when unable to open the source file prevents silent data corruption.
27-31: Destination file checks are valid.
Same reasoning as above. The additional error message is helpful.debugging/pretty_printer_embed.cmake (2)
12-19: Portability was previously discussed.
Using objcopy with ELF-specific flags is acceptable for your supported platforms. This mirrors existing limitations noted in previous comments.
29-32: Build approach is well-organized.
Creating a small dedicated executable is a clean solution for generating the .debug_gdb_scripts section data.debugging/offset_span_pretty_printer.py (5)
4-15: LGTM! Well-designed lazy loading implementation.The
LazyChildrenclass provides an efficient array-like interface with proper bounds checking.
19-29: LGTM! Robust pointer handling.The constructor correctly handles:
- Template type extraction
- Special case for nullptr (offset=1)
- Proper pointer arithmetic with type casting
31-35: LGTM! Clean array element access.Efficient implementation using lazy loading for child elements.
37-44: LGTM! Clear display formatting.The display methods provide a clean representation of the span type and contents.
46-54: LGTM! Robust type matching implementation.The lookup function follows best practices:
- Explicit namespace prevents false matches
- Proper handling of type qualifiers
- Safe null checks
CMakeLists.txt (1)
20-21: 🧹 Nitpick (assertive)Add more descriptive help text for the CMake option.
Consider expanding the option description to better explain its purpose and impact:
option(DECODELESS_EMBED_PRETTY_PRINTERS - "Embed python pretty printers in .debug_gdb_scripts sections" ON) + "Embed Python pretty printers in .debug_gdb_scripts sections for enhanced GDB debugging. \ +When enabled, pretty printers will be automatically loaded by GDB to improve display of offset_ptr and offset_span types." ON)⛔ Skipped due to learnings
Learnt from: pknowles PR: decodeless/offset_ptr#6 File: debugging/pretty_printer_embed.cmake:29-32 Timestamp: 2025-02-01T01:53:31.064Z Learning: The pretty printer embedding functionality in offset_ptr is controlled by the CMake option DECODELESS_EMBED_PRETTY_PRINTERS (default: ON) and is only available on non-MSVC platforms.README.md (2)
140-141: 🧹 Nitpick (assertive)Fix capitalization and compound words.
Improve consistency in technical terms:
-If you're using CMake, debugging should be seamless on windows without any setup -and near-seamless on linux (see `auto-load safe-path` below). +If you're using CMake, debugging should be seamless on Windows without any setup +and near-seamless on Linux (see `autoload safe-path` below).Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ... CMake, debugging should be seamless on windows without any setup and near-seamless on ...(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (seeauto-load safe-pathbelow). Without debug visua...(EN_COMPOUNDS_AUTO_LOAD)
162-164: 🧹 Nitpick (assertive)Fix compound word and improve sentence structure.
Improve readability:
-You may need to set an [auto-load -safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) -before gdb will read the inlined scripts. Add this to `~/.gdbinit`: +You may need to set an [autoload +safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) +before GDB will read the inlined scripts. Add this to `~/.gdbinit`:Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...(EN_COMPOUNDS_AUTO_LOAD)
| if offset == 1: | ||
| base = gdb.Value(0) | ||
| offset = gdb.Value(0) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify the special case for offset == 1.
It's not immediately obvious why offset==1 represents a NULL-like pointer. Adding a short comment would help maintainers understand the rationale.
| std::string name = std::filesystem::path(argv[1]).filename().string(); | ||
| dst.write("\x04", 1); // "python text" identifier byte | ||
| dst << name << "\n"; // first line is the name of the script | ||
| dst << src.rdbuf(); // inline the script | ||
| dst.write("\0", 1); // null terminated |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider validating write success.
If writing fails (e.g., filesystem is full), the program won’t catch it. Checking for dst.fail() or adding more robust error handling may prevent incomplete binary files.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(2 hunks)README.md(3 hunks)debugging/offset_ptr_pretty_printer.py(1 hunks)debugging/offset_span_pretty_printer.py(1 hunks)debugging/pretty_printer_embed.cmake(1 hunks)debugging/pretty_printer_embed.cpp(1 hunks)test/CMakeLists.txt(0 hunks)
💤 Files with no reviewable changes (1)
- test/CMakeLists.txt
🧰 Additional context used
📓 Learnings (4)
README.md (2)
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:37-39
Timestamp: 2024-11-12T07:43:50.881Z
Learning: The `offset_span::subspan()` function already ensures that `offset` is within bounds, and no additional validation is necessary.
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:64-64
Timestamp: 2024-11-12T07:43:50.881Z
Learning: In `offset_span`, the `size()` function returns `const size_type&` intentionally to allow taking the address of `m_size` for binary stability.
debugging/pretty_printer_embed.cmake (1)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:29-32
Timestamp: 2025-02-01T01:53:31.064Z
Learning: The pretty printer embedding functionality in offset_ptr is controlled by the CMake option DECODELESS_EMBED_PRETTY_PRINTERS (default: ON) and is only available on non-MSVC platforms.
debugging/offset_span_pretty_printer.py (1)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.408Z
Learning: GDB pretty printers for C++ templates should keep explicit namespaces in their patterns (e.g., "decodeless::offset_ptr<.*>") to prevent false matches, while using `strip_typedefs().unqualified().tag` to handle qualifiers.
debugging/offset_ptr_pretty_printer.py (2)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.408Z
Learning: GDB pretty printers for C++ templates should keep explicit namespaces in their patterns (e.g., "decodeless::offset_ptr<.*>") to prevent false matches, while using `strip_typedefs().unqualified().tag` to handle qualifiers.
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_ptr_pretty_printer.py:25-26
Timestamp: 2025-02-01T02:03:25.205Z
Learning: GDB pretty printers only support 'string', 'array', 'map' display hints, or None, as documented in https://sourceware.org/gdb/current/onlinedocs/gdb.html/Pretty-Printing-API.html#Pretty-Printing-API. 'pointer' is not a valid display hint.
🪛 LanguageTool
README.md
[style] ~10-~10: Only include ‘own’ if you want to emphasize that something belongs to a person or thing or was done by a person or thing.
Context: ...fset pointer is a relative address from its own address in memory rather than an absolu...
(PRP_OWN)
[uncategorized] ~12-~12: Possible missing comma found.
Context: ...apped or copied within the same address range it will still work. Quite useful if you...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~12-~12: Consider using the typographical ellipsis character here instead.
Context: ... will still work. Quite useful if you're... - Saving a data structure to disk - Be...
(ELLIPSIS)
[style] ~18-~18: A comma is missing here.
Context: ...ing the raw memory of a data structure, e.g. a low overhead deep copy A tiny offset...
(EG_NO_COMMA)
[typographical] ~82-~82: To join two clauses or introduce examples, consider using an em dash.
Context: ...pointer behaves like a regular C pointer - you can assign to it, dereference etc....
(DASH_RULE)
[typographical] ~84-~84: To join two clauses or introduce examples, consider using an em dash.
Context: ...izes to nullptr (perhaps controversial - the glm library does not zero-initiali...
(DASH_RULE)
[style] ~93-~93: Consider using a more formal/concise alternative here.
Context: ...eader-only library with no dependencies other than C++20. CMake integration is provided. Y...
(OTHER_THAN)
[style] ~126-~126: Consider using the typographical ellipsis character here instead.
Context: ... library, a config file is provided for find_package(... CONFIG ...), which trivially includes ...
(ELLIPSIS)
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ... CMake, debugging should be seamless on windows without any setup and near-seamless on ...
(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (see auto-load safe-path below). Without debug visua...
(EN_COMPOUNDS_AUTO_LOAD)
[uncategorized] ~143-~143: Possible missing comma found.
Context: ...-load safe-pathbelow). Without debug visualizers you'd just seem_offset` in bytes when...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~145-~145: Consider using the typographical ellipsis character here instead.
Context: ...char*)&offsetPtr + offsetPtr.m_offset)` ... lol. Instead, it'd be nicer if a debugg...
(ELLIPSIS)
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...
(EN_COMPOUNDS_AUTO_LOAD)
🪛 markdownlint-cli2 (0.17.2)
README.md
26-26: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
97-97: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
124-124: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
150-150: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
156-156: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🔇 Additional comments (10)
debugging/offset_ptr_pretty_printer.py (1)
32-32: Namespace pattern usage looks correct.
Using an explicit namespace pattern for offset_ptr is aligned with best practices and prevents false matches.debugging/pretty_printer_embed.cpp (1)
35-37: Validate write success to avoid partial or corrupted output.
If the filesystem is full or the write fails, the code won't detect it. This has been flagged previously.debugging/pretty_printer_embed.cmake (4)
32-32: LGTM!The executable definition is clean and minimal.
17-17: Consider architecture-specific objcopy flags.The
-O elf64-x86-64flag is hardcoded, which might not work on other architectures like ARM64.
29-32: LGTM!Good choice to use a C++ executable for header generation. The comment clearly explains the rationale.
16-18: Consider making the architecture specification configurable.The hardcoded
-O elf64-x86-64might not work on all Linux architectures (e.g., ARM64).Consider using a variable to specify the target architecture:
- ${CMAKE_OBJCOPY} -I binary -O elf64-x86-64 --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}" + ${CMAKE_OBJCOPY} -I binary -O ${DECODELESS_ELF_FORMAT} --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}"CMakeLists.txt (1)
12-29: LGTM!The changes are well-structured with:
- Clear separation between MSVC and non-MSVC environments
- Proper conditional handling of pretty printer embedding
- Correct usage of CMake commands and options
README.md (3)
93-136: LGTM! Well-structured CMake integration documentation.The CMake integration section is comprehensive and well-organized, covering all major integration methods with clear examples.
🧰 Tools
🪛 LanguageTool
[style] ~93-~93: Consider using a more formal/concise alternative here.
Context: ...eader-only library with no dependencies other than C++20. CMake integration is provided. Y...(OTHER_THAN)
[style] ~126-~126: Consider using the typographical ellipsis character here instead.
Context: ... library, a config file is provided forfind_package(... CONFIG ...), which trivially includes ...(ELLIPSIS)
🪛 markdownlint-cli2 (0.17.2)
97-97: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
124-124: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
162-164: 🧹 Nitpick (assertive)Fix compound word and improve sentence structure.
For consistency:
-You may need to set an [auto-load -safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) -before gdb will read the inlined scripts. Add this to `~/.gdbinit`: +You may need to set an [autoload +safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) +before GDB will read the inlined scripts. Add this to `~/.gdbinit`:Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...(EN_COMPOUNDS_AUTO_LOAD)
140-141: 🧹 Nitpick (assertive)Fix capitalization and compound words.
For consistency and correctness:
-If you're using CMake, debugging should be seamless on windows without any setup -and near-seamless on linux (see `auto-load safe-path` below). +If you're using CMake, debugging should be seamless on Windows without any setup +and near-seamless on Linux (see `autoload safe-path` below).Likely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ... CMake, debugging should be seamless on windows without any setup and near-seamless on ...(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (seeauto-load safe-pathbelow). Without debug visua...(EN_COMPOUNDS_AUTO_LOAD)
| # TODO: avoid the custom target dance | ||
| target_link_libraries(${target} INTERFACE "${object}") | ||
| add_custom_target("generate_${source_name}" DEPENDS "${object}") | ||
| add_dependencies(${target} "generate_${source_name}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using object library to avoid custom targets.
The TODO comment suggests finding an alternative to custom targets. Consider using an OBJECT library type instead:
- target_link_libraries(${target} INTERFACE "${object}")
- add_custom_target("generate_${source_name}" DEPENDS "${object}")
- add_dependencies(${target} "generate_${source_name}")
+ add_library("${source_name}_obj" OBJECT IMPORTED)
+ set_property(TARGET "${source_name}_obj" PROPERTY IMPORTED_OBJECTS "${object}")
+ target_link_libraries(${target} INTERFACE "${source_name}_obj")There was a problem hiding this comment.
I investigated OBJECT libraries and couldn't get around the need for extra targets. One day I might return to this.
There was a problem hiding this comment.
@pknowles Understood, thanks for investigating that approach. I'll retract my suggestion about using OBJECT libraries.
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:22-25
Timestamp: 2025-02-07T22:33:13.665Z
Learning: Using OBJECT libraries in CMake for embedding GDB pretty printers still requires extra targets, making it not significantly better than using custom targets directly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Function to embed a python pretty printer in an object (.o) file and add it as | ||
| # a dependency of the given target. See https://www.heuristic42.com/blog/64/. | ||
| # Usage: decodeless_embed_pretty_printer(MyTarget printer_a.py printer_b.py) | ||
| function(decodeless_embed_pretty_printers target) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance documentation with platform requirements.
The documentation is clear but could be more helpful by mentioning:
- Platform limitations (Linux/GNU toolchain only)
- Required tools (objcopy)
- CMake option that controls this feature
# Function to embed a python pretty printer in an object (.o) file and add it as
# a dependency of the given target. See https://www.heuristic42.com/blog/64/.
+# Note: This functionality is only available on Linux/GNU platforms and requires
+# objcopy from binutils. It is controlled by DECODELESS_EMBED_PRETTY_PRINTERS option.
# Usage: decodeless_embed_pretty_printer(MyTarget printer_a.py printer_b.py)| add_custom_command( | ||
| OUTPUT "${object}" | ||
| COMMAND | ||
| "$<TARGET_FILE:decodeless_pretty_printer_embed>" "${source}" "${object}.bin" && | ||
| ${CMAKE_OBJCOPY} -I binary -O elf64-x86-64 --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}" | ||
| DEPENDS decodeless_pretty_printer_embed "${source}" | ||
| COMMENT "Embedding GDB pretty-printer ${source}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error handling for missing tools and failed commands.
The custom command should fail gracefully if objcopy is not available or if the commands fail.
+find_program(CMAKE_OBJCOPY objcopy)
+if(NOT CMAKE_OBJCOPY)
+ message(WARNING "objcopy not found. GDB pretty printers will not be embedded.")
+ return()
+endif()
add_custom_command(
OUTPUT "${object}"
COMMAND
"$<TARGET_FILE:decodeless_pretty_printer_embed>" "${source}" "${object}.bin" &&
${CMAKE_OBJCOPY} -I binary -O elf64-x86-64 --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}"
+ COMMAND_ERROR_IS_FATAL ANY
DEPENDS decodeless_pretty_printer_embed "${source}"
COMMENT "Embedding GDB pretty-printer ${source}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| add_custom_command( | |
| OUTPUT "${object}" | |
| COMMAND | |
| "$<TARGET_FILE:decodeless_pretty_printer_embed>" "${source}" "${object}.bin" && | |
| ${CMAKE_OBJCOPY} -I binary -O elf64-x86-64 --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}" | |
| DEPENDS decodeless_pretty_printer_embed "${source}" | |
| COMMENT "Embedding GDB pretty-printer ${source}") | |
| find_program(CMAKE_OBJCOPY objcopy) | |
| if(NOT CMAKE_OBJCOPY) | |
| message(WARNING "objcopy not found. GDB pretty printers will not be embedded.") | |
| return() | |
| endif() | |
| add_custom_command( | |
| OUTPUT "${object}" | |
| COMMAND "$<TARGET_FILE:decodeless_pretty_printer_embed>" "${source}" "${object}.bin" && | |
| ${CMAKE_OBJCOPY} -I binary -O elf64-x86-64 --rename-section .data=.debug_gdb_scripts "${object}.bin" "${object}" | |
| COMMAND_ERROR_IS_FATAL ANY | |
| DEPENDS decodeless_pretty_printer_embed "${source}" | |
| COMMENT "Embedding GDB pretty-printer ${source}") |
There was a problem hiding this comment.
I think it's safe to assume people compiling on linux will have this. && should cover failures
There was a problem hiding this comment.
@pknowles Understood, that's a fair point. The && operator will indeed handle command failures, and objcopy being a standard Linux tool makes the explicit check unnecessary.
🐰 nods thoughtfully while taking notes
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:13-19
Timestamp: 2025-02-07T22:31:28.117Z
Learning: For Linux-specific functionality in CMake, it's acceptable to assume the availability of standard binary utilities (binutils) like `objcopy`. The `&&` operator in shell commands provides sufficient error handling by ensuring subsequent commands only run if the previous command succeeds.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(2 hunks)README.md(3 hunks)debugging/offset_ptr_pretty_printer.py(1 hunks)debugging/offset_span_pretty_printer.py(1 hunks)debugging/pretty_printer_embed.cmake(1 hunks)debugging/pretty_printer_embed.cpp(1 hunks)test/CMakeLists.txt(0 hunks)
💤 Files with no reviewable changes (1)
- test/CMakeLists.txt
🧰 Additional context used
📓 Learnings (4)
debugging/offset_ptr_pretty_printer.py (2)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.408Z
Learning: GDB pretty printers for C++ templates should keep explicit namespaces in their patterns (e.g., "decodeless::offset_ptr<.*>") to prevent false matches, while using `strip_typedefs().unqualified().tag` to handle qualifiers.
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_ptr_pretty_printer.py:25-26
Timestamp: 2025-02-01T02:03:25.205Z
Learning: GDB pretty printers only support 'string', 'array', 'map' display hints, or None, as documented in https://sourceware.org/gdb/current/onlinedocs/gdb.html/Pretty-Printing-API.html#Pretty-Printing-API. 'pointer' is not a valid display hint.
README.md (2)
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:37-39
Timestamp: 2024-11-12T07:43:50.881Z
Learning: The `offset_span::subspan()` function already ensures that `offset` is within bounds, and no additional validation is necessary.
Learnt from: pknowles
PR: decodeless/offset_ptr#4
File: include/decodeless/offset_span.hpp:64-64
Timestamp: 2024-11-12T07:43:50.881Z
Learning: In `offset_span`, the `size()` function returns `const size_type&` intentionally to allow taking the address of `m_size` for binary stability.
debugging/pretty_printer_embed.cmake (3)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:22-25
Timestamp: 2025-02-07T22:33:13.834Z
Learning: Using OBJECT libraries in CMake for embedding GDB pretty printers still requires extra targets, making it not significantly better than using custom targets directly.
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:13-19
Timestamp: 2025-02-07T22:31:28.265Z
Learning: For Linux-specific functionality in CMake, it's acceptable to assume the availability of standard binary utilities (binutils) like `objcopy`. The `&&` operator in shell commands provides sufficient error handling by ensuring subsequent commands only run if the previous command succeeds.
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/pretty_printer_embed.cmake:29-32
Timestamp: 2025-02-01T01:53:31.064Z
Learning: The pretty printer embedding functionality in offset_ptr is controlled by the CMake option DECODELESS_EMBED_PRETTY_PRINTERS (default: ON) and is only available on non-MSVC platforms.
debugging/offset_span_pretty_printer.py (1)
Learnt from: pknowles
PR: decodeless/offset_ptr#6
File: debugging/offset_span_pretty_printer.py:46-54
Timestamp: 2025-02-01T02:04:21.408Z
Learning: GDB pretty printers for C++ templates should keep explicit namespaces in their patterns (e.g., "decodeless::offset_ptr<.*>") to prevent false matches, while using `strip_typedefs().unqualified().tag` to handle qualifiers.
🪛 LanguageTool
README.md
[style] ~10-~10: Only include ‘own’ if you want to emphasize that something belongs to a person or thing or was done by a person or thing.
Context: ...fset pointer is a relative address from its own address in memory rather than an absolu...
(PRP_OWN)
[uncategorized] ~12-~12: A comma might be missing here.
Context: ...apped or copied within the same address range it will still work. Quite useful if you...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~12-~12: Consider using the typographical ellipsis character here instead.
Context: ... will still work. Quite useful if you're... - Saving a data structure to disk - Be...
(ELLIPSIS)
[style] ~18-~18: A comma is missing here.
Context: ...ing the raw memory of a data structure, e.g. a low overhead deep copy A tiny offset...
(EG_NO_COMMA)
[typographical] ~82-~82: To join two clauses or introduce examples, consider using an em dash.
Context: ...pointer behaves like a regular C pointer - you can assign to it, dereference etc....
(DASH_RULE)
[typographical] ~84-~84: To join two clauses or introduce examples, consider using an em dash.
Context: ...izes to nullptr (perhaps controversial - the glm library does not zero-initiali...
(DASH_RULE)
[style] ~93-~93: Consider using a more formal/concise alternative here.
Context: ...eader-only library with no dependencies other than C++20. CMake integration is provided. Y...
(OTHER_THAN)
[style] ~126-~126: Consider using the typographical ellipsis character here instead.
Context: ... library, a config file is provided for find_package(... CONFIG ...), which trivially includes ...
(ELLIPSIS)
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ... CMake, debugging should be seamless on windows without any setup and near-seamless on ...
(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (see auto-load safe-path below). Without debug visua...
(EN_COMPOUNDS_AUTO_LOAD)
[uncategorized] ~143-~143: Possible missing comma found.
Context: ...-load safe-pathbelow). Without debug visualizers you'd just seem_offset` in bytes when...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~144-~144: A punctuation mark might be missing here.
Context: ..._ptr. You *could* add a watch or print (MyType)((char*)&offsetPtr + offsetPtr.m...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[style] ~145-~145: Consider using the typographical ellipsis character here instead.
Context: ...char*)&offsetPtr + offsetPtr.m_offset)` ... lol. Instead, it'd be nicer if a debugg...
(ELLIPSIS)
[uncategorized] ~158-~158: A punctuation mark might be missing here.
Context: ... python pretty printers are embedded in [.debug_gdb_scripts](https://www.heuris...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...
(EN_COMPOUNDS_AUTO_LOAD)
🪛 markdownlint-cli2 (0.17.2)
README.md
26-26: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
97-97: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
124-124: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
150-150: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
156-156: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
🔇 Additional comments (26)
debugging/offset_ptr_pretty_printer.py (7)
1-4: Imports appear correct.All imports look fine. Ensure that the environment where this script runs always has the "gdb" module available (e.g., within GDB's embedded Python environment).
11-19: Clarify offset==1 usage more explicitly.Although there's already a comment explaining the magic offset of '1', adding a brief rationale (e.g., "offset==1 is treated like a NULL pointer in decodeless") would further help new contributors.
20-24: Single child in children() is acceptable.Returning just one child named "operator*()" is consistent with pointer semantics. It's a valid approach for a pointer-like type.
25-26: String representation is straightforward.The to_string method provides a clear pointer-style display. No issues found.
29-29: Regex pattern is correct and sufficiently scoped.Explicitly matching "decodeless::offset_ptr<.*>$" prevents false positives. This aligns with the best practice of using explicit namespaces.
32-39: Lookup function logic is sound.The offset_ptr_lookup function properly checks the matched type. No concerns with performance or correctness here.
42-42: Appending printer is correct.Registering the pretty printer is correct. Typically, GDB won't re-append duplicates, so this is fine.
debugging/offset_span_pretty_printer.py (7)
1-4: Imports appear correct.Used modules (gdb, re) are appropriate for GDB Python pretty printers. No issues found.
5-8: Performance-conscious comments are helpful.The comment clarifies why a lazy approach is needed for large arrays. Good documentation practice.
8-20: LazyChildren class looks good.Index enforcement is robust, preventing out-of-range access. This helps GDB handle large arrays gracefully.
36-50: Child iteration is well structured.Returning lazy children, implementing len, and giving a clear to_string are all consistent with GDB’s pretty printer guidelines.
52-52: Regex matches namespace standards.Ensuring “^decodeless::offset_span<.*>$” helps prevent accidental matches with other symbols.
55-63: Lookup function is straightforward.The offset_span_lookup properly returns an OffsetSpanPrinter if the regex matches. This is consistent with best practices.
65-65: Printer registration is successful.Appending the printer is fine. GDB will use this printer for matching types.
debugging/pretty_printer_embed.cpp (4)
1-15: Documentation is clear.Comments about the .debug_gdb_scripts section and usage instructions are detailed and help future maintainers or users.
16-19: Argument count check is correct.Exiting if argc != 3 is straightforward. The user message is concise.
21-31: File opening checks are good.Using “good()” to verify input and output streams addresses basic open failures.
33-38: 🧹 Nitpick (assertive)Consider validating the write outcome.
After writing script data to the binary file, consider checking if the output stream states are still good (e.g., using dst.fail()) to catch filesystem errors.
dst << src.rdbuf(); // inline the script dst.write("\0", 1); // null terminated +if (dst.fail()) { + std::cerr << "Error: writing to output file failed.\n"; + return EXIT_FAILURE; +}Likely invalid or redundant comment.
debugging/pretty_printer_embed.cmake (3)
3-6: Enhance documentation with platform requirements.Apply this diff to improve the documentation:
# Function to embed a python pretty printer in an object (.o) file and add it as # a dependency of the given target. See https://www.heuristic42.com/blog/64/. +# Note: This functionality is only available on Linux/GNU platforms and requires +# objcopy from binutils. It is controlled by DECODELESS_EMBED_PRETTY_PRINTERS option. # Usage: decodeless_embed_pretty_printer(MyTarget printer_a.py printer_b.py)
7-19: LGTM! Implementation follows best practices.The code correctly handles file paths, dependencies, and error cases. The use of
&&ensures the command fails if any step fails.
29-32: LGTM! Well-documented implementation.The comment clearly explains why a separate executable is needed, and the implementation is correct.
CMakeLists.txt (3)
1-11: LGTM! Project setup follows best practices.The implementation correctly updates the copyright year and follows CMake best practices.
20-21: Add documentation for the CMake option.Apply this diff to improve the documentation:
option(DECODELESS_EMBED_PRETTY_PRINTERS - "Embed python pretty printers in .debug_gdb_scripts sections" ON) + "Embed python pretty printers in .debug_gdb_scripts sections for enhanced GDB debugging experience. \ + When enabled, the pretty printers will be embedded in the object files and automatically loaded by GDB." ON)
33-39: LGTM! Testing setup follows best practices.The implementation correctly guards testing with options and follows CMake best practices.
README.md (2)
140-141: Fix capitalization and compound words.Apply this diff to improve readability:
-If you're using CMake, debugging should be seamless on windows without any setup -and near-seamless on linux (see `auto-load safe-path` below). +If you're using CMake, debugging should be seamless on Windows without any setup +and near-seamless on Linux (see `autoload safe-path` below).🧰 Tools
🪛 LanguageTool
[grammar] ~140-~140: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ... CMake, debugging should be seamless on windows without any setup and near-seamless on ...(A_WINDOWS)
[misspelling] ~141-~141: This word is normally spelled as one.
Context: ...y setup and near-seamless on linux (seeauto-load safe-pathbelow). Without debug visua...(EN_COMPOUNDS_AUTO_LOAD)
162-164: Fix compound word and improve sentence structure.Apply this diff to improve readability:
-You may need to set an [auto-load -safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) -before gdb will read the inlined scripts. Add this to `~/.gdbinit`: +You may need to set an [autoload +safe-path](https://sourceware.org/gdb/current/onlinedocs/gdb.html/dotdebug_005fgdb_005fscripts-section.html) +before GDB will read the inlined scripts. Add this to `~/.gdbinit`:🧰 Tools
🪛 LanguageTool
[misspelling] ~162-~162: This word is normally spelled as one.
Context: ...g compilation. You may need to set an [auto-load safe-path](https://sourceware.org/gdb/c...(EN_COMPOUNDS_AUTO_LOAD)
| class OffsetPtrPrinter: | ||
| """Pretty printer for decodeless::offset_ptr<T>""" | ||
| def __init__(self, val): | ||
| self.val = val | ||
| self.T = self.val.type.strip_typedefs().unqualified().template_argument(0) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Class documentation is concise.
The class docstring is clear. Consider adding short docstrings for each method to aid future maintainers.
| class OffsetSpanPrinter: | ||
| """Pretty printer for decodeless::offset_span<T>""" | ||
| def __init__(self, val): | ||
| self.T = val.type.strip_typedefs().unqualified().template_argument(0) | ||
| self.length = val["m_size"] | ||
|
|
||
| ptr = val["m_data"] | ||
| base = ptr.address | ||
| offset = ptr["m_offset"] | ||
| if offset == 1: | ||
| base = gdb.Value(0) | ||
| offset = gdb.Value(0) | ||
| self.pointer = (base.cast(gdb.lookup_type("char").pointer()) + offset).cast(self.T.pointer()) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
OffsetSpanPrinter constructor is readable.
The logic of extracting type and adjusting base/offset for a null-like representation is consistent with what we saw in offset_ptr. Possibly add a short comment explaining offset==1 is treated as NULL.
| # Add the object file as a target dependency | ||
| # TODO: avoid the custom target dance | ||
| target_link_libraries(${target} INTERFACE "${object}") | ||
| add_custom_target("generate_${source_name}" DEPENDS "${object}") | ||
| add_dependencies(${target} "generate_${source_name}") | ||
| endforeach() | ||
| endfunction() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve TODO comment with investigation results.
The TODO comment could be more descriptive about the investigation results.
Apply this diff to improve the comment:
- # TODO: avoid the custom target dance
+ # TODO: avoid the custom target dance. OBJECT libraries were investigated but
+ # still required extra targets. Alternative approaches welcome.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Add the object file as a target dependency | |
| # TODO: avoid the custom target dance | |
| target_link_libraries(${target} INTERFACE "${object}") | |
| add_custom_target("generate_${source_name}" DEPENDS "${object}") | |
| add_dependencies(${target} "generate_${source_name}") | |
| endforeach() | |
| endfunction() | |
| # Add the object file as a target dependency | |
| # TODO: avoid the custom target dance. OBJECT libraries were investigated but | |
| # still required extra targets. Alternative approaches welcome. | |
| target_link_libraries(${target} INTERFACE "${object}") | |
| add_custom_target("generate_${source_name}" DEPENDS "${object}") | |
| add_dependencies(${target} "generate_${source_name}") |
Summary by CodeRabbit
Release Notes
Documentation
offset_ptrandoffset_spanNew Features
offset_ptrandoffset_span, enhancing debugging capabilitiesChores