-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[lldb] Refactor variable annotation logic in Disassembler::PrintInstructions #156118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb] Refactor variable annotation logic in Disassembler::PrintInstructions #156118
Conversation
…uctions - Move VarState definition out of PrintInstructions into a private helper (anonymous namespace), with member comments placed before fields. - Introduce a VariableAnnotator helper class to encapsulate state and logic for live variable tracking across instructions. - Replace seen_this_inst bookkeeping with a map-diff approach: recompute the current variable set per instruction and diff against the previous set. - Use nullptr instead of an empty ProcessSP when calling ABI::FindPlugin. - Narrow Block* scope with `if (Block *B = ...)`. - Set DIDumpOptions::PrintRegisterOnly directly from `static_cast<bool>(abi_sp)`. - Prefer emplace_back over push_back for event strings. - General cleanup to match LLVM coding style and reviewer feedback. This makes the annotation code easier to read and consistent with LLVM/LLDB conventions while preserving functionality.
@llvm/pr-subscribers-lldb Author: Abdullah Mohammad Amin (UltimateForce21) ChangesThis patch is a follow-up to #152887, addressing review comments that came in after the original change was merged.
This makes the annotation code easier to read and consistent with LLVM/LLDB conventions while preserving functionality. Full diff: https://github.com/llvm/llvm-project/pull/156118.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Core/Disassembler.h b/lldb/include/lldb/Core/Disassembler.h
index 6a21470b2472c..db186dd33d774 100644
--- a/lldb/include/lldb/Core/Disassembler.h
+++ b/lldb/include/lldb/Core/Disassembler.h
@@ -566,6 +566,26 @@ class Disassembler : public std::enable_shared_from_this<Disassembler>,
const Disassembler &operator=(const Disassembler &) = delete;
};
+/// Tracks live variable annotations across instructions and produces
+/// per-instruction "events" like `name = RDI` or `name = <undef>`.
+class VariableAnnotator {
+ struct VarState {
+ /// Display name.
+ std::string name;
+ /// Last printed location (empty means <undef>).
+ std::string last_loc;
+ };
+
+ // Live state from the previous instruction, keyed by Variable::GetID().
+ llvm::DenseMap<lldb::user_id_t, VarState> Live_;
+
+public:
+ /// Compute annotation strings for a single instruction and update `Live_`.
+ /// Returns only the events that should be printed *at this instruction*.
+ std::vector<std::string> annotate(Instruction &inst, Target &target,
+ const lldb::ModuleSP &module_sp);
+};
+
} // namespace lldb_private
#endif // LLDB_CORE_DISASSEMBLER_H
diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index 9b0b604d6caa0..f2ed1f7395346 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -286,6 +286,127 @@ bool Disassembler::ElideMixedSourceAndDisassemblyLine(
return false;
}
+// For each instruction, this block attempts to resolve in-scope variables
+// and determine if the current PC falls within their
+// DWARF location entry. If so, it prints a simplified annotation using the
+// variable name and its resolved location (e.g., "var = reg; " ).
+//
+// Annotations are only included if the variable has a valid DWARF location
+// entry, and the location string is non-empty after filtering. Decoding
+// errors and DWARF opcodes are intentionally omitted to keep the output
+// concise and user-friendly.
+//
+// The goal is to give users helpful live variable hints alongside the
+// disassembled instruction stream, similar to how debug information
+// enhances source-level debugging.
+std::vector<std::string>
+VariableAnnotator::annotate(Instruction &inst, Target &target,
+ const lldb::ModuleSP &module_sp) {
+ std::vector<std::string> events;
+
+ // If we lost module context, everything becomes <undef>.
+ if (!module_sp) {
+ for (const auto &KV : Live_)
+ events.emplace_back(llvm::formatv("{0} = <undef>", KV.second.name).str());
+ Live_.clear();
+ return events;
+ }
+
+ // Resolve function/block at this *file* address.
+ SymbolContext sc;
+ const Address &iaddr = inst.GetAddress();
+ const auto mask = eSymbolContextFunction | eSymbolContextBlock;
+ if (!module_sp->ResolveSymbolContextForAddress(iaddr, mask, sc) ||
+ !sc.function) {
+ // No function context: everything dies here.
+ for (const auto &KV : Live_)
+ events.emplace_back(llvm::formatv("{0} = <undef>", KV.second.name).str());
+ Live_.clear();
+ return events;
+ }
+
+ // Collect in-scope variables for this instruction into Current.
+ VariableList var_list;
+ // Innermost block containing iaddr.
+ if (Block *B = sc.block) {
+ auto filter = [](Variable *v) -> bool { return v && !v->IsArtificial(); };
+ B->AppendVariables(/*can_create*/ true,
+ /*get_parent_variables*/ true,
+ /*stop_if_block_is_inlined_function*/ false,
+ /*filter*/ filter,
+ /*variable_list*/ &var_list);
+ }
+
+ const lldb::addr_t pc_file = iaddr.GetFileAddress();
+ const lldb::addr_t func_file = sc.function->GetAddress().GetFileAddress();
+
+ // ABI from Target (pretty reg names if plugin exists). Safe to be null.
+ lldb::ABISP abi_sp = ABI::FindPlugin(nullptr, target.GetArchitecture());
+ ABI *abi = abi_sp.get();
+
+ llvm::DIDumpOptions opts;
+ opts.ShowAddresses = false;
+ // Prefer "register-only" output when we have an ABI.
+ opts.PrintRegisterOnly = static_cast<bool>(abi_sp);
+
+ llvm::DenseMap<lldb::user_id_t, VarState> Current;
+
+ for (size_t i = 0, e = var_list.GetSize(); i != e; ++i) {
+ lldb::VariableSP v = var_list.GetVariableAtIndex(i);
+ if (!v || v->IsArtificial())
+ continue;
+
+ const char *nm = v->GetName().AsCString();
+ llvm::StringRef name = nm ? nm : "<anon>";
+
+ DWARFExpressionList &exprs = v->LocationExpressionList();
+ if (!exprs.IsValid())
+ continue;
+
+ auto entry_or_err = exprs.GetExpressionEntryAtAddress(func_file, pc_file);
+ if (!entry_or_err)
+ continue;
+
+ auto entry = *entry_or_err;
+
+ StreamString loc_ss;
+ entry.expr->DumpLocation(&loc_ss, eDescriptionLevelBrief, abi, opts);
+
+ llvm::StringRef loc = llvm::StringRef(loc_ss.GetString()).trim();
+ if (loc.empty())
+ continue;
+
+ Current.try_emplace(v->GetID(),
+ VarState{std::string(name), std::string(loc)});
+ }
+
+ // Diff Live_ → Current.
+
+ // 1) Starts/changes: iterate Current and compare with Live_.
+ for (const auto &KV : Current) {
+ auto it = Live_.find(KV.first);
+ if (it == Live_.end()) {
+ // Newly live.
+ events.emplace_back(
+ llvm::formatv("{0} = {1}", KV.second.name, KV.second.last_loc).str());
+ } else if (it->second.last_loc != KV.second.last_loc) {
+ // Location changed.
+ events.emplace_back(
+ llvm::formatv("{0} = {1}", KV.second.name, KV.second.last_loc).str());
+ }
+ }
+
+ // 2) Ends: anything that was live but is not in Current becomes <undef>.
+ for (const auto &KV : Live_) {
+ if (!Current.count(KV.first))
+ events.emplace_back(llvm::formatv("{0} = <undef>", KV.second.name).str());
+ }
+
+ // Commit new state.
+ Live_ = std::move(Current);
+ return events;
+}
+
void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
const ExecutionContext &exe_ctx,
bool mixed_source_and_assembly,
@@ -382,147 +503,7 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
}
}
- // Add variable location annotations to the disassembly output.
- //
- // For each instruction, this block attempts to resolve in-scope variables
- // and determine if the current PC falls within their
- // DWARF location entry. If so, it prints a simplified annotation using the
- // variable name and its resolved location (e.g., "var = reg; " ).
- //
- // Annotations are only included if the variable has a valid DWARF location
- // entry, and the location string is non-empty after filtering. Decoding
- // errors and DWARF opcodes are intentionally omitted to keep the output
- // concise and user-friendly.
- //
- // The goal is to give users helpful live variable hints alongside the
- // disassembled instruction stream, similar to how debug information
- // enhances source-level debugging.
-
- struct VarState {
- std::string name; ///< Display name.
- std::string last_loc; ///< Last printed location (empty means <undef>).
- bool seen_this_inst = false;
- };
-
- // Track live variables across instructions.
- llvm::DenseMap<lldb::user_id_t, VarState> live_vars;
-
- // Stateful annotator: updates live_vars and returns only what should be
- // printed for THIS instruction.
- auto annotate_static = [&](Instruction &inst, Target &target,
- ModuleSP module_sp) -> std::vector<std::string> {
- std::vector<std::string> events;
-
- // Reset per-instruction seen flags.
- for (auto &kv : live_vars)
- kv.second.seen_this_inst = false;
-
- const Address &iaddr = inst.GetAddress();
- if (!module_sp) {
- // Everything previously live becomes <undef>.
- for (auto I = live_vars.begin(), E = live_vars.end(); I != E;) {
- auto Cur = I++;
- events.push_back(
- llvm::formatv("{0} = <undef>", Cur->second.name).str());
- live_vars.erase(Cur);
- }
- return events;
- }
-
- // Resolve innermost block at this *file* address.
- SymbolContext sc;
- const lldb::SymbolContextItem mask =
- eSymbolContextFunction | eSymbolContextBlock;
- if (!module_sp->ResolveSymbolContextForAddress(iaddr, mask, sc) ||
- !sc.function) {
- // No function context: everything dies here.
- for (auto I = live_vars.begin(), E = live_vars.end(); I != E;) {
- auto Cur = I++;
- events.push_back(
- llvm::formatv("{0} = <undef>", Cur->second.name).str());
- live_vars.erase(Cur);
- }
- return events;
- }
-
- Block *B = sc.block; ///< Innermost block containing iaddr.
- VariableList var_list;
- if (B) {
- auto filter = [](Variable *v) -> bool { return v && !v->IsArtificial(); };
-
- B->AppendVariables(/*can_create*/ true,
- /*get_parent_variables*/ true,
- /*stop_if_block_is_inlined_function*/ false,
- /*filter*/ filter,
- /*variable_list*/ &var_list);
- }
-
- const lldb::addr_t pc_file = iaddr.GetFileAddress();
- const lldb::addr_t func_file = sc.function->GetAddress().GetFileAddress();
-
- // ABI from Target (pretty reg names if plugin exists). Safe to be null.
- lldb::ProcessSP no_process;
- lldb::ABISP abi_sp = ABI::FindPlugin(no_process, target.GetArchitecture());
- ABI *abi = abi_sp.get();
-
- llvm::DIDumpOptions opts;
- opts.ShowAddresses = false;
- if (abi)
- opts.PrintRegisterOnly = true;
-
- for (size_t i = 0, e = var_list.GetSize(); i != e; ++i) {
- lldb::VariableSP v = var_list.GetVariableAtIndex(i);
- if (!v || v->IsArtificial())
- continue;
-
- const char *nm = v->GetName().AsCString();
- llvm::StringRef name = nm ? nm : "<anon>";
-
- lldb_private::DWARFExpressionList &exprs = v->LocationExpressionList();
- if (!exprs.IsValid())
- continue;
-
- auto entry_or_err = exprs.GetExpressionEntryAtAddress(func_file, pc_file);
- if (!entry_or_err)
- continue;
-
- auto entry = *entry_or_err;
-
- StreamString loc_ss;
- entry.expr->DumpLocation(&loc_ss, eDescriptionLevelBrief, abi, opts);
- llvm::StringRef loc = llvm::StringRef(loc_ss.GetString()).trim();
- if (loc.empty())
- continue;
-
- auto ins = live_vars.insert(
- {v->GetID(), VarState{name.str(), loc.str(), /*seen*/ true}});
- if (ins.second) {
- // Newly live.
- events.push_back(llvm::formatv("{0} = {1}", name, loc).str());
- } else {
- VarState &vs = ins.first->second;
- vs.seen_this_inst = true;
- if (vs.last_loc != loc) {
- vs.last_loc = loc.str();
- events.push_back(llvm::formatv("{0} = {1}", vs.name, loc).str());
- }
- }
- }
-
- // Anything previously live that we didn't see a location for at this inst
- // is now <undef>.
- for (auto I = live_vars.begin(), E = live_vars.end(); I != E;) {
- auto Cur = I++;
- if (!Cur->second.seen_this_inst) {
- events.push_back(
- llvm::formatv("{0} = <undef>", Cur->second.name).str());
- live_vars.erase(Cur);
- }
- }
-
- return events;
- };
-
+ VariableAnnotator annot;
previous_symbol = nullptr;
SourceLine previous_line;
for (size_t i = 0; i < num_instructions_found; ++i) {
@@ -695,7 +676,7 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
address_text_size);
if ((options & eOptionVariableAnnotations) && target_sp) {
- auto annotations = annotate_static(*inst, *target_sp, module_sp);
+ auto annotations = annot.annotate(*inst, *target_sp, module_sp);
if (!annotations.empty()) {
const size_t annotation_column = 100;
inst_line.FillLastLineToColumn(annotation_column, ' ');
|
Hi @JDevlieghere, I’ve opened a follow-up PR to address the review comments you left on #152887 Thanks again for the detailed feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This patch is a follow-up to #152887, addressing review comments that came in after the original change was merged.
VarState
definition out ofPrintInstructions
into a private helper, with member comments placed before fields.VariableAnnotator
helper class to encapsulate state and logic for live variable tracking across instructions.seen_this_inst
flag with a map-diff approach: recompute the current variable set per instruction and diff against the previous set.nullptr
instead of an emptyProcessSP
when callingABI::FindPlugin
.Block*
scope withif (Block *B = ...)
.DIDumpOptions::PrintRegisterOnly
directly fromstatic_cast<bool>(abi_sp)
.emplace_back
overpush_back
for event strings.This makes the annotation code easier to read and consistent with LLVM/LLDB conventions while preserving functionality.