[lldb] Disable some unwind plans for discontinuous functions (#140927)

Basically, disable everything except the eh_frame unwind plan, as that's
the only one which supports this right now. The other plans are working
with now trying the interpret everything in between the function parts
as a part of the function, which is more likely to produce wrong results
than correct ones.

I changed the interface for object file plans, to give the
implementations a chance to implement this correctly, but I haven't
actually converted PECallFrameInfo (its only implementation) to handle
that. (from the looks of things, it should be relatively easy to do, if
it becomes necessary)

I'm also deleting UnwindPlan::GetFirstNonPrologueInsn, as it's not used,
and it doesn't work for discontinuous functions.

lldb/include/lldb/Symbol/CallFrameInfo.h

@@ -19,8 +19,10 @@ public:
 
   virtual bool GetAddressRange(Address addr, AddressRange &range) = 0;
 
-  virtual bool GetUnwindPlan(const Address &addr, UnwindPlan &unwind_plan) = 0;
-  virtual bool GetUnwindPlan(const AddressRange &range, UnwindPlan &unwind_plan) = 0;
+  virtual std::unique_ptr<UnwindPlan>
+  GetUnwindPlan(llvm::ArrayRef<AddressRange> ranges, const Address &addr) = 0;
+
+  virtual std::unique_ptr<UnwindPlan> GetUnwindPlan(const Address &addr) = 0;
 };
 
 } // namespace lldb_private

lldb/include/lldb/Symbol/FuncUnwinders.h

@@ -51,8 +51,6 @@ public:
   std::shared_ptr<const UnwindPlan>
   GetUnwindPlanArchitectureDefaultAtFunctionEntry(lldb_private::Thread &thread);
 
-  Address &GetFirstNonPrologueInsn(Target &target);
-
   const Address &GetFunctionStartAddress() const;
 
   bool ContainsAddress(const Address &addr) const {
@@ -114,10 +112,6 @@ private:
   /// The address ranges of the function.
   AddressRanges m_ranges;
 
-  /// The smallest address range covering the entire function.
-  /// DEPRECATED: Use m_ranges instead.
-  AddressRange m_range;
-
   std::recursive_mutex m_mutex;
 
   std::shared_ptr<const UnwindPlan> m_unwind_plan_assembly_sp;

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

@@ -455,27 +455,26 @@ bool PECallFrameInfo::GetAddressRange(Address addr, AddressRange &range) {
   return true;
 }
 
-bool PECallFrameInfo::GetUnwindPlan(const Address &addr,
-                                    UnwindPlan &unwind_plan) {
-  return GetUnwindPlan(AddressRange(addr, 1), unwind_plan);
-}
-
-bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range,
-                                    UnwindPlan &unwind_plan) {
-  unwind_plan.Clear();
-
-  unwind_plan.SetSourceName("PE EH info");
-  unwind_plan.SetSourcedFromCompiler(eLazyBoolYes);
-  unwind_plan.SetRegisterKind(eRegisterKindLLDB);
+std::unique_ptr<UnwindPlan> PECallFrameInfo::GetUnwindPlan(
+    llvm::ArrayRef<lldb_private::AddressRange> ranges,
+    const lldb_private::Address &addr) {
+  // Only continuous functions are supported.
+  if (ranges.size() != 1)
+    return nullptr;
+  const AddressRange &range = ranges[0];
 
   const RuntimeFunction *runtime_function =
       FindRuntimeFunctionIntersectsWithRange(range);
   if (!runtime_function)
-    return false;
+    return nullptr;
+
+  auto plan_up = std::make_unique<UnwindPlan>(eRegisterKindLLDB);
+  plan_up->SetSourceName("PE EH info");
+  plan_up->SetSourcedFromCompiler(eLazyBoolYes);
 
   EHProgramBuilder builder(m_object_file, runtime_function->UnwindInfoOffset);
   if (!builder.Build())
-    return false;
+    return nullptr;
 
   std::vector<UnwindPlan::Row> rows;
 
@@ -493,14 +492,14 @@ bool PECallFrameInfo::GetUnwindPlan(const AddressRange &range,
   }
 
   for (auto it = rows.rbegin(); it != rows.rend(); ++it)
-    unwind_plan.AppendRow(std::move(*it));
+    plan_up->AppendRow(std::move(*it));
 
-  unwind_plan.SetPlanValidAddressRanges({AddressRange(
+  plan_up->SetPlanValidAddressRanges({AddressRange(
       m_object_file.GetAddress(runtime_function->StartAddress),
       runtime_function->EndAddress - runtime_function->StartAddress)});
-  unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  plan_up->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
 
-  return true;
+  return plan_up;
 }
 
 const RuntimeFunction *PECallFrameInfo::FindRuntimeFunctionIntersectsWithRange(

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

@@ -9,7 +9,9 @@
 #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_PECOFF_PECALLFRAMEINFO_H
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_PECOFF_PECALLFRAMEINFO_H
 
+#include "lldb/Core/AddressRange.h"
 #include "lldb/Symbol/CallFrameInfo.h"
+#include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Utility/DataExtractor.h"
 
 class ObjectFilePECOFF;
@@ -31,10 +33,14 @@ public:
   bool GetAddressRange(lldb_private::Address addr,
                        lldb_private::AddressRange &range) override;
 
-  bool GetUnwindPlan(const lldb_private::Address &addr,
-                     lldb_private::UnwindPlan &unwind_plan) override;
-  bool GetUnwindPlan(const lldb_private::AddressRange &range,
-                     lldb_private::UnwindPlan &unwind_plan) override;
+  std::unique_ptr<lldb_private::UnwindPlan>
+  GetUnwindPlan(const lldb_private::Address &addr) override {
+    return GetUnwindPlan({lldb_private::AddressRange(addr, 1)}, addr);
+  }
+
+  std::unique_ptr<lldb_private::UnwindPlan>
+  GetUnwindPlan(llvm::ArrayRef<lldb_private::AddressRange> ranges,
+                const lldb_private::Address &addr) override;
 
 private:
   const llvm::Win64EH::RuntimeFunction *FindRuntimeFunctionIntersectsWithRange(

lldb/source/Symbol/FuncUnwinders.cpp

@@ -31,30 +31,11 @@
 using namespace lldb;
 using namespace lldb_private;
 
-static AddressRange CollapseRanges(llvm::ArrayRef<AddressRange> ranges) {
-  if (ranges.empty())
-    return AddressRange();
-  if (ranges.size() == 1)
-    return ranges[0];
-
-  Address lowest_addr = ranges[0].GetBaseAddress();
-  addr_t highest_addr = lowest_addr.GetFileAddress() + ranges[0].GetByteSize();
-  for (const AddressRange &range : ranges.drop_front()) {
-    Address range_begin = range.GetBaseAddress();
-    addr_t range_end = range_begin.GetFileAddress() + range.GetByteSize();
-    if (range_begin.GetFileAddress() < lowest_addr.GetFileAddress())
-      lowest_addr = range_begin;
-    if (range_end > highest_addr)
-      highest_addr = range_end;
-  }
-  return AddressRange(lowest_addr, highest_addr - lowest_addr.GetFileAddress());
-}
-
 FuncUnwinders::FuncUnwinders(UnwindTable &unwind_table, Address addr,
                              AddressRanges ranges)
     : m_unwind_table(unwind_table), m_addr(std::move(addr)),
-      m_ranges(std::move(ranges)), m_range(CollapseRanges(m_ranges)),
-      m_tried_unwind_plan_assembly(false), m_tried_unwind_plan_eh_frame(false),
+      m_ranges(std::move(ranges)), m_tried_unwind_plan_assembly(false),
+      m_tried_unwind_plan_eh_frame(false),
       m_tried_unwind_plan_object_file(false),
       m_tried_unwind_plan_debug_frame(false),
       m_tried_unwind_plan_object_file_augmented(false),
@@ -106,8 +87,9 @@ FuncUnwinders::GetCompactUnwindUnwindPlan(Target &target) {
     return nullptr;
 
   m_tried_unwind_plan_compact_unwind = true;
-  if (m_range.GetBaseAddress().IsValid()) {
-    Address current_pc(m_range.GetBaseAddress());
+  // Only continuous functions are supported.
+  if (m_ranges.size() == 1) {
+    Address current_pc(m_ranges[0].GetBaseAddress());
     CompactUnwindInfo *compact_unwind = m_unwind_table.GetCompactUnwindInfo();
     if (compact_unwind) {
       auto unwind_plan_sp =
@@ -131,14 +113,10 @@ FuncUnwinders::GetObjectFileUnwindPlan(Target &target) {
     return m_unwind_plan_object_file_sp;
 
   m_tried_unwind_plan_object_file = true;
-  if (m_range.GetBaseAddress().IsValid()) {
-    CallFrameInfo *object_file_frame = m_unwind_table.GetObjectFileUnwindInfo();
-    if (object_file_frame) {
-      auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric);
-      if (object_file_frame->GetUnwindPlan(m_range, *plan_sp))
-        m_unwind_plan_object_file_sp = std::move(plan_sp);
-    }
-  }
+  if (CallFrameInfo *object_file_frame =
+          m_unwind_table.GetObjectFileUnwindInfo())
+    m_unwind_plan_object_file_sp =
+        object_file_frame->GetUnwindPlan(m_ranges, m_addr);
   return m_unwind_plan_object_file_sp;
 }
 
@@ -178,8 +156,9 @@ FuncUnwinders::GetArmUnwindUnwindPlan(Target &target) {
     return m_unwind_plan_arm_unwind_sp;
 
   m_tried_unwind_plan_arm_unwind = true;
-  if (m_range.GetBaseAddress().IsValid()) {
-    Address current_pc(m_range.GetBaseAddress());
+  // Only continuous functions are supported.
+  if (m_ranges.size() == 1) {
+    Address current_pc = m_ranges[0].GetBaseAddress();
     ArmUnwindInfo *arm_unwind_info = m_unwind_table.GetArmUnwindInfo();
     if (arm_unwind_info) {
       auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric);
@@ -215,9 +194,10 @@ FuncUnwinders::GetSymbolFileUnwindPlan(Thread &thread) {
     return m_unwind_plan_symbol_file_sp;
 
   m_tried_unwind_plan_symbol_file = true;
-  if (SymbolFile *symfile = m_unwind_table.GetSymbolFile()) {
+  if (SymbolFile *symfile = m_unwind_table.GetSymbolFile();
+      symfile && m_ranges.size() == 1) {
     m_unwind_plan_symbol_file_sp = symfile->GetUnwindPlan(
-        m_range.GetBaseAddress(),
+        m_ranges[0].GetBaseAddress(),
         RegisterContextToInfo(*thread.GetRegisterContext()));
   }
   return m_unwind_plan_symbol_file_sp;
@@ -242,10 +222,11 @@ FuncUnwinders::GetObjectFileAugmentedUnwindPlan(Target &target,
   // so the UnwindPlan can be used at any instruction in the function.
 
   UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target));
-  if (assembly_profiler_sp) {
+  // Only continuous functions are supported.
+  if (assembly_profiler_sp && m_ranges.size() == 1) {
     auto plan_sp = std::make_shared<UnwindPlan>(*object_file_unwind_plan);
 
-    if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_range, thread,
+    if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_ranges[0], thread,
                                                             *plan_sp))
       m_unwind_plan_object_file_augmented_sp = std::move(plan_sp);
   }
@@ -280,9 +261,10 @@ FuncUnwinders::GetEHFrameAugmentedUnwindPlan(Target &target, Thread &thread) {
   // so the UnwindPlan can be used at any instruction in the function.
 
   UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target));
-  if (assembly_profiler_sp) {
+  // Only continuous functions are supported.
+  if (assembly_profiler_sp && m_ranges.size() == 1) {
     auto plan_sp = std::make_shared<UnwindPlan>(*eh_frame_plan);
-    if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_range, thread,
+    if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_ranges[0], thread,
                                                             *plan_sp))
       m_unwind_plan_eh_frame_augmented_sp = std::move(plan_sp);
   }
@@ -319,10 +301,11 @@ FuncUnwinders::GetDebugFrameAugmentedUnwindPlan(Target &target,
   // function.
 
   UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target));
-  if (assembly_profiler_sp) {
+  // Only continuous functions are supported.
+  if (assembly_profiler_sp && m_ranges.size() == 1) {
     auto plan_sp = std::make_shared<UnwindPlan>(*debug_frame_plan);
 
-    if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_range, thread,
+    if (assembly_profiler_sp->AugmentUnwindPlanFromCallSite(m_ranges[0], thread,
                                                             *plan_sp))
       m_unwind_plan_debug_frame_augmented_sp = std::move(plan_sp);
   }
@@ -339,18 +322,19 @@ FuncUnwinders::GetAssemblyUnwindPlan(Target &target, Thread &thread) {
 
   m_tried_unwind_plan_assembly = true;
 
-  // Don't analyze more than 10 megabytes of instructions,
-  // if a function is legitimately larger than that, we'll
-  // miss the epilogue instructions, but guard against a
-  // bogusly large function and analyzing large amounts of
-  // non-instruction data.
-  AddressRange range = m_range;
-  const addr_t func_size =
-      std::min(range.GetByteSize(), (addr_t)1024 * 10 * 10);
-  range.SetByteSize(func_size);
-
   UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target));
-  if (assembly_profiler_sp) {
+  // Only continuous functions are supported.
+  if (assembly_profiler_sp && m_ranges.size() == 1) {
+    // Don't analyze more than 10 megabytes of instructions,
+    // if a function is legitimately larger than that, we'll
+    // miss the epilogue instructions, but guard against a
+    // bogusly large function and analyzing large amounts of
+    // non-instruction data.
+    AddressRange range = m_ranges[0];
+    const addr_t func_size =
+        std::min(range.GetByteSize(), (addr_t)1024 * 10 * 10);
+    range.SetByteSize(func_size);
+
     auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric);
     if (assembly_profiler_sp->GetNonCallSiteUnwindPlanFromAssembly(
             range, thread, *plan_sp))
@@ -457,9 +441,9 @@ FuncUnwinders::GetUnwindPlanFastUnwind(Target &target, Thread &thread) {
   m_tried_unwind_fast = true;
 
   UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target));
-  if (assembly_profiler_sp) {
+  if (assembly_profiler_sp && m_ranges.size() == 1) {
     auto plan_sp = std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric);
-    if (assembly_profiler_sp->GetFastUnwindPlan(m_range, thread, *plan_sp))
+    if (assembly_profiler_sp->GetFastUnwindPlan(m_ranges[0], thread, *plan_sp))
       m_unwind_plan_fast_sp = std::move(plan_sp);
   }
   return m_unwind_plan_fast_sp;
@@ -503,19 +487,6 @@ FuncUnwinders::GetUnwindPlanArchitectureDefaultAtFunctionEntry(Thread &thread) {
   return m_unwind_plan_arch_default_at_func_entry_sp;
 }
 
-Address &FuncUnwinders::GetFirstNonPrologueInsn(Target &target) {
-  std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  if (m_first_non_prologue_insn.IsValid())
-    return m_first_non_prologue_insn;
-
-  ExecutionContext exe_ctx(target.shared_from_this(), false);
-  UnwindAssemblySP assembly_profiler_sp(GetUnwindAssemblyProfiler(target));
-  if (assembly_profiler_sp)
-    assembly_profiler_sp->FirstNonPrologueInsn(m_range, exe_ctx,
-                                               m_first_non_prologue_insn);
-  return m_first_non_prologue_insn;
-}
-
 const Address &FuncUnwinders::GetFunctionStartAddress() const { return m_addr; }
 
 lldb::UnwindAssemblySP

lldb/source/Target/RegisterContextUnwind.cpp

@@ -880,10 +880,9 @@ RegisterContextUnwind::GetFullUnwindPlanForFrame() {
     CallFrameInfo *object_file_unwind =
         pc_module_sp->GetUnwindTable().GetObjectFileUnwindInfo();
     if (object_file_unwind) {
-      auto unwind_plan_sp =
-          std::make_shared<UnwindPlan>(lldb::eRegisterKindGeneric);
-      if (object_file_unwind->GetUnwindPlan(m_current_pc, *unwind_plan_sp))
-        return unwind_plan_sp;
+      if (std::unique_ptr<UnwindPlan> plan_up =
+              object_file_unwind->GetUnwindPlan(m_current_pc))
+        return plan_up;
     }
 
     return arch_default_unwind_plan_sp;

lldb/unittests/ObjectFile/PECOFF/TestPECallFrameInfo.cpp

@@ -24,12 +24,10 @@ using namespace lldb;
 
 class PECallFrameInfoTest : public testing::Test {
   SubsystemRAII<FileSystem, ObjectFilePECOFF> subsystems;
-
-protected:
-  void GetUnwindPlan(addr_t file_addr, UnwindPlan &plan) const;
 };
 
-void PECallFrameInfoTest::GetUnwindPlan(addr_t file_addr, UnwindPlan &plan) const {
+static llvm::Expected<std::unique_ptr<UnwindPlan>>
+GetUnwindPlan(addr_t file_addr) {
   llvm::Expected<TestFile> ExpectedFile = TestFile::fromYaml(
       R"(
 --- !COFF
@@ -190,24 +188,34 @@ sections:
 symbols:         []
 ...
 )");
-  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+  if (!ExpectedFile)
+    return ExpectedFile.takeError();
 
   ModuleSP module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec());
   ObjectFile *object_file = module_sp->GetObjectFile();
-  ASSERT_NE(object_file, nullptr);
+  if (!object_file)
+    return llvm::createStringError("object file is null");
 
   std::unique_ptr<CallFrameInfo> cfi = object_file->CreateCallFrameInfo();
-  ASSERT_NE(cfi.get(), nullptr);
+  if (!cfi)
+    return llvm::createStringError("call frame info is null");
 
   SectionList *sect_list = object_file->GetSectionList();
-  ASSERT_NE(sect_list, nullptr);
+  if (!sect_list)
+    return llvm::createStringError("section list is null");
 
-  EXPECT_TRUE(cfi->GetUnwindPlan(Address(file_addr, sect_list), plan));
+  std::unique_ptr<UnwindPlan> plan_up =
+      cfi->GetUnwindPlan(Address(file_addr, sect_list));
+  if (!plan_up)
+    return llvm::createStringError("unwind plan is null");
+  return plan_up;
 }
 
 TEST_F(PECallFrameInfoTest, Basic_eh) {
-  UnwindPlan plan(eRegisterKindLLDB);
-  GetUnwindPlan(0x1001080, plan);
+  llvm::Expected<std::unique_ptr<UnwindPlan>> expected_plan =
+      GetUnwindPlan(0x1001080);
+  ASSERT_THAT_EXPECTED(expected_plan, llvm::Succeeded());
+  UnwindPlan &plan = **expected_plan;
   EXPECT_EQ(plan.GetRowCount(), 7);
 
   UnwindPlan::Row row;
@@ -248,8 +256,10 @@ TEST_F(PECallFrameInfoTest, Basic_eh) {
 }
 
 TEST_F(PECallFrameInfoTest, Chained_eh) {
-  UnwindPlan plan(eRegisterKindLLDB);
-  GetUnwindPlan(0x1001180, plan);
+  llvm::Expected<std::unique_ptr<UnwindPlan>> expected_plan =
+      GetUnwindPlan(0x1001180);
+  ASSERT_THAT_EXPECTED(expected_plan, llvm::Succeeded());
+  UnwindPlan &plan = **expected_plan;
   EXPECT_EQ(plan.GetRowCount(), 2);
 
   UnwindPlan::Row row;
@@ -270,8 +280,10 @@ TEST_F(PECallFrameInfoTest, Chained_eh) {
 }
 
 TEST_F(PECallFrameInfoTest, Frame_reg_eh) {
-  UnwindPlan plan(eRegisterKindLLDB);
-  GetUnwindPlan(0x1001280, plan);
+  llvm::Expected<std::unique_ptr<UnwindPlan>> expected_plan =
+      GetUnwindPlan(0x1001280);
+  ASSERT_THAT_EXPECTED(expected_plan, llvm::Succeeded());
+  UnwindPlan &plan = **expected_plan;
   EXPECT_EQ(plan.GetRowCount(), 11);
 
   UnwindPlan::Row row;