Stateful variable-location annotations in Disassembler::PrintInstructions() (follow-up to #147460) #152887
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
…DWARFExpressionList This introduces a new API for retrieving DWARF expression metadata associated with variable location entries at a given PC address. It provides the base, end, and expression pointer for downstream consumers such as disassembler annotations. Intended for use in richer instruction annotations in Instruction::Dump().
Co-authored-by: Jonas Devlieghere <[email protected]>
Updated comment for GetExpressionEntryAtAddress to directly refer to struct DWARFExpressionEntry Co-authored-by: Jonas Devlieghere <[email protected]>
updating code style for function GetExpressionEntryAtAddress Co-authored-by: Jonas Devlieghere <[email protected]>
Replace raw base/end with `AddressRange` in `DWARFExpressionEntry` and cleans up helper comments to follow Doxygen convention. Using `AddressRange` makes the intent clearer, avoids duplication of basic `AddressRange` logic usage
Converts `GetExpressionEntryAtAddress` to return `llvm::Expected<DWARFExpressionEntry>` using the updated `DWARFExpressionEntry`. Updates the implementation to compute a single `AddressRange file_range` for each DWARF location interval.
Updated commenting style for struct DWARFExpressionEntry
Updated function `llvm::Expected<DWARFExpressionList::DWARFExpressionEntry> DWARFExpressionList::GetExpressionEntryAtAddress` to use `FindEntryThatContains` instead of `FindEntryIndexThatContains`
Co-authored-by: Adrian Prantl <[email protected]>
This patch adds explicit checks:
- ensure `load_addr >= func_load_addr` to avoid underflow,
- compute and verify a temporary delta variable, then verify `delta + m_func_file_addr` does
not exceed `addr_t` max to avoid overflow.
…bly output. Right now just checks if DW annotations show up for a basic program and that a variable location is annotated (i.e 'a = DW_OP_reg...').
- Fixed an issue where variable location annotations were not shown if the current instruction address did not exactly match the DWARF base address. Now, annotations are shown as long as the PC is within the valid range. - Improved alignment of annotation comments in Instruction::Dump(). While `FillLastLineToColumn` can sometimes overcompensate due to internal formatting or byte-width mismatches, the overall alignment is now significantly more consistent and readable.
Previously, when a DWARF expression contained any decoding error, the entire variable location annotation was printed with the error, e.g. `c = DW_OP_addr 0x0, <decoding error> 00 00 00`. This was misleading and cluttered the disassembly view. This patch improves the formatting by stripping out the `<decoding error ...>` fragments while preserving the valid portions of the expression, so that partial information like `c = DW_OP_addr 0x0` can still be shown. This allows the rich disassembler to give more meaningful variable annotations, especially in optimized (-O1/-O2) builds where partial DWARF corruption or unsupported expressions may occur.
Handled edge case where the entire DWARF expression is a `<decoding error>`, ensuring no misleading or empty annotations are printed for such variables.
This patch adds API tests to verify that DWARF variable location annotations are correctly displayed in the disassembly output. The tests cover: - Local variables in loops and functions - Multiple stack variables - Control flow edge cases - Different optimization levels (-O0, -O1, -O2) - Ensuring decoding errors are excluded from output
…try API This rebases the `add-disassembler-annotations` work onto the latest `add-dwarfexprentry-api` branch so that the instruction annotation patches sit cleanly atop the new DWARFExpressionEntry struct and helper API. All conflicts have been resolved and the annotation code now integrates with the updated std::optional<AddressRange>-based GetExpressionEntryAtAddress signature.
…w `DWARFExpression::DumpLocationWithOptions` for simplified expression printing This patch introduces a PrintRegisterOnly flag to the DIDumpOptions struct, enabling concise rendering of DWARF expressions for disassembler annotations. Key changes: - Added DumpLocationWithOptions to DWARFExpression for flexible dumping with DIDumpOptions. - Updated DWARFExpression::print and Operation::print to respect PrintRegisterOnly, rendering registers like RDI without DW_OP_ or llvm: prefixes. - Suppressed <decoding error> output when PrintRegisterOnly is true to avoid clutter during register-only disassembly output. These changes are motivated by LLDB’s rich disassembler feature, where annotations should match user-facing register names without DWARF-level detail. Test impact: Some rich-disassembler tests that relied on DW_OP_ for validation were deprecated. Updated tests aligned with the new formatting will be added next.
…n Instruction::Dump
…tate_variable` lambda function
…IDumpOptions instead of having to introduce new function
Resolve conflicts in DWARFExpression files after upstream refactor
…e path** * Added a new `--rich` (`-R`) command-line option to `CommandObjectDisassemble` to enable rich disassembly annotations for the current invocation. * Plumbed a new `enable_rich_annotations` flag through: * `Disassembler::Disassemble` overloads * `Disassembler::PrintInstructions` * `Instruction::Dump` * `StackFrame::Disassemble` * Updated `StackFrame::Disassemble` to take an optional `bool enable_rich_annotations` (default `false`) so the SB API can request annotated output without CLI involvement. * Ensured annotations are only added when `enable_rich_annotations` is `true`; preserved caching for the non-rich path. * Modified `Options.td` to define the new `--rich` option. * Added/updated API test `TestRichDisassembler.py` to run `disassemble --rich -f` and check annotated output. * Kept default behavior unchanged so existing scripts and IDE integrations are unaffected.
This change introduces a simple live-variable tracking system for annotated
disassembly. While iterating over instructions, we now maintain an
unordered_map keyed by `lldb::user_id_t` to remember each in-scope variable's
last known location string.
For each instruction:
* If a variable is new, print `name = location` and add it to the map.
* If a variable's location has changed, print the updated mapping.
* If a previously tracked variable is no longer found, print
`name = <undef>` and remove it.
This produces concise, stateful annotations that only update when needed,
reducing noise in the disassembly while still showing variable lifetimes.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@adrian-prantl @JDevlieghere PR posted for the variable location tracking and annotations moved to |
| @@ -445,10 +444,11 @@ class Disassembler : public std::enable_shared_from_this<Disassembler>, | |||
| const ExecutionContext &exe_ctx, const Address &start, | |||
| Limit limit, bool mixed_source_and_assembly, | |||
| uint32_t num_mixed_context_lines, uint32_t options, | |||
There was a problem hiding this comment.
Why is the new flag not part of options?
There was a problem hiding this comment.
They are defined in Disassembler.h
There was a problem hiding this comment.
Ah, I see. I misunderstood earlier when you mentioned about “options,” thinking you were referring only to CommandObjectDisassemble’s CommandOptions. I missed that you meant the existing Disassembler options bitmask.
I’ll update the patch so that --rich sets a new eOptionRichAnnotations flag in the Disassembler options enum instead of plumbing a separate boolean through the APIs, and then read that flag in PrintInstructions to enable annotations.
There was a problem hiding this comment.
No I wasn't being quite as specific ;-)
I did mean to add a user-visible option to the command (which you implemented as --rich), and I didn't bother checking how to thread that flag through all layers of the API. At the lldb_private::Dissassembler reusing the existing option set seems like the right choice.
There was a problem hiding this comment.
As an aside: given that there is an existing -r (--raw) option to the disassemble command, what do you think about naming it --variables <boolean> to be both more descriptive and avoid confusion with the -r option?
There was a problem hiding this comment.
Seems like a good idea to me. Right now --rich is synonymous to -R, but I agree --variable would be clear to users to be different.
lldb/source/Core/Disassembler.cpp
Outdated
| }; | ||
|
|
||
| // Track live variables across instructions (keyed by stable LLDB user_id_t) | ||
| std::unordered_map<lldb::user_id_t, VarState> live_vars; |
There was a problem hiding this comment.
Not a big deal, but we typically prefer using llvm::DenseMap<> or llvm::SmallDenseMap<>
There was a problem hiding this comment.
I see, the erase loop you're doing depends on this.
There was a problem hiding this comment.
Thanks for the pointer, I have switched to using llvm::SmallDenseMap<> now.
lldb/source/Core/Disassembler.cpp
Outdated
| if (!frame || !target_sp || !process_sp) | ||
| return events; | ||
|
|
||
| // Reset "seen" flags for this instruction |
There was a problem hiding this comment.
FYI: LLVM coding style wants full sentences in comments with a . at the end. (This applies to all comments)
There was a problem hiding this comment.
Sorry, I keep forgetting to do this, thank you for all the reminders. Updating them now.
| @@ -0,0 +1,6 @@ | |||
|
|
|||
| # CXX_SOURCES := a_loop_with_local_variable.c b_multiple_stack_variables.c c_variable_passed_to_another_function.c d_original_example.c e_control_flow_edge.c | |||
There was a problem hiding this comment.
Removed now
lldb/source/Core/Disassembler.cpp
Outdated
| // enhances source-level debugging. | ||
|
|
||
| struct VarState { | ||
| std::string name; // display name |
There was a problem hiding this comment.
Suggested change
| std::string name; // display name | |
| std::string name; ///< Display name. |
This change refactors how the `--rich` flag is handled, based on code review feedback.
- Removed the `enable_rich_annotations` boolean from the API signatures of:
- Disassembler::Disassemble(...)
- Disassembler::PrintInstructions(...)
- StackFrame::Disassemble(...)
- Added a new Disassembler::Option enum value: eOptionRichAnnotations.
- The `--rich` CLI flag now sets the new option bit in CommandObjectDisassemble::DoExecute:
options |= Disassembler::eOptionRichAnnotations;
- Disassembler::PrintInstructions checks the bit to determine whether to enable rich annotations:
const bool enable_rich = (options & eOptionRichAnnotations) != 0;
The SB API remains unchanged and defaults to non-rich output.
Tested via the existing test using `disassemble --rich -f`.
Address code review feedback suggesting the use of LLVM's DenseMap family over std::unordered_map for consistency and potential performance benefits within LLDB. Replaced: std::unordered_map<lldb::user_id_t, VarState> with: llvm::SmallDenseMap<lldb::user_id_t, VarState, 8> The small buffer size of 8 is a heuristic for typical numbers of live variables in scope, reducing allocations for common cases.
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.
Context
Follow-up to #147460, which added the ability to surface register-resident variable locations.
This PR moves the annotation logic out of
Instruction::Dump()and intoDisassembler::PrintInstructions(), and adds lightweight state tracking so we only print changes at range starts and when variables go out of scope.What this does
While iterating the instructions for a function, we maintain a “live variable map” keyed by
lldb::user_id_t(theVariable’s ID) to remember each variable’s last emitted location string. For each instruction:name = <location>once at the start of its DWARF location range, cache it.name = <undef>and drop it.This produces concise, stateful annotations that highlight variable lifetime transitions without spamming every line.
Why in
PrintInstructions()?Instructionstateless and avoids changing theInstruction::Dump()virtual API.prev → current) inside the single driver loop.How it works (high-level)
StackFrame::GetInScopeVariableList(/*get_parent=*/true).Variable, queryDWARFExpressionList::GetExpressionEntryAtAddress(func_load_addr, current_pc)(added in [lldb] Add DWARFExpressionEntry and GetExpressionEntryAtAddress() to … #144238).DumpLocation(..., eDescriptionLevelBrief, abi)to get a short, ABI-aware location string (e.g.,DW_OP_reg3 RBX → RBX).name = <location>and record it.name = <undef>for any that disappeared.Internally:
DWARFExpressionList.Example output (x86_64, simplified)
Only transitions are shown: the start of a location, changes, and end-of-lifetime.
Scope & limitations (by design)
DumpLocation).Implementation notes
Disassembler::PrintInstructions().std::unordered_map<lldb::user_id_t, std::string>as the live map.Instructioninterface.Requested feedback
<undef>marker.ExecutionContext).Thanks for reviewing! Happy to adjust behavior/format based on feedback.