Fix pc/function name for signal handler frame.

This refactors the step function slightly to split it up into
distinct pieces since the code needs to handle a signal handler
versus normal step slightly differently.

Add a new error for an invalid elf.

Modify libbacktrace code to handle new error code.

Bug: 130302288

Test: libbacktrace/libunwindstack unit tests.
Change-Id: I3fb9b00c02d2cf2cc5911541bba0346c6f39b8e6
Merged-In: I3fb9b00c02d2cf2cc5911541bba0346c6f39b8e6
(cherry picked from commit d11ed86d65e870c5ea0d4918693376d474dbfe7d)

11 files changed, 82 insertions, 58 deletions

diff --git a/libbacktrace/Backtrace.cpp b/libbacktrace/Backtrace.cpp
index 6bec63c..71980d7 100644
--- a/libbacktrace/Backtrace.cpp
+++ b/libbacktrace/Backtrace.cpp
@@ -170,5 +170,7 @@ std::string Backtrace::GetErrorString(BacktraceUnwindError error) {
       return "Failed to unwind due to invalid unwind information";
     case BACKTRACE_UNWIND_ERROR_REPEATED_FRAME:
       return "Failed to unwind due to same sp/pc repeating";
+    case BACKTRACE_UNWIND_ERROR_INVALID_ELF:
+      return "Failed to unwind due to invalid elf";
   }
 }
diff --git a/libbacktrace/UnwindStack.cpp b/libbacktrace/UnwindStack.cpp
index f5f9b2a..36640cd 100644
--- a/libbacktrace/UnwindStack.cpp
+++ b/libbacktrace/UnwindStack.cpp
@@ -89,6 +89,10 @@ bool Backtrace::Unwind(unwindstack::Regs* regs, BacktraceMap* back_map,
       case unwindstack::ERROR_REPEATED_FRAME:
         error->error_code = BACKTRACE_UNWIND_ERROR_REPEATED_FRAME;
         break;
+
+      case unwindstack::ERROR_INVALID_ELF:
+        error->error_code = BACKTRACE_UNWIND_ERROR_INVALID_ELF;
+        break;
     }
   }
 
diff --git a/libbacktrace/include/backtrace/Backtrace.h b/libbacktrace/include/backtrace/Backtrace.h
index 10e790b..404e7e8 100644
--- a/libbacktrace/include/backtrace/Backtrace.h
+++ b/libbacktrace/include/backtrace/Backtrace.h
@@ -64,6 +64,8 @@ enum BacktraceUnwindErrorCode : uint32_t {
   BACKTRACE_UNWIND_ERROR_UNWIND_INFO,
   // Unwind information stopped due to sp/pc repeating.
   BACKTRACE_UNWIND_ERROR_REPEATED_FRAME,
+  // Unwind information stopped due to invalid elf.
+  BACKTRACE_UNWIND_ERROR_INVALID_ELF,
 };
 
 struct BacktraceUnwindError {
diff --git a/libunwindstack/Elf.cpp b/libunwindstack/Elf.cpp
index 4b93abb..3454913 100644
--- a/libunwindstack/Elf.cpp
+++ b/libunwindstack/Elf.cpp
@@ -160,7 +160,7 @@ ErrorCode Elf::GetLastErrorCode() {
   if (valid_) {
     return interface_->LastErrorCode();
   }
-  return ERROR_NONE;
+  return ERROR_INVALID_ELF;
 }
 
 uint64_t Elf::GetLastErrorAddress() {
@@ -170,22 +170,23 @@ uint64_t Elf::GetLastErrorAddress() {
   return 0;
 }
 
-// The relative pc is always relative to the start of the map from which it comes.
-bool Elf::Step(uint64_t rel_pc, uint64_t adjusted_rel_pc, Regs* regs, Memory* process_memory,
-               bool* finished) {
+// The relative pc expectd by this function is relative to the start of the elf.
+bool Elf::StepIfSignalHandler(uint64_t rel_pc, Regs* regs, Memory* process_memory) {
   if (!valid_) {
     return false;
   }
+  return regs->StepIfSignalHandler(rel_pc, this, process_memory);
+}
 
-  // The relative pc expectd by StepIfSignalHandler is relative to the start of the elf.
-  if (regs->StepIfSignalHandler(rel_pc, this, process_memory)) {
-    *finished = false;
-    return true;
+// The relative pc is always relative to the start of the map from which it comes.
+bool Elf::Step(uint64_t rel_pc, Regs* regs, Memory* process_memory, bool* finished) {
+  if (!valid_) {
+    return false;
   }
 
   // Lock during the step which can update information in the object.
   std::lock_guard<std::mutex> guard(lock_);
-  return interface_->Step(adjusted_rel_pc, regs, process_memory, finished);
+  return interface_->Step(rel_pc, regs, process_memory, finished);
 }
 
 bool Elf::IsValidElf(Memory* memory) {
diff --git a/libunwindstack/LocalUnwinder.cpp b/libunwindstack/LocalUnwinder.cpp
index 5b2fadf..5d81200 100644
--- a/libunwindstack/LocalUnwinder.cpp
+++ b/libunwindstack/LocalUnwinder.cpp
@@ -111,6 +111,14 @@ bool LocalUnwinder::Unwind(std::vector<LocalFrameData>* frame_info, size_t max_f
       pc_adjustment = 0;
     }
     step_pc -= pc_adjustment;
+
+    bool finished = false;
+    if (elf->StepIfSignalHandler(rel_pc, regs.get(), process_memory_.get())) {
+      step_pc = rel_pc;
+    } else if (!elf->Step(step_pc, regs.get(), process_memory_.get(), &finished)) {
+      finished = true;
+    }
+
     // Skip any locations that are within this library.
     if (num_frames != 0 || !ShouldSkipLibrary(map_info->name)) {
       // Add frame information.
@@ -124,22 +132,12 @@ bool LocalUnwinder::Unwind(std::vector<LocalFrameData>* frame_info, size_t max_f
       }
       num_frames++;
     }
-    if (!elf->valid()) {
-      break;
-    }
-    if (frame_info->size() == max_frames) {
-      break;
-    }
 
-    adjust_pc = true;
-    bool finished;
-    if (!elf->Step(rel_pc, step_pc, regs.get(), process_memory_.get(), &finished) || finished) {
-      break;
-    }
-    // pc and sp are the same, terminate the unwind.
-    if (cur_pc == regs->pc() && cur_sp == regs->sp()) {
+    if (finished || frame_info->size() == max_frames ||
+        (cur_pc == regs->pc() && cur_sp == regs->sp())) {
       break;
     }
+    adjust_pc = true;
   }
   return num_frames != 0;
 }
diff --git a/libunwindstack/Unwinder.cpp b/libunwindstack/Unwinder.cpp
index 3f2e1c1..f3d2b5e 100644
--- a/libunwindstack/Unwinder.cpp
+++ b/libunwindstack/Unwinder.cpp
@@ -89,8 +89,8 @@ void Unwinder::FillInDexFrame() {
 #endif
 }
 
-void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_t func_pc,
-                           uint64_t pc_adjustment) {
+FrameData* Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc,
+                                 uint64_t pc_adjustment) {
   size_t frame_num = frames_.size();
   frames_.resize(frame_num + 1);
   FrameData* frame = &frames_.at(frame_num);
@@ -100,7 +100,8 @@ void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_
   frame->pc = regs_->pc() - pc_adjustment;
 
   if (map_info == nullptr) {
-    return;
+    // Nothing else to update.
+    return nullptr;
   }
 
   if (resolve_names_) {
@@ -118,12 +119,7 @@ void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_
   frame->map_end = map_info->end;
   frame->map_flags = map_info->flags;
   frame->map_load_bias = elf->GetLoadBias();
-
-  if (!resolve_names_ ||
-      !elf->GetFunctionName(func_pc, &frame->function_name, &frame->function_offset)) {
-    frame->function_name = "";
-    frame->function_offset = 0;
-  }
+  return frame;
 }
 
 static bool ShouldStop(const std::vector<std::string>* map_suffixes_to_ignore,
@@ -194,6 +190,7 @@ void Unwinder::Unwind(const std::vector<std::string>* initial_map_names_to_skip,
       }
     }
 
+    FrameData* frame = nullptr;
     if (map_info == nullptr || initial_map_names_to_skip == nullptr ||
         std::find(initial_map_names_to_skip->begin(), initial_map_names_to_skip->end(),
                   basename(map_info->name.c_str())) == initial_map_names_to_skip->end()) {
@@ -210,23 +207,21 @@ void Unwinder::Unwind(const std::vector<std::string>* initial_map_names_to_skip,
         }
       }
 
-      FillInFrame(map_info, elf, rel_pc, step_pc, pc_adjustment);
+      frame = FillInFrame(map_info, elf, rel_pc, pc_adjustment);
 
       // Once a frame is added, stop skipping frames.
       initial_map_names_to_skip = nullptr;
     }
     adjust_pc = true;
 
-    bool stepped;
+    bool stepped = false;
     bool in_device_map = false;
-    if (map_info == nullptr) {
-      stepped = false;
-    } else {
+    bool finished = false;
+    if (map_info != nullptr) {
       if (map_info->flags & MAPS_FLAGS_DEVICE_MAP) {
         // Do not stop here, fall through in case we are
         // in the speculative unwind path and need to remove
         // some of the speculative frames.
-        stepped = false;
         in_device_map = true;
       } else {
         MapInfo* sp_info = maps_->Find(regs_->sp());
@@ -234,19 +229,37 @@ void Unwinder::Unwind(const std::vector<std::string>* initial_map_names_to_skip,
           // Do not stop here, fall through in case we are
           // in the speculative unwind path and need to remove
           // some of the speculative frames.
-          stepped = false;
           in_device_map = true;
         } else {
-          bool finished;
-          stepped = elf->Step(rel_pc, step_pc, regs_, process_memory_.get(), &finished);
-          elf->GetLastError(&last_error_);
-          if (stepped && finished) {
-            break;
+          if (elf->StepIfSignalHandler(rel_pc, regs_, process_memory_.get())) {
+            stepped = true;
+            if (frame != nullptr) {
+              // Need to adjust the relative pc because the signal handler
+              // pc should not be adjusted.
+              frame->rel_pc = rel_pc;
+              frame->pc += pc_adjustment;
+              step_pc = rel_pc;
+            }
+          } else if (elf->Step(step_pc, regs_, process_memory_.get(), &finished)) {
+            stepped = true;
           }
+          elf->GetLastError(&last_error_);
         }
       }
     }
 
+    if (frame != nullptr) {
+      if (!resolve_names_ ||
+          !elf->GetFunctionName(step_pc, &frame->function_name, &frame->function_offset)) {
+        frame->function_name = "";
+        frame->function_offset = 0;
+      }
+    }
+
+    if (finished) {
+      break;
+    }
+
     if (!stepped) {
       if (return_address_attempt) {
         // Only remove the speculative frame if there are more than two frames
diff --git a/libunwindstack/include/unwindstack/Elf.h b/libunwindstack/include/unwindstack/Elf.h
index ac94f10..56bf318 100644
--- a/libunwindstack/include/unwindstack/Elf.h
+++ b/libunwindstack/include/unwindstack/Elf.h
@@ -67,8 +67,9 @@ class Elf {
 
   uint64_t GetRelPc(uint64_t pc, const MapInfo* map_info);
 
-  bool Step(uint64_t rel_pc, uint64_t adjusted_rel_pc, Regs* regs, Memory* process_memory,
-            bool* finished);
+  bool StepIfSignalHandler(uint64_t rel_pc, Regs* regs, Memory* process_memory);
+
+  bool Step(uint64_t rel_pc, Regs* regs, Memory* process_memory, bool* finished);
 
   ElfInterface* CreateInterfaceFromMemory(Memory* memory);
 
diff --git a/libunwindstack/include/unwindstack/Error.h b/libunwindstack/include/unwindstack/Error.h
index 6ed0e0f..72ec454 100644
--- a/libunwindstack/include/unwindstack/Error.h
+++ b/libunwindstack/include/unwindstack/Error.h
@@ -29,6 +29,7 @@ enum ErrorCode : uint8_t {
   ERROR_INVALID_MAP,          // Unwind in an invalid map.
   ERROR_MAX_FRAMES_EXCEEDED,  // The number of frames exceed the total allowed.
   ERROR_REPEATED_FRAME,       // The last frame has the same pc/sp as the next.
+  ERROR_INVALID_ELF,          // Unwind in an invalid elf.
 };
 
 struct ErrorData {
diff --git a/libunwindstack/include/unwindstack/Unwinder.h b/libunwindstack/include/unwindstack/Unwinder.h
index 8b01654..75be209 100644
--- a/libunwindstack/include/unwindstack/Unwinder.h
+++ b/libunwindstack/include/unwindstack/Unwinder.h
@@ -118,8 +118,7 @@ class Unwinder {
   Unwinder(size_t max_frames) : max_frames_(max_frames) { frames_.reserve(max_frames); }
 
   void FillInDexFrame();
-  void FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_t func_pc,
-                   uint64_t pc_adjustment);
+  FrameData* FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_t pc_adjustment);
 
   size_t max_frames_;
   Maps* maps_;
diff --git a/libunwindstack/tests/ElfTest.cpp b/libunwindstack/tests/ElfTest.cpp
index 23c9cf8..c432d6d 100644
--- a/libunwindstack/tests/ElfTest.cpp
+++ b/libunwindstack/tests/ElfTest.cpp
@@ -132,8 +132,12 @@ TEST_F(ElfTest, elf_invalid) {
   uint64_t func_offset;
   ASSERT_FALSE(elf.GetFunctionName(0, &name, &func_offset));
 
+  ASSERT_FALSE(elf.StepIfSignalHandler(0, nullptr, nullptr));
+  EXPECT_EQ(ERROR_INVALID_ELF, elf.GetLastErrorCode());
+
   bool finished;
-  ASSERT_FALSE(elf.Step(0, 0, nullptr, nullptr, &finished));
+  ASSERT_FALSE(elf.Step(0, nullptr, nullptr, &finished));
+  EXPECT_EQ(ERROR_INVALID_ELF, elf.GetLastErrorCode());
 }
 
 TEST_F(ElfTest, elf32_invalid_machine) {
@@ -295,9 +299,8 @@ TEST_F(ElfTest, step_in_signal_map) {
   }
 
   elf.FakeSetValid(true);
-  bool finished;
-  ASSERT_TRUE(elf.Step(0x3000, 0x1000, &regs, &process_memory, &finished));
-  EXPECT_FALSE(finished);
+  ASSERT_TRUE(elf.StepIfSignalHandler(0x3000, &regs, &process_memory));
+  EXPECT_EQ(ERROR_NONE, elf.GetLastErrorCode());
   EXPECT_EQ(15U, regs.pc());
   EXPECT_EQ(13U, regs.sp());
 }
@@ -336,7 +339,7 @@ TEST_F(ElfTest, step_in_interface) {
   EXPECT_CALL(*interface, Step(0x1000, &regs, &process_memory, &finished))
       .WillOnce(::testing::Return(true));
 
-  ASSERT_TRUE(elf.Step(0x1004, 0x1000, &regs, &process_memory, &finished));
+  ASSERT_TRUE(elf.Step(0x1000, &regs, &process_memory, &finished));
 }
 
 TEST_F(ElfTest, get_global_invalid_elf) {
diff --git a/libunwindstack/tests/UnwindOfflineTest.cpp b/libunwindstack/tests/UnwindOfflineTest.cpp
index 02ba9c8..6c64c40 100644
--- a/libunwindstack/tests/UnwindOfflineTest.cpp
+++ b/libunwindstack/tests/UnwindOfflineTest.cpp
@@ -1215,7 +1215,7 @@ TEST_F(UnwindOfflineTest, offset_arm) {
       "  #02 pc 0032bff3  libunwindstack_test (SignalOuterFunction+2)\n"
       "  #03 pc 0032fed3  libunwindstack_test "
       "(unwindstack::SignalCallerHandler(int, siginfo*, void*)+26)\n"
-      "  #04 pc 00026528  libc.so\n"
+      "  #04 pc 0002652c  libc.so (__restore)\n"
       "  #05 pc 00000000  <unknown>\n"
       "  #06 pc 0032c2d9  libunwindstack_test (InnerFunction+736)\n"
       "  #07 pc 0032cc4f  libunwindstack_test (MiddleFunction+42)\n"
@@ -1243,7 +1243,7 @@ TEST_F(UnwindOfflineTest, offset_arm) {
   EXPECT_EQ(0xf43d2ce8U, unwinder.frames()[2].sp);
   EXPECT_EQ(0x2e59ed3U, unwinder.frames()[3].pc);
   EXPECT_EQ(0xf43d2cf0U, unwinder.frames()[3].sp);
-  EXPECT_EQ(0xf4136528U, unwinder.frames()[4].pc);
+  EXPECT_EQ(0xf413652cU, unwinder.frames()[4].pc);
   EXPECT_EQ(0xf43d2d10U, unwinder.frames()[4].sp);
   EXPECT_EQ(0U, unwinder.frames()[5].pc);
   EXPECT_EQ(0xffcc0ee0U, unwinder.frames()[5].sp);
@@ -1326,7 +1326,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_arm64) {
       "  #00 pc 000000000014ccbc  linker64 (__dl_syscall+28)\n"
       "  #01 pc 000000000005426c  linker64 "
       "(__dl__ZL24debuggerd_signal_handleriP7siginfoPv+1128)\n"
-      "  #02 pc 00000000000008bc  vdso.so\n"
+      "  #02 pc 00000000000008c0  vdso.so (__kernel_rt_sigreturn)\n"
       "  #03 pc 00000000000846f4  libc.so (abort+172)\n"
       "  #04 pc 0000000000084ad4  libc.so (__assert2+36)\n"
       "  #05 pc 000000000003d5b4  ANGLEPrebuilt.apk!libfeature_support_angle.so (offset 0x4000) "
@@ -1338,7 +1338,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_arm64) {
   EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[0].sp);
   EXPECT_EQ(0x7e82b5726cULL, unwinder.frames()[1].pc);
   EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[1].sp);
-  EXPECT_EQ(0x7e82b018bcULL, unwinder.frames()[2].pc);
+  EXPECT_EQ(0x7e82b018c0ULL, unwinder.frames()[2].pc);
   EXPECT_EQ(0x7df8ca3da0ULL, unwinder.frames()[2].sp);
   EXPECT_EQ(0x7e7eecc6f4ULL, unwinder.frames()[3].pc);
   EXPECT_EQ(0x7dabf3db60ULL, unwinder.frames()[3].sp);
@@ -1366,7 +1366,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_memory_only_arm64) {
       "  #00 pc 000000000014ccbc  linker64 (__dl_syscall+28)\n"
       "  #01 pc 000000000005426c  linker64 "
       "(__dl__ZL24debuggerd_signal_handleriP7siginfoPv+1128)\n"
-      "  #02 pc 00000000000008bc  vdso.so\n"
+      "  #02 pc 00000000000008c0  vdso.so (__kernel_rt_sigreturn)\n"
       "  #03 pc 00000000000846f4  libc.so (abort+172)\n"
       "  #04 pc 0000000000084ad4  libc.so (__assert2+36)\n"
       "  #05 pc 000000000003d5b4  ANGLEPrebuilt.apk (offset 0x21d5000)\n"
@@ -1377,7 +1377,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_memory_only_arm64) {
   EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[0].sp);
   EXPECT_EQ(0x7e82b5726cULL, unwinder.frames()[1].pc);
   EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[1].sp);
-  EXPECT_EQ(0x7e82b018bcULL, unwinder.frames()[2].pc);
+  EXPECT_EQ(0x7e82b018c0ULL, unwinder.frames()[2].pc);
   EXPECT_EQ(0x7df8ca3da0ULL, unwinder.frames()[2].sp);
   EXPECT_EQ(0x7e7eecc6f4ULL, unwinder.frames()[3].pc);
   EXPECT_EQ(0x7dabf3db60ULL, unwinder.frames()[3].sp);