Automerge: [lldb] Refactor variable annotation logic in Disassembler::PrintInstructions (#156118)

This patch is a follow-up to
[#152887](llvm/llvm-project#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.

Author: UltimateForce21

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

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, ' ');