Author: Abdullah Mohammad Amin Date: 2025-08-30T13:17:05-07:00 New Revision: d39772cb053c194ef809f3ca765f37237c4e1956
URL: https://github.com/llvm/llvm-project/commit/d39772cb053c194ef809f3ca765f37237c4e1956 DIFF: https://github.com/llvm/llvm-project/commit/d39772cb053c194ef809f3ca765f37237c4e1956.diff LOG: [lldb] Refactor variable annotation logic in Disassembler::PrintInstructions (#156118) This patch is a follow-up to [#152887](https://github.com/llvm/llvm-project/pull/152887), addressing review comments that came in after the original change was merged. - Move `VarState` definition out of `PrintInstructions` into a private helper, 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` flag 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. Added: Modified: lldb/include/lldb/Core/Disassembler.h lldb/source/Core/Disassembler.cpp Removed: ################################################################################ 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, ' '); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
