[lldb] Don't hand out UnwindPlan::Row shared_ptrs (#128181)

The whole unwind plan is already stored in a shared pointer, and there's
no need to persist Rows individually. If there's ever a need to do that,
there are at least two options:
- copy the row (they're not that big, and they're being copied left and
right during construction already)
- use the shared_ptr subobject constructor to create a shared_ptr which
points to a Row but holds the entire unwind plan alive

This also changes all of the getter functions to return const Row
pointers, which is important for safety because all of these objects are
cached and potentially accessed from multiple threads. (Technically one
could hand out `shared_ptr<const Row>`s, but we don't have a habit of
doing that.)

As a next step, I'd like to remove the internal UnwindPlan usages of the
shared pointer, but I'm doing this separately to gauge feedback, and
also because the patch got rather big.

Author: labath

lldb/include/lldb/Symbol/UnwindPlan.h

@@ -175,13 +175,13 @@ class UnwindPlan {
 
       void SetIsDWARFExpression(const uint8_t *opcodes, uint32_t len);
 
-      const uint8_t *GetDWARFExpressionBytes() {
+      const uint8_t *GetDWARFExpressionBytes() const {
         if (m_type == atDWARFExpression || m_type == isDWARFExpression)
           return m_location.expr.opcodes;
         return nullptr;
       }
 
-      int GetDWARFExpressionLength() {
+      int GetDWARFExpressionLength() const {
         if (m_type == atDWARFExpression || m_type == isDWARFExpression)
           return m_location.expr.length;
         return 0;
@@ -308,13 +308,13 @@ class UnwindPlan {
         }
       }
 
-      const uint8_t *GetDWARFExpressionBytes() {
+      const uint8_t *GetDWARFExpressionBytes() const {
         if (m_type == isDWARFExpression)
           return m_value.expr.opcodes;
         return nullptr;
       }
 
-      int GetDWARFExpressionLength() {
+      int GetDWARFExpressionLength() const {
         if (m_type == isDWARFExpression)
           return m_value.expr.length;
         return 0;
@@ -362,8 +362,10 @@ class UnwindPlan {
 
     void SlideOffset(lldb::addr_t offset) { m_offset += offset; }
 
+    const FAValue &GetCFAValue() const { return m_cfa_value; }
     FAValue &GetCFAValue() { return m_cfa_value; }
 
+    const FAValue &GetAFAValue() const { return m_afa_value; }
     FAValue &GetAFAValue() { return m_afa_value; }
 
     bool SetRegisterLocationToAtCFAPlusOffset(uint32_t reg_num, int32_t offset,
@@ -464,7 +466,7 @@ class UnwindPlan {
   // unknown - the final row in the UnwindPlan is returned. In practice, the
   // UnwindPlan for a function with no known start address will be the
   // architectural default UnwindPlan which will only have one row.
-  UnwindPlan::RowSP GetRowForFunctionOffset(int offset) const;
+  const UnwindPlan::Row *GetRowForFunctionOffset(int offset) const;
 
   lldb::RegisterKind GetRegisterKind() const { return m_register_kind; }
 
@@ -495,9 +497,9 @@ class UnwindPlan {
 
   bool IsValidRowIndex(uint32_t idx) const;
 
-  const UnwindPlan::RowSP GetRowAtIndex(uint32_t idx) const;
+  const UnwindPlan::Row *GetRowAtIndex(uint32_t idx) const;
 
-  const UnwindPlan::RowSP GetLastRow() const;
+  const UnwindPlan::Row *GetLastRow() const;
 
   lldb_private::ConstString GetSourceName() const;
 

lldb/include/lldb/Target/RegisterContextUnwind.h

@@ -191,7 +191,8 @@ class RegisterContextUnwind : public lldb_private::RegisterContext {
 
   // Get the Frame Address register for a given frame.
   bool ReadFrameAddress(lldb::RegisterKind register_kind,
-                          UnwindPlan::Row::FAValue &fa, lldb::addr_t &address);
+                        const UnwindPlan::Row::FAValue &fa,
+                        lldb::addr_t &address);
 
   lldb::UnwindPlanSP GetFastUnwindPlanForFrame();
 

lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

@@ -116,11 +116,9 @@ bool UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
 
         // Make a copy of the current instruction Row and save it in m_curr_row
         // so we can add updates as we process the instructions.
-        UnwindPlan::RowSP last_row = unwind_plan.GetLastRow();
-        UnwindPlan::Row *newrow = new UnwindPlan::Row;
-        if (last_row.get())
-          *newrow = *last_row.get();
-        m_curr_row.reset(newrow);
+        UnwindPlan::RowSP last_row =
+            std::make_shared<UnwindPlan::Row>(*unwind_plan.GetLastRow());
+        m_curr_row = std::make_shared<UnwindPlan::Row>(*last_row);
 
         // Add the initial state to the save list with offset 0.
         saved_unwind_states.insert({0, {last_row, m_register_values}});

lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp

@@ -68,8 +68,8 @@ bool UnwindAssembly_x86::AugmentUnwindPlanFromCallSite(
     AddressRange &func, Thread &thread, UnwindPlan &unwind_plan) {
   bool do_augment_unwindplan = true;
 
-  UnwindPlan::RowSP first_row = unwind_plan.GetRowForFunctionOffset(0);
-  UnwindPlan::RowSP last_row = unwind_plan.GetRowForFunctionOffset(-1);
+  const UnwindPlan::Row *first_row = unwind_plan.GetRowForFunctionOffset(0);
+  const UnwindPlan::Row *last_row = unwind_plan.GetLastRow();
 
   int wordsize = 8;
   ProcessSP process_sp(thread.GetProcess());

lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp

@@ -1359,7 +1359,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
   if (unwind_plan.GetRowCount() < 2)
     return false;
 
-  UnwindPlan::RowSP first_row = unwind_plan.GetRowAtIndex(0);
+  const UnwindPlan::Row *first_row = unwind_plan.GetRowAtIndex(0);
   if (first_row->GetOffset() != 0)
     return false;
   uint32_t cfa_reg = first_row->GetCFAValue().GetRegisterNumber();
@@ -1372,7 +1372,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
       first_row->GetCFAValue().GetOffset() != m_wordsize)
     return false;
 
-  UnwindPlan::RowSP original_last_row = unwind_plan.GetRowForFunctionOffset(-1);
+  const UnwindPlan::Row *original_last_row = unwind_plan.GetLastRow();
 
   size_t offset = 0;
   int row_id = 1;
@@ -1417,7 +1417,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
            unwind_plan.GetRowAtIndex(row_id)->GetOffset() <= offset) {
       row_id++;
     }
-    UnwindPlan::RowSP original_row = unwind_plan.GetRowAtIndex(row_id - 1);
+    const UnwindPlan::Row *original_row = unwind_plan.GetRowAtIndex(row_id - 1);
     if (original_row->GetOffset() == offset) {
       *row = *original_row;
       continue;

lldb/source/Symbol/FuncUnwinders.cpp

@@ -366,11 +366,11 @@ LazyBool FuncUnwinders::CompareUnwindPlansForIdenticalInitialPCLocation(
   RegisterNumber pc_reg(thread, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
   uint32_t pc_reg_lldb_regnum = pc_reg.GetAsKind(eRegisterKindLLDB);
 
-  if (a.get() && b.get()) {
-    UnwindPlan::RowSP a_first_row = a->GetRowAtIndex(0);
-    UnwindPlan::RowSP b_first_row = b->GetRowAtIndex(0);
+  if (a && b) {
+    const UnwindPlan::Row *a_first_row = a->GetRowAtIndex(0);
+    const UnwindPlan::Row *b_first_row = b->GetRowAtIndex(0);
 
-    if (a_first_row.get() && b_first_row.get()) {
+    if (a_first_row && b_first_row) {
       UnwindPlan::Row::AbstractRegisterLocation a_pc_regloc;
       UnwindPlan::Row::AbstractRegisterLocation b_pc_regloc;
 

lldb/source/Symbol/UnwindPlan.cpp

@@ -412,48 +412,44 @@ void UnwindPlan::InsertRow(const UnwindPlan::RowSP &row_sp,
     *it = row_sp;
 }
 
-UnwindPlan::RowSP UnwindPlan::GetRowForFunctionOffset(int offset) const {
+const UnwindPlan::Row *UnwindPlan::GetRowForFunctionOffset(int offset) const {
+  if (m_row_list.empty())
+    return nullptr;
+  if (offset == -1)
+    return m_row_list.back().get();
+
   RowSP row;
-  if (!m_row_list.empty()) {
-    if (offset == -1)
-      row = m_row_list.back();
-    else {
-      collection::const_iterator pos, end = m_row_list.end();
-      for (pos = m_row_list.begin(); pos != end; ++pos) {
-        if ((*pos)->GetOffset() <= static_cast<lldb::offset_t>(offset))
-          row = *pos;
-        else
-          break;
-      }
-    }
+  collection::const_iterator pos, end = m_row_list.end();
+  for (pos = m_row_list.begin(); pos != end; ++pos) {
+    if ((*pos)->GetOffset() <= static_cast<lldb::offset_t>(offset))
+      row = *pos;
+    else
+      break;
   }
-  return row;
+  return row.get();
 }
 
 bool UnwindPlan::IsValidRowIndex(uint32_t idx) const {
   return idx < m_row_list.size();
 }
 
-const UnwindPlan::RowSP UnwindPlan::GetRowAtIndex(uint32_t idx) const {
+const UnwindPlan::Row *UnwindPlan::GetRowAtIndex(uint32_t idx) const {
   if (idx < m_row_list.size())
-    return m_row_list[idx];
-  else {
-    Log *log = GetLog(LLDBLog::Unwind);
-    LLDB_LOGF(log,
-              "error: UnwindPlan::GetRowAtIndex(idx = %u) invalid index "
-              "(number rows is %u)",
-              idx, (uint32_t)m_row_list.size());
-    return UnwindPlan::RowSP();
-  }
+    return m_row_list[idx].get();
+  LLDB_LOG(GetLog(LLDBLog::Unwind),
+           "error: UnwindPlan::GetRowAtIndex(idx = {0}) invalid index "
+           "(number rows is {1})",
+           idx, m_row_list.size());
+  return nullptr;
 }
 
-const UnwindPlan::RowSP UnwindPlan::GetLastRow() const {
+const UnwindPlan::Row *UnwindPlan::GetLastRow() const {
   if (m_row_list.empty()) {
     Log *log = GetLog(LLDBLog::Unwind);
     LLDB_LOGF(log, "UnwindPlan::GetLastRow() when rows are empty");
-    return UnwindPlan::RowSP();
+    return nullptr;
   }
-  return m_row_list.back();
+  return m_row_list.back().get();
 }
 
 int UnwindPlan::GetRowCount() const { return m_row_list.size(); }
@@ -486,7 +482,7 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) {
   // If the 0th Row of unwind instructions is missing, or if it doesn't provide
   // a register to use to find the Canonical Frame Address, this is not a valid
   // UnwindPlan.
-  if (GetRowAtIndex(0).get() == nullptr ||
+  if (GetRowAtIndex(0) == nullptr ||
       GetRowAtIndex(0)->GetCFAValue().GetValueType() ==
           Row::FAValue::unspecified) {
     Log *log = GetLog(LLDBLog::Unwind);

lldb/source/Target/RegisterContextUnwind.cpp

@@ -207,7 +207,7 @@ void RegisterContextUnwind::InitializeZerothFrame() {
   m_fast_unwind_plan_sp = GetFastUnwindPlanForFrame();
   m_full_unwind_plan_sp = GetFullUnwindPlanForFrame();
 
-  UnwindPlan::RowSP active_row;
+  const UnwindPlan::Row *active_row;
   lldb::RegisterKind row_register_kind = eRegisterKindGeneric;
 
   // If we have LanguageRuntime UnwindPlan for this unwind, use those
@@ -246,15 +246,15 @@ void RegisterContextUnwind::InitializeZerothFrame() {
     active_row =
         m_full_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
     row_register_kind = m_full_unwind_plan_sp->GetRegisterKind();
-    if (active_row.get() && log) {
+    if (active_row && log) {
       StreamString active_row_strm;
       active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(), &m_thread,
                        m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
       UnwindLogMsg("%s", active_row_strm.GetData());
     }
   }
 
-  if (!active_row.get()) {
+  if (!active_row) {
     UnwindLogMsg("could not find an unwindplan row for this frame's pc");
     m_frame_type = eNotAValidFrame;
     return;
@@ -442,8 +442,8 @@ void RegisterContextUnwind::InitializeNonZerothFrame() {
       m_current_offset = -1;
       m_current_offset_backed_up_one = -1;
       RegisterKind row_register_kind = m_full_unwind_plan_sp->GetRegisterKind();
-      UnwindPlan::RowSP row = m_full_unwind_plan_sp->GetRowForFunctionOffset(0);
-      if (row.get()) {
+      if (const UnwindPlan::Row *row =
+              m_full_unwind_plan_sp->GetRowForFunctionOffset(0)) {
         if (!ReadFrameAddress(row_register_kind, row->GetCFAValue(), m_cfa)) {
           UnwindLogMsg("failed to get cfa value");
           if (m_frame_type != eSkipFrame) // don't override eSkipFrame
@@ -593,7 +593,7 @@ void RegisterContextUnwind::InitializeNonZerothFrame() {
     }
   }
 
-  UnwindPlan::RowSP active_row;
+  const UnwindPlan::Row *active_row;
   RegisterKind row_register_kind = eRegisterKindGeneric;
 
   // If we have LanguageRuntime UnwindPlan for this unwind, use those
@@ -640,7 +640,7 @@ void RegisterContextUnwind::InitializeNonZerothFrame() {
         m_fast_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
     row_register_kind = m_fast_unwind_plan_sp->GetRegisterKind();
     PropagateTrapHandlerFlagFromUnwindPlan(m_fast_unwind_plan_sp);
-    if (active_row.get() && log) {
+    if (active_row && log) {
       StreamString active_row_strm;
       active_row->Dump(active_row_strm, m_fast_unwind_plan_sp.get(), &m_thread,
                        m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
@@ -655,7 +655,7 @@ void RegisterContextUnwind::InitializeNonZerothFrame() {
           m_current_offset_backed_up_one);
       row_register_kind = m_full_unwind_plan_sp->GetRegisterKind();
       PropagateTrapHandlerFlagFromUnwindPlan(m_full_unwind_plan_sp);
-      if (active_row.get() && log) {
+      if (active_row && log) {
         StreamString active_row_strm;
         active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(),
                          &m_thread,
@@ -667,7 +667,7 @@ void RegisterContextUnwind::InitializeNonZerothFrame() {
     }
   }
 
-  if (!active_row.get()) {
+  if (!active_row) {
     m_frame_type = eNotAValidFrame;
     UnwindLogMsg("could not find unwind row for this pc");
     return;
@@ -1285,7 +1285,7 @@ RegisterContextUnwind::SavedLocationForRegister(
   RegisterKind unwindplan_registerkind = kNumRegisterKinds;
 
   if (m_fast_unwind_plan_sp) {
-    UnwindPlan::RowSP active_row =
+    const UnwindPlan::Row *active_row =
         m_fast_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
     unwindplan_registerkind = m_fast_unwind_plan_sp->GetRegisterKind();
     if (regnum.GetAsKind(unwindplan_registerkind) == LLDB_INVALID_REGNUM) {
@@ -1326,12 +1326,12 @@ RegisterContextUnwind::SavedLocationForRegister(
       RegisterNumber pc_regnum(m_thread, eRegisterKindGeneric,
                                LLDB_REGNUM_GENERIC_PC);
 
-      UnwindPlan::RowSP active_row =
+      const UnwindPlan::Row *active_row =
           m_full_unwind_plan_sp->GetRowForFunctionOffset(
               m_current_offset_backed_up_one);
       unwindplan_registerkind = m_full_unwind_plan_sp->GetRegisterKind();
 
-      if (got_new_full_unwindplan && active_row.get() && log) {
+      if (got_new_full_unwindplan && active_row && log) {
         StreamString active_row_strm;
         ExecutionContext exe_ctx(m_thread.shared_from_this());
         active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(),
@@ -1455,7 +1455,7 @@ RegisterContextUnwind::SavedLocationForRegister(
         if (ForceSwitchToFallbackUnwindPlan()) {
           // Update for the possibly new unwind plan
           unwindplan_registerkind = m_full_unwind_plan_sp->GetRegisterKind();
-          UnwindPlan::RowSP active_row =
+          const UnwindPlan::Row *active_row =
               m_full_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
 
           // Sanity check: Verify that we can fetch a pc value and CFA value
@@ -1799,7 +1799,7 @@ bool RegisterContextUnwind::TryFallbackUnwindPlan() {
 
   m_full_unwind_plan_sp = m_fallback_unwind_plan_sp;
 
-  UnwindPlan::RowSP active_row =
+  const UnwindPlan::Row *active_row =
       m_fallback_unwind_plan_sp->GetRowForFunctionOffset(
           m_current_offset_backed_up_one);
 
@@ -1885,7 +1885,7 @@ bool RegisterContextUnwind::ForceSwitchToFallbackUnwindPlan() {
     return false;
   }
 
-  UnwindPlan::RowSP active_row =
+  const UnwindPlan::Row *active_row =
       m_fallback_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
 
   if (active_row &&
@@ -1967,7 +1967,7 @@ void RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan(
 }
 
 bool RegisterContextUnwind::ReadFrameAddress(
-    lldb::RegisterKind row_register_kind, UnwindPlan::Row::FAValue &fa,
+    lldb::RegisterKind row_register_kind, const UnwindPlan::Row::FAValue &fa,
     addr_t &address) {
   RegisterValue reg_value;