[lldb] [disassembler] chore: update VariableAnnotator::Annotate to except only Instruction as param and drop module and target #168276
Conversation
…cept only Instruction as param and drop module and target, rename Live_ and Current to follow lldb codding style Signed-off-by: Nikita B <[email protected]>
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: None (n2h9) ChangesDescriptionThis PR simplifies the other minor changes (to follow lldb coding style):
Performance impact
DetailedI have moved some refactoring from #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 While working in this pr #165163 I have noticed that In particular:
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 TestingUnit tests are passing <details> Full diff: https://github.com/llvm/llvm-project/pull/168276.diff 2 Files Affected:
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, ' ');
|
3 participants
Description
This PR simplifies the
VariableAnnotator::AnnotateAPI by removing redundant parameters, making it easier to usefrom future scripting APIs.
other minor changes (to follow lldb coding style):
annotatetoAnnotate;Live_andCurrentvariables;Performance impact
GetModule()per instruction;GetModuleper instruction.(additional call to
GetModuleinsideAnnotatemethod.)Detailed
I have moved some refactoring from #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 #165163 I have noticed that
VariableAnnotator::Annotateacceptsinstruction,moduleandtargetas input params. Which could be simplified to depend only on theinstructionobject.In particular:
targetis used to get architecture specification, which could be taken frommodule.modulecould be taken frominstruction'sAddressobject.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
instructionobject.Testing
Run test with
all tests (9 existing) are passing.
screenshot 2025-11-16