[Lldb-commits] [lldb] [lldb][NFC] Add helper to compute breakpoint's constituent load address (PR #190762)

[Felipe de Azevedo Piovezan via lldb-commits]

https://github.com/felipepiovezan updated https://github.com/llvm/llvm-project/pull/190762

>From 23083dee3f8aec713d6fc5d99b9a7c53cb93c7b6 Mon Sep 17 00:00:00 2001
From: Felipe de Azevedo Piovezan <fpiovezan at apple.com>
Date: Tue, 31 Mar 2026 15:39:05 +0100
Subject: [PATCH] [lldb][NFC] Add helper to compute breakpoint's constituent
 load address

This allows the callsite to be simplified.
This also exposes a bug where the variable `ShouldShowError` is guarding
more than the error printing.
---
 lldb/source/Target/Process.cpp | 120 +++++++++++++++++----------------
 1 file changed, 62 insertions(+), 58 deletions(-)

diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index ac182f1746712..5ddf0c3f1593f 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1600,76 +1600,80 @@ static bool ShouldShowError(Process &process) {
   llvm_unreachable("unhandled process state");
 }
 
-lldb::break_id_t
-Process::CreateBreakpointSite(const BreakpointLocationSP &constituent,
-                              bool use_hardware) {
-  addr_t load_addr = LLDB_INVALID_ADDRESS;
+static addr_t ComputeConstituentLoadAddress(BreakpointLocation &constituent,
+                                            Process &proc) {
+  // Reset the IsIndirect flag here, in case the location changes from pointing
+  // from an indirect symbol to a regular symbol.
+  constituent.SetIsIndirect(false);
 
-  bool show_error = ShouldShowError(*this);
+  Target &target = proc.GetTarget();
 
-  // Reset the IsIndirect flag here, in case the location changes from pointing
-  // to a indirect symbol to a regular symbol.
-  constituent->SetIsIndirect(false);
+  if (!constituent.ShouldResolveIndirectFunctions())
+    return constituent.GetAddress().GetOpcodeLoadAddress(&target);
 
-  if (constituent->ShouldResolveIndirectFunctions()) {
-    const Symbol *symbol =
-        constituent->GetAddress().CalculateSymbolContextSymbol();
-    if (symbol && symbol->IsIndirect()) {
-      Status error;
-      Address symbol_address = symbol->GetAddress();
-      load_addr = ResolveIndirectFunction(&symbol_address, error);
-      if (!error.Success() && show_error) {
-        GetTarget().GetDebugger().GetAsyncErrorStream()->Printf(
-            "warning: failed to resolve indirect function at 0x%" PRIx64
-            " for breakpoint %i.%i: %s\n",
-            symbol->GetLoadAddress(&GetTarget()),
-            constituent->GetBreakpoint().GetID(), constituent->GetID(),
-            error.AsCString() ? error.AsCString() : "unknown error");
-        return LLDB_INVALID_BREAK_ID;
-      }
-      Address resolved_address(load_addr);
-      load_addr = resolved_address.GetOpcodeLoadAddress(&GetTarget());
-      constituent->SetIsIndirect(true);
-    } else
-      load_addr = constituent->GetAddress().GetOpcodeLoadAddress(&GetTarget());
-  } else
-    load_addr = constituent->GetAddress().GetOpcodeLoadAddress(&GetTarget());
+  const Symbol *symbol =
+      constituent.GetAddress().CalculateSymbolContextSymbol();
+  if (!symbol || !symbol->IsIndirect())
+    return constituent.GetAddress().GetOpcodeLoadAddress(&target);
+
+  // An indirect symbol is involved.
+  Status error;
+  Address symbol_address = symbol->GetAddress();
+  addr_t load_addr = proc.ResolveIndirectFunction(&symbol_address, error);
+
+  if (!error.Success() && ShouldShowError(proc)) {
+    target.GetDebugger().GetAsyncErrorStream()->Printf(
+        "warning: failed to resolve indirect function at 0x%" PRIx64
+        " for breakpoint %i.%i: %s\n",
+        symbol->GetLoadAddress(&target), constituent.GetBreakpoint().GetID(),
+        constituent.GetID(),
+        error.AsCString() ? error.AsCString() : "unknown error");
+    // FIXME: ShouldShowError must only guard the error message.
+    return LLDB_INVALID_ADDRESS;
+  }
 
-  if (load_addr != LLDB_INVALID_ADDRESS) {
-    BreakpointSiteSP bp_site_sp;
+  Address resolved_address(load_addr);
+  constituent.SetIsIndirect(true);
+  return resolved_address.GetOpcodeLoadAddress(&target);
+}
 
-    // Look up this breakpoint site.  If it exists, then add this new
-    // constituent, otherwise create a new breakpoint site and add it.
+lldb::break_id_t
+Process::CreateBreakpointSite(const BreakpointLocationSP &constituent,
+                              bool use_hardware) {
+  addr_t load_addr = ComputeConstituentLoadAddress(*constituent, *this);
 
-    bp_site_sp = m_breakpoint_site_list.FindByAddress(load_addr);
+  if (load_addr == LLDB_INVALID_ADDRESS)
+    return LLDB_INVALID_BREAK_ID;
 
+  BreakpointSiteSP bp_site_sp;
+
+  // Look up this breakpoint site.  If it exists, then add this new
+  // constituent, otherwise create a new breakpoint site and add it.
+  bp_site_sp = m_breakpoint_site_list.FindByAddress(load_addr);
+  if (bp_site_sp) {
+    bp_site_sp->AddConstituent(constituent);
+    constituent->SetBreakpointSite(bp_site_sp);
+    return bp_site_sp->GetID();
+  } else {
+    bp_site_sp.reset(new BreakpointSite(constituent, load_addr, use_hardware));
     if (bp_site_sp) {
-      bp_site_sp->AddConstituent(constituent);
-      constituent->SetBreakpointSite(bp_site_sp);
-      return bp_site_sp->GetID();
-    } else {
-      bp_site_sp.reset(
-          new BreakpointSite(constituent, load_addr, use_hardware));
-      if (bp_site_sp) {
-        Status error = EnableBreakpointSite(bp_site_sp.get());
-        if (error.Success()) {
-          constituent->SetBreakpointSite(bp_site_sp);
-          return m_breakpoint_site_list.Add(bp_site_sp);
-        } else {
-          if (show_error || use_hardware) {
-            // Report error for setting breakpoint...
-            GetTarget().GetDebugger().GetAsyncErrorStream()->Printf(
-                "warning: failed to set breakpoint site at 0x%" PRIx64
-                " for breakpoint %i.%i: %s\n",
-                load_addr, constituent->GetBreakpoint().GetID(),
-                constituent->GetID(),
-                error.AsCString() ? error.AsCString() : "unknown error");
-          }
+      Status error = EnableBreakpointSite(bp_site_sp.get());
+      if (error.Success()) {
+        constituent->SetBreakpointSite(bp_site_sp);
+        return m_breakpoint_site_list.Add(bp_site_sp);
+      } else {
+        if (ShouldShowError(*this) || use_hardware) {
+          // Report error for setting breakpoint...
+          GetTarget().GetDebugger().GetAsyncErrorStream()->Printf(
+              "warning: failed to set breakpoint site at 0x%" PRIx64
+              " for breakpoint %i.%i: %s\n",
+              load_addr, constituent->GetBreakpoint().GetID(),
+              constituent->GetID(),
+              error.AsCString() ? error.AsCString() : "unknown error");
         }
       }
     }
   }
-  // We failed to enable the breakpoint
   return LLDB_INVALID_BREAK_ID;
 }