llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: None (n2h9) <details> <summary>Changes</summary> ## Description This PR simplifies the `VariableAnnotator::Annotate` API by removing redundant parameters, making it easier to use from future scripting APIs. other minor changes (to follow lldb coding style): - rename `annotate` to `Annotate`; - rename Live_ and Current variables; ### Performance impact - current version (in main) does one call to `GetModule()` per instruction; - updated version makes two calls to `GetModule` per instruction. (additional call to `GetModule` inside `Annotate` method.) ### Detailed I have moved some refactoring from https://github.com/llvm/llvm-project/pull/165163 to make it easier to review and split to smaller parts, and this particular part could be treated as not related to structured API but as general enhancement of `VariableAnnotator`. While working in this pr https://github.com/llvm/llvm-project/pull/165163 I have noticed that `VariableAnnotator::Annotate` accepts `instruction`, `module` and `target` as input params. Which could be simplified to depend only on the `instruction` object. In particular: - `target` is used to get architecture specification, which could be taken from `module`. - `module` could be taken from `instruction`'s `Address` object. Each variable annotation is rather tied to the instruction's execution point, then to the module or target as a whole. So, it looks like it will make api cleaner if it will depend only on `instruction` object. ## Testing Unit tests are passing <details> <summary>screenshot</summary> <img width="1268" height="849" alt="image" src="https://github.com/user-attachments/assets/417b4b3c-dea0-4348-b5ce-bf3deaa01d81"; /> <details> --- Full diff: https://github.com/llvm/llvm-project/pull/168276.diff 2 Files Affected: - (modified) lldb/include/lldb/Core/Disassembler.h (+5-5) - (modified) lldb/source/Core/Disassembler.cpp (+22-22) ``````````diff diff --git a/lldb/include/lldb/Core/Disassembler.h b/lldb/include/lldb/Core/Disassembler.h index daa7b3d25f11b..5de314109b0cc 100644 --- a/lldb/include/lldb/Core/Disassembler.h +++ b/lldb/include/lldb/Core/Disassembler.h @@ -581,13 +581,13 @@ class VariableAnnotator { }; // Live state from the previous instruction, keyed by Variable::GetID(). - llvm::DenseMap<lldb::user_id_t, VarState> Live_; + llvm::DenseMap<lldb::user_id_t, VarState> m_live_vars; 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); + /// Compute annotation strings for a single instruction and update + /// `m_live_vars`. Returns only the events that should be printed *at this + /// instruction*. + std::vector<std::string> Annotate(Instruction &inst); }; } // namespace lldb_private diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp index f2ed1f7395346..431678f505f77 100644 --- a/lldb/source/Core/Disassembler.cpp +++ b/lldb/source/Core/Disassembler.cpp @@ -299,16 +299,16 @@ bool Disassembler::ElideMixedSourceAndDisassemblyLine( // 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> VariableAnnotator::Annotate(Instruction &inst) { std::vector<std::string> events; + auto module_sp = inst.GetAddress().GetModule(); + // If we lost module context, everything becomes <undef>. if (!module_sp) { - for (const auto &KV : Live_) + for (const auto &KV : m_live_vars) events.emplace_back(llvm::formatv("{0} = <undef>", KV.second.name).str()); - Live_.clear(); + m_live_vars.clear(); return events; } @@ -319,13 +319,13 @@ VariableAnnotator::annotate(Instruction &inst, Target &target, if (!module_sp->ResolveSymbolContextForAddress(iaddr, mask, sc) || !sc.function) { // No function context: everything dies here. - for (const auto &KV : Live_) + for (const auto &KV : m_live_vars) events.emplace_back(llvm::formatv("{0} = <undef>", KV.second.name).str()); - Live_.clear(); + m_live_vars.clear(); return events; } - // Collect in-scope variables for this instruction into Current. + // Collect in-scope variables for this instruction into current_vars. VariableList var_list; // Innermost block containing iaddr. if (Block *B = sc.block) { @@ -341,7 +341,7 @@ VariableAnnotator::annotate(Instruction &inst, Target &target, 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()); + lldb::ABISP abi_sp = ABI::FindPlugin(nullptr, module_sp->GetArchitecture()); ABI *abi = abi_sp.get(); llvm::DIDumpOptions opts; @@ -349,7 +349,7 @@ VariableAnnotator::annotate(Instruction &inst, Target &target, // Prefer "register-only" output when we have an ABI. opts.PrintRegisterOnly = static_cast<bool>(abi_sp); - llvm::DenseMap<lldb::user_id_t, VarState> Current; + llvm::DenseMap<lldb::user_id_t, VarState> current_vars; for (size_t i = 0, e = var_list.GetSize(); i != e; ++i) { lldb::VariableSP v = var_list.GetVariableAtIndex(i); @@ -376,16 +376,16 @@ VariableAnnotator::annotate(Instruction &inst, Target &target, if (loc.empty()) continue; - Current.try_emplace(v->GetID(), - VarState{std::string(name), std::string(loc)}); + current_vars.try_emplace(v->GetID(), + VarState{std::string(name), std::string(loc)}); } - // Diff Live_ → Current. + // Diff m_live_vars → current_vars. - // 1) Starts/changes: iterate Current and compare with Live_. - for (const auto &KV : Current) { - auto it = Live_.find(KV.first); - if (it == Live_.end()) { + // 1) Starts/changes: iterate current_vars and compare with m_live_vars. + for (const auto &KV : current_vars) { + auto it = m_live_vars.find(KV.first); + if (it == m_live_vars.end()) { // Newly live. events.emplace_back( llvm::formatv("{0} = {1}", KV.second.name, KV.second.last_loc).str()); @@ -396,14 +396,14 @@ VariableAnnotator::annotate(Instruction &inst, Target &target, } } - // 2) Ends: anything that was live but is not in Current becomes <undef>. - for (const auto &KV : Live_) { - if (!Current.count(KV.first)) + // 2) Ends: anything that was live but is not in current_vars becomes <undef>. + for (const auto &KV : m_live_vars) { + if (!current_vars.count(KV.first)) events.emplace_back(llvm::formatv("{0} = <undef>", KV.second.name).str()); } // Commit new state. - Live_ = std::move(Current); + m_live_vars = std::move(current_vars); return events; } @@ -676,7 +676,7 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch, address_text_size); if ((options & eOptionVariableAnnotations) && target_sp) { - auto annotations = annot.annotate(*inst, *target_sp, module_sp); + auto annotations = annot.Annotate(*inst); if (!annotations.empty()) { const size_t annotation_column = 100; inst_line.FillLastLineToColumn(annotation_column, ' '); `````````` </details> https://github.com/llvm/llvm-project/pull/168276 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
