Skip to content

Conversation

UltimateForce21
Copy link
Contributor

This patch is a follow-up to #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.

…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.
@llvmbot
Copy link
Member

llvmbot commented Aug 29, 2025

@llvm/pr-subscribers-lldb

Author: Abdullah Mohammad Amin (UltimateForce21)

Changes

This patch is a follow-up to #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&lt;bool&gt;(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.


Full diff: https://github.com/llvm/llvm-project/pull/156118.diff

2 Files Affected:

  • (modified) lldb/include/lldb/Core/Disassembler.h (+20)
  • (modified) lldb/source/Core/Disassembler.cpp (+123-142)
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, ' ');

@UltimateForce21
Copy link
Contributor Author

Hi @JDevlieghere,

I’ve opened a follow-up PR to address the review comments you left on #152887
after it was merged. This PR moves VarState into a helper class, adopts a map-diff approach for live variable tracking, and applies the style/cleanup changes you suggested.

Thanks again for the detailed feedback!

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@JDevlieghere JDevlieghere merged commit d39772c into llvm:main Aug 30, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants