[lldb] s/ValidRange/ValidRanges in UnwindPlan

To be able to describe discontinuous functions, this patch changes the
UnwindPlan to accept more than one address range.

I've also squeezed in a couple improvements/modernizations, for example
using the lower_bound function instead of a linear scan.

Author: labath

lldb/include/lldb/Symbol/UnwindPlan.h

@@ -438,7 +438,7 @@ class UnwindPlan {
 
   // Performs a deep copy of the plan, including all the rows (expensive).
   UnwindPlan(const UnwindPlan &rhs)
-      : m_plan_valid_address_range(rhs.m_plan_valid_address_range),
+      : m_plan_valid_ranges(rhs.m_plan_valid_ranges),
         m_register_kind(rhs.m_register_kind),
         m_return_addr_register(rhs.m_return_addr_register),
         m_source_name(rhs.m_source_name),
@@ -492,10 +492,8 @@ class UnwindPlan {
   // This UnwindPlan may not be valid at every address of the function span.
   // For instance, a FastUnwindPlan will not be valid at the prologue setup
   // instructions - only in the body of the function.
-  void SetPlanValidAddressRange(const AddressRange &range);
-
-  const AddressRange &GetAddressRange() const {
-    return m_plan_valid_address_range;
+  void SetPlanValidAddressRanges(std::vector<AddressRange> ranges) {
+    m_plan_valid_ranges = std::move(ranges);
   }
 
   bool PlanValidAtAddress(Address addr);
@@ -544,11 +542,11 @@ class UnwindPlan {
     m_plan_is_for_signal_trap = is_for_signal_trap;
   }
 
-  int GetRowCount() const;
+  int GetRowCount() const { return m_row_list.size(); }
 
   void Clear() {
     m_row_list.clear();
-    m_plan_valid_address_range.Clear();
+    m_plan_valid_ranges.clear();
     m_register_kind = lldb::eRegisterKindDWARF;
     m_source_name.Clear();
     m_plan_is_sourced_from_compiler = eLazyBoolCalculate;
@@ -571,9 +569,8 @@ class UnwindPlan {
   }
 
 private:
-  typedef std::vector<RowSP> collection;
-  collection m_row_list;
-  AddressRange m_plan_valid_address_range;
+  std::vector<RowSP> m_row_list;
+  std::vector<AddressRange> m_plan_valid_ranges;
   lldb::RegisterKind m_register_kind; // The RegisterKind these register numbers
                                       // are in terms of - will need to be
   // translated to lldb native reg nums at unwind time

lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp

@@ -495,9 +495,9 @@ bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range,
   for (auto it = rows.rbegin(); it != rows.rend(); ++it)
     unwind_plan.AppendRow(*it);
 
-  unwind_plan.SetPlanValidAddressRange(AddressRange(
+  unwind_plan.SetPlanValidAddressRanges({AddressRange(
       m_object_file.GetAddress(runtime_function->StartAddress),
-      runtime_function->EndAddress - runtime_function->StartAddress));
+      runtime_function->EndAddress - runtime_function->StartAddress)});
   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
 
   return true;

lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

@@ -656,9 +656,9 @@ SymbolFileBreakpad::ParseCFIUnwindPlan(const Bookmark &bookmark,
   plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
   plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
   plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
-  plan_sp->SetPlanValidAddressRange(
-      AddressRange(base + init_record->Address, *init_record->Size,
-                   m_objfile_sp->GetModule()->GetSectionList()));
+  plan_sp->SetPlanValidAddressRanges(
+      {AddressRange(base + init_record->Address, *init_record->Size,
+                    m_objfile_sp->GetModule()->GetSectionList())});
 
   auto row_sp = std::make_shared<UnwindPlan::Row>();
   row_sp->SetOffset(0);
@@ -698,9 +698,9 @@ SymbolFileBreakpad::ParseWinUnwindPlan(const Bookmark &bookmark,
   plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
   plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolNo);
   plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
-  plan_sp->SetPlanValidAddressRange(
-      AddressRange(base + record->RVA, record->CodeSize,
-                   m_objfile_sp->GetModule()->GetSectionList()));
+  plan_sp->SetPlanValidAddressRanges(
+      {AddressRange(base + record->RVA, record->CodeSize,
+                    m_objfile_sp->GetModule()->GetSectionList())});
 
   auto row_sp = std::make_shared<UnwindPlan::Row>();
   row_sp->SetOffset(0);

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

@@ -918,7 +918,7 @@ bool x86AssemblyInspectionEngine::GetNonCallSiteUnwindPlanFromAssembly(
   UnwindPlan::Row::AbstractRegisterLocation initial_regloc;
   UnwindPlan::RowSP row(new UnwindPlan::Row);
 
-  unwind_plan.SetPlanValidAddressRange(func_range);
+  unwind_plan.SetPlanValidAddressRanges({func_range});
   unwind_plan.SetRegisterKind(eRegisterKindLLDB);
 
   // At the start of the function, find the CFA by adding wordsize to the SP
@@ -1573,7 +1573,7 @@ bool x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite(
     }
   }
 
-  unwind_plan.SetPlanValidAddressRange(func_range);
+  unwind_plan.SetPlanValidAddressRanges({func_range});
   if (unwind_plan_updated) {
     std::string unwind_plan_source(unwind_plan.GetSourceName().AsCString());
     unwind_plan_source += " plus augmentation from assembly parsing";

lldb/source/Symbol/CompactUnwindInfo.cpp

@@ -206,7 +206,7 @@ bool CompactUnwindInfo::GetUnwindPlan(Target &target, Address addr,
                                   function_info.valid_range_offset_end -
                                       function_info.valid_range_offset_start,
                                   sl);
-          unwind_plan.SetPlanValidAddressRange(func_range);
+          unwind_plan.SetPlanValidAddressRanges({func_range});
         }
       }
 

lldb/source/Symbol/DWARFCallFrameInfo.cpp

@@ -623,7 +623,7 @@ bool DWARFCallFrameInfo::FDEToUnwindPlan(dw_offset_t dwarf_offset,
   uint32_t code_align = cie->code_align;
   int32_t data_align = cie->data_align;
 
-  unwind_plan.SetPlanValidAddressRange(range);
+  unwind_plan.SetPlanValidAddressRanges({range});
   UnwindPlan::Row *cie_initial_row = new UnwindPlan::Row;
   *cie_initial_row = cie->initial_row;
   UnwindPlan::RowSP row(cie_initial_row);

lldb/source/Symbol/UnwindPlan.cpp

@@ -15,6 +15,7 @@
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/DebugInfo/DIContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include <optional>
@@ -397,36 +398,35 @@ void UnwindPlan::AppendRow(const UnwindPlan::RowSP &row_sp) {
     m_row_list.back() = row_sp;
 }
 
+struct RowLess {
+  bool operator()(addr_t a, const UnwindPlan::RowSP &b) const {
+    return a < b->GetOffset();
+  }
+  bool operator()(const UnwindPlan::RowSP &a, addr_t b) const {
+    return a->GetOffset() < b;
+  }
+};
+
 void UnwindPlan::InsertRow(const UnwindPlan::RowSP &row_sp,
                            bool replace_existing) {
-  collection::iterator it = m_row_list.begin();
-  while (it != m_row_list.end()) {
-    RowSP row = *it;
-    if (row->GetOffset() >= row_sp->GetOffset())
-      break;
-    it++;
-  }
-  if (it == m_row_list.end() || (*it)->GetOffset() != row_sp->GetOffset())
+  auto it = llvm::lower_bound(m_row_list, row_sp->GetOffset(), RowLess());
+  if (it == m_row_list.end() || it->get()->GetOffset() > row_sp->GetOffset())
     m_row_list.insert(it, row_sp);
-  else if (replace_existing)
-    *it = row_sp;
+  else {
+    assert(it->get()->GetOffset() == row_sp->GetOffset());
+    if (replace_existing)
+      *it = row_sp;
+  }
 }
 
 const UnwindPlan::Row *UnwindPlan::GetRowForFunctionOffset(int offset) const {
-  if (m_row_list.empty())
+  auto it = offset == -1 ? m_row_list.end()
+                         : llvm::upper_bound(m_row_list, offset, RowLess());
+  if (it == m_row_list.begin())
     return nullptr;
-  if (offset == -1)
-    return m_row_list.back().get();
-
-  RowSP row;
-  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.get();
+  // upper_bound returns the row strictly greater than our desired offset, which
+  // means that the row before it is a match.
+  return std::prev(it)->get();
 }
 
 bool UnwindPlan::IsValidRowIndex(uint32_t idx) const {
@@ -445,20 +445,13 @@ const UnwindPlan::Row *UnwindPlan::GetRowAtIndex(uint32_t idx) 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");
+    LLDB_LOG(GetLog(LLDBLog::Unwind),
+             "UnwindPlan::GetLastRow() when rows are empty");
     return nullptr;
   }
   return m_row_list.back().get();
 }
 
-int UnwindPlan::GetRowCount() const { return m_row_list.size(); }
-
-void UnwindPlan::SetPlanValidAddressRange(const AddressRange &range) {
-  if (range.GetBaseAddress().IsValid() && range.GetByteSize() != 0)
-    m_plan_valid_address_range = range;
-}
-
 bool UnwindPlan::PlanValidAtAddress(Address addr) {
   // If this UnwindPlan has no rows, it is an invalid UnwindPlan.
   if (GetRowCount() == 0) {
@@ -482,9 +475,9 @@ 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) == nullptr ||
-      GetRowAtIndex(0)->GetCFAValue().GetValueType() ==
-          Row::FAValue::unspecified) {
+  const Row *row0 = GetRowForFunctionOffset(0);
+  if (!row0 ||
+      row0->GetCFAValue().GetValueType() == Row::FAValue::unspecified) {
     Log *log = GetLog(LLDBLog::Unwind);
     if (log) {
       StreamString s;
@@ -503,17 +496,15 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) {
     return false;
   }
 
-  if (!m_plan_valid_address_range.GetBaseAddress().IsValid() ||
-      m_plan_valid_address_range.GetByteSize() == 0)
+  if (m_plan_valid_ranges.empty())
     return true;
 
   if (!addr.IsValid())
     return true;
 
-  if (m_plan_valid_address_range.ContainsFileAddress(addr))
-    return true;
-
-  return false;
+  return llvm::any_of(m_plan_valid_ranges, [&](const AddressRange &range) {
+    return range.ContainsFileAddress(addr);
+  });
 }
 
 void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
@@ -570,20 +561,17 @@ void UnwindPlan::Dump(Stream &s, Thread *thread, lldb::addr_t base_addr) const {
     s.Printf("not specified.\n");
     break;
   }
-  if (m_plan_valid_address_range.GetBaseAddress().IsValid() &&
-      m_plan_valid_address_range.GetByteSize() > 0) {
+  if (!m_plan_valid_ranges.empty()) {
     s.PutCString("Address range of this UnwindPlan: ");
     TargetSP target_sp(thread->CalculateTarget());
-    m_plan_valid_address_range.Dump(&s, target_sp.get(),
-                                    Address::DumpStyleSectionNameOffset);
+    for (const AddressRange &range : m_plan_valid_ranges)
+      range.Dump(&s, target_sp.get(), Address::DumpStyleSectionNameOffset);
     s.EOL();
   }
-  collection::const_iterator pos, begin = m_row_list.begin(),
-                                  end = m_row_list.end();
-  for (pos = begin; pos != end; ++pos) {
-    s.Printf("row[%u]: ", (uint32_t)std::distance(begin, pos));
-    (*pos)->Dump(s, this, thread, base_addr);
-    s.Printf("\n");
+  for (const auto &[index, row_sp] : llvm::enumerate(m_row_list)) {
+    s.Format("row[{0}]: ", index);
+    row_sp->Dump(s, this, thread, base_addr);
+    s << "\n";
   }
 }
 

lldb/unittests/Symbol/CMakeLists.txt

@@ -12,6 +12,7 @@ add_lldb_unittest(SymbolTests
   TestDWARFCallFrameInfo.cpp
   TestType.cpp
   TestLineEntry.cpp
+  UnwindPlanTest.cpp
 
   LINK_LIBS
     lldbCore

lldb/unittests/Symbol/UnwindPlanTest.cpp

@@ -0,0 +1,76 @@
+//===-- UnwindPlanTest.cpp ------------------------------------------------===//
+//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Symbol/UnwindPlan.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+static UnwindPlan::RowSP make_simple_row(addr_t offset, uint64_t cfa_value) {
+  UnwindPlan::RowSP row_sp = std::make_shared<UnwindPlan::Row>();
+  row_sp->SetOffset(offset);
+  row_sp->GetCFAValue().SetIsConstant(cfa_value);
+  return row_sp;
+}
+
+TEST(UnwindPlan, InsertRow) {
+  UnwindPlan::RowSP row1_sp = make_simple_row(0, 42);
+  UnwindPlan::RowSP row2_sp = make_simple_row(0, 47);
+
+  UnwindPlan plan(eRegisterKindGeneric);
+  plan.InsertRow(row1_sp);
+  EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(*row1_sp));
+
+  plan.InsertRow(row2_sp, /*replace_existing=*/false);
+  EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(*row1_sp));
+
+  plan.InsertRow(row2_sp, /*replace_existing=*/true);
+  EXPECT_THAT(plan.GetRowForFunctionOffset(0), testing::Pointee(*row2_sp));
+}
+
+TEST(UnwindPlan, GetRowForFunctionOffset) {
+  UnwindPlan::RowSP row1_sp = make_simple_row(10, 42);
+  UnwindPlan::RowSP row2_sp = make_simple_row(20, 47);
+
+  UnwindPlan plan(eRegisterKindGeneric);
+  plan.InsertRow(row1_sp);
+  plan.InsertRow(row2_sp);
+
+  EXPECT_THAT(plan.GetRowForFunctionOffset(0), nullptr);
+  EXPECT_THAT(plan.GetRowForFunctionOffset(9), nullptr);
+  EXPECT_THAT(plan.GetRowForFunctionOffset(10), testing::Pointee(*row1_sp));
+  EXPECT_THAT(plan.GetRowForFunctionOffset(19), testing::Pointee(*row1_sp));
+  EXPECT_THAT(plan.GetRowForFunctionOffset(20), testing::Pointee(*row2_sp));
+  EXPECT_THAT(plan.GetRowForFunctionOffset(99), testing::Pointee(*row2_sp));
+}
+
+TEST(UnwindPlan, PlanValidAtAddress) {
+  UnwindPlan::RowSP row1_sp = make_simple_row(0, 42);
+  UnwindPlan::RowSP row2_sp = make_simple_row(10, 47);
+
+  UnwindPlan plan(eRegisterKindGeneric);
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
+
+  plan.InsertRow(row2_sp);
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
+
+  plan.InsertRow(row1_sp);
+  EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
+  EXPECT_TRUE(plan.PlanValidAtAddress(Address(10)));
+
+  plan.SetPlanValidAddressRanges({AddressRange(0, 5), AddressRange(15, 5)});
+  EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(5)));
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(10)));
+  EXPECT_TRUE(plan.PlanValidAtAddress(Address(15)));
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(20)));
+  EXPECT_FALSE(plan.PlanValidAtAddress(Address(25)));
+}

lldb/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

@@ -2248,7 +2248,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSpArithx86_64Augmented) {
   sample_range = AddressRange(0x1000, sizeof(data));
 
   unwind_plan.SetSourceName("unit testing hand-created unwind plan");
-  unwind_plan.SetPlanValidAddressRange(sample_range);
+  unwind_plan.SetPlanValidAddressRanges({sample_range});
   unwind_plan.SetRegisterKind(eRegisterKindLLDB);
 
   row_sp = std::make_shared<UnwindPlan::Row>();
@@ -2335,7 +2335,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSimplex86_64Augmented) {
   sample_range = AddressRange(0x1000, sizeof(data));
 
   unwind_plan.SetSourceName("unit testing hand-created unwind plan");
-  unwind_plan.SetPlanValidAddressRange(sample_range);
+  unwind_plan.SetPlanValidAddressRanges({sample_range});
   unwind_plan.SetRegisterKind(eRegisterKindLLDB);
 
   row_sp = std::make_shared<UnwindPlan::Row>();
@@ -2413,7 +2413,7 @@ TEST_F(Testx86AssemblyInspectionEngine, TestSimplei386ugmented) {
   sample_range = AddressRange(0x1000, sizeof(data));
 
   unwind_plan.SetSourceName("unit testing hand-created unwind plan");
-  unwind_plan.SetPlanValidAddressRange(sample_range);
+  unwind_plan.SetPlanValidAddressRanges({sample_range});
   unwind_plan.SetRegisterKind(eRegisterKindLLDB);
 
   row_sp = std::make_shared<UnwindPlan::Row>();