[lldb][AArch64] Add UnwindPlan for Linux sigreturn

[DavidSpickett]

Summary

This adds a specific unwind plan for AArch64 Linux sigreturn frames.
Previously we assumed that the fp would be valid here but it is not.

https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/vdso/sigreturn.S

On Ubuntu Bionic it happened to point to an old frame info which meant
you got what looked like a correct backtrace. On Focal, the info is
completely invalid. (probably due to some code shuffling in libc)

This adds an UnwindPlan that knows that the sp in a sigreturn frame
points to an rt_sigframe from which we can offset to get saved
sp and pc values to backtrace correctly.

Based on LibUnwind's change: https://reviews.llvm.org/D90898

A new test is added that sets all compares the frames from the initial
signal catch to the handler break. Ensuring that the stack/frame pointer,
function name and register values match.
(this test is AArch64 Linux specific because it's the only one
with a specific unwind plan for this situation)

Fixes https://bugs.llvm.org/show_bug.cgi?id=52165

[DavidSpickett]

lldb/source/Symbol/AArch64UnwindInfo.cpp

40

This comment comes from the libunwind change.

58

If this means "at all instructions within that function" this could be yes, but no one seems to read this field anyway.

[omjavaid]

This look great. Thanks for doing the legit fix.

@mgorny This is something you might wanna implement/test on BSD.

[labath]

I don't think this is a scalable way to fix this, and should get more discussion. Jason what do you make of this?

[labath]

I'm sorry for terse response. I wanted to ensure this gets more attention, but I wasn't able to look at this straight away. Let me try to elaborate.

We generally try to avoid platform-specific code in the core libraries. The unwind code is one of the biggest offenders here, but I'd still want to avoid making it worse, particularly if there is a fairly simple way to avoid that. And I think there might be one. We already involve the Platform class in the unwinding logic (see Platform::GetTrapHandlerSymbolNames). Since it already knows about their names, it doesn't seem far-fetched to have it provide a way to unwind out of them (if necessary). Creating a new Platform entry point to provide the unwind plan (or somehow refactoring the GetTrapHandlerSymbolNames so that it can also provide an unwind plan) would enable us to put this code into PlatformLinux, which (although not ideal) seems much better than smacking it into the middle of the Symbol library. It also seems consistent with the intent in one of the comments in the code you modify.

lldb/source/Symbol/AArch64UnwindInfo.cpp

58

The way I read it is "all locations within the range of this unwind plan" as given by UnwindPlan::GetAddressRange. compiler-generated unwind plans might cover the entire function, but only be valid (correct) at call sites.

lldb/source/Target/RegisterContextUnwind.cpp

900

this comment

[DavidSpickett]

Creating a new Platform entry point to provide the unwind plan (or somehow refactoring the GetTrapHandlerSymbolNames so that it can also provide an unwind plan)

Makes sense, I'll try that.

[DavidSpickett]

lldb/source/Symbol/AArch64UnwindInfo.cpp

58

So this would be valid at all instructions because at any point in __kernel_rt_sigreturn the stack pointer is the same.

[DavidSpickett]

Set ValidAtAllInstructions.

Move building unwind plan into PlatformLinux and add a platform method
to lookup unwind plans for signal handlers. I'm still checking architecture
in PlatformLinux but it does move the code out of Symbol.

The only per arch thing on lldb's side is Architecture plugins but they're
not meant to know about operating system. So it seems like a reasonable
compromise to put it in PlatformLinux.

[DavidSpickett]

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

258 ↗

(On Diff #382287)

PlatformLinux lists 3 handlers:

• _sigtramp
• __kernel_rt_sigreturn
• __restore_rt

I think the first 2 are the same for AArch64 due to this line in the kernel:
VDSO_sigtramp = __kernel_rt_sigreturn;

The third is for real time signals which I'm only just learning about. So I'll see what that's about but probably not relevant for this change.

[DavidSpickett]

Set SP not FP based on the sigcontext.

Before, the SP of the frame before sigcontext would be incorrect.
(noticed while testing alt signal stacks)

This was already done in the libunwind change but I got them mixed up.

I'll come up with some addition to the tests to check this.

[labath]

Thanks for doing this. Just a couple of comments inline.

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

288 ↗

(On Diff #382354)

I have a feeling that this eRegisterKind enum should correspond with the register numbers used in the unwind plan. It'd be great if it did, as then you could replace arm64_dwarf::{pc,sp} with LLDB_REGNUM_GENERIC_{PC,SP}, and avoid including the arm64 header. Though if it does work as I remember, then I guess the question is how does this work in the first place.

303–305 ↗

(On Diff #382354)

If you passed in a llvm::Triple, you could replace this with triple.isAArch64().

And one could imagine that e.g. the environment field of the triple could be important in determining the precise unwind plan.

lldb/source/Target/RegisterContextUnwind.cpp

900

I guess we should also update this comment now. Maybe:

On some platforms the unwind information for signal handlers is not present or correct. Give the platform plugins a chance to provide substitute plan. Otherwise, use eh_frame.

?

[labath]

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

295 ↗

(On Diff #382354)

Ah now I see that it worked because of this. Maybe you could try dropping this line and replace the dwarf numbers with generic regnums?

[DavidSpickett]

Turns out comparing thread.get_thread_frames() doesn't work because
you end up getting references to the same frames. Instead save the info
we want to compare.

One obvious reason this tactic should have failed is that the libc raise
frame will have a different ID between the two backtraces. This is why I've
ignored ID in the comparion.

Doing this showed me that sp and fp should be set, so I've got fp from
the sigcontext also.

[DavidSpickett]

Thanks for doing this. Just a couple of comments inline.

Will respond to these shortly.

[labath]

Since the signal context contains a copy of all registers, maybe you should be setting all of them. The other registers will not be used for unwinding, but they will enable to user to inspect the registers in the stack frames above the signal handler.
One way to demonstrate (and test) this would be to have a bit of inline asm, which sets all registers to known values, and then raises a signal (SIGILL is probably the easiest to raise from asm). Then lldb breaks on the signal handler, goes up the stack, and observes the registers.

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

295 ↗

(On Diff #382354)

Except of course, if you end up setting all of the registers.

[DavidSpickett]

Use generic register names and don't set type to DWARF.

Pass triple instead of machine and use isAArch64.

[DavidSpickett]

Update comment.

[DavidSpickett]

One way to demonstrate (and test) this would be to have a bit of inline asm, which sets all registers to known values, and then raises a signal (SIGILL is probably the easiest to raise from asm).

I will give that a go, probably as a separate test case.

[DavidSpickett]

• Set locations for all general purpose registers
• Set register type to DWARF due to that
• Add a new test that sets known register values and compares them between signal catch and handler
• Remove changes to handle abort test (though I could do those as a separate thing later)

One thing remains, the sigcontext can include floating point and SVE
registers. We'd need to read some memory to determine if it does:
https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/sigcontext.h#L39

Which I can do by passing the target and generating the plan based on
what's present.

For now let me know if the test case makes sense. Maybe this is ok
to go in as is and I can follow up with the other registers?

[DavidSpickett]

I'll at least make the floating point/SVE changes children of this review since it'll add some complexity.

[DavidSpickett]

lldb/test/API/linux/aarch64/unwind_signal/main.c

23 ↗

(On Diff #386108)

In my testing the kernel didn't go out of its way to change all the register values but it always changed a few so the test fails consistently if the unwind plan doesn't set the locations.

[labath]

In D112069#3120951, @DavidSpickett wrote:

• Set locations for all general purpose registers
• Set register type to DWARF due to that
• Add a new test that sets known register values and compares them between signal catch and handler
• Remove changes to handle abort test (though I could do those as a separate thing later)

One thing remains, the sigcontext can include floating point and SVE
registers. We'd need to read some memory to determine if it does:
https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/sigcontext.h#L39

Which I can do by passing the target and generating the plan based on
what's present.

For now let me know if the test case makes sense. Maybe this is ok
to go in as is and I can follow up with the other registers?

Yes, this is definitely fine. Thanks for taking your time to do that.

Dynamically generating unwind plans based on the contents of memory kinda goes against the concept of unwind plans, which were meant to be static (and cacheable) descriptions of how to unwind from a given point (PC) in a program. I guess the most "pure" solution would be to encode this logic into the dwarf expressions for the relevant registers. I think this should be doable (dwarf expressions being turing-complete and all), but the resulting expressions would probably be horrendous.

Overall, I'm not too worried about not being able to recover non-gpr registers so (unless you really want to work on this) I think that can stay unimplemented. (I'd also be fine with not recovering gpr registers either, though it's obviously better if they are -- the reason this came up was because I was looking at ways to simplify the eRegisterKind business).

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

322 ↗

(On Diff #386108)

Change to eRegisterKindDWARF and remove the SetRegisterKind call below.

lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py

82–83 ↗

(On Diff #386108)

I don't think this is bad, but you could just hardcode the expectations into the python file. Then you could skip this step and continue straight to the final breakpoint.

lldb/test/API/linux/aarch64/unwind_signal/main.c

23 ↗

(On Diff #386108)

One could also have a similar block (but with different values) in the signal handler, which would guarantee that all registers are being read from the stack.

[DavidSpickett]

• Set x registers to a different pattern in the signal handler
• Don't save frames before and after, just check for the right pattern when unwinding out of the handler to sigill()
• Set register kind when we first make the unwind plan

[DavidSpickett]

Thanks for the help, really helped get a high quality fix!

lldb/include/lldb/Symbol/AArch64UnwindInfo.h

+//===-- AArch64UnwindInfo.h -------------------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SYMBOL_AARCH64UNWINDINFO_H
+#define LLDB_SYMBOL_AARCH64UNWINDINFO_H
+
+#include "lldb/Symbol/UnwindPlan.h"
+#include "lldb/Target/Target.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-private.h"
+
+namespace lldb_private {
+
+// If target is AArch64 Linux return a shared pointer to
+// an UnwindPlan. Otherwise return an invalid shared pointer.
+// We assume the caller has checked that they are in a trap handler frame.
+lldb::UnwindPlanSP GetAArch64LinuxTrapHandlerUnwindPlan(const Target &target);
+
+} // namespace lldb_private
+
+#endif // LLDB_SYMBOL_AARCH64UNWINDINFO_H

lldb/source/Symbol/AArch64UnwindInfo.cpp

+//===-- AArch64UnwindInfo.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/AArch64UnwindInfo.h"
+#include "Utility/ARM64_DWARF_Registers.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+lldb::UnwindPlanSP
+lldb_private::GetAArch64LinuxTrapHandlerUnwindPlan(const Target &target) {
+  lldb::UnwindPlanSP unwind_plan_sp;
+  ArchSpec arch = target.GetArchitecture();
+  llvm::Triple::ArchType machine = arch.GetMachine();
+
+  if ((machine != llvm::Triple::ArchType::aarch64 &&
+       machine != llvm::Triple::ArchType::aarch64_be &&
+       machine != llvm::Triple::ArchType::aarch64_32) ||
+      arch.GetTriple().getOS() != llvm::Triple::OSType::Linux)
+    return unwind_plan_sp;
+
+  UnwindPlan::RowSP row = std::make_shared<UnwindPlan::Row>();
+  row->SetOffset(0);
+
+  // In the signal trampoline frame, sp points to an rt_sigframe[1], which is:
+  //  - 128-byte siginfo struct
+  //  - ucontext struct:
+  //     - 8-byte long (uc_flags)
+  //     - 8-byte pointer (uc_link)
+  //     - 24-byte stack_t
+  //     - 128-byte signal set
+  //     - 8 bytes of padding because sigcontext has 16-byte alignment
+  //     - sigcontext/mcontext_t
+  // [1]
+  // https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/signal.c
+  int32_t offset = 128 + 8 + 8 + 24 + 128 + 8;
+  // Then sigcontext[2] is:
+  // - 8 byte fault address
+  // - 31 8 byte registers
+  // - 8 byte sp
+  // - 8 byte pc
+  // [2]
+  // https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/sigcontext.h
+  offset += 8 + (31 * 8);
+  row->GetCFAValue().SetIsRegisterPlusOffset(arm64_dwarf::sp, offset);
+  row->SetRegisterLocationToAtCFAPlusOffset(arm64_dwarf::fp, 0, false);
+  row->SetRegisterLocationToAtCFAPlusOffset(arm64_dwarf::pc, 8, false);
+
+  unwind_plan_sp = std::make_shared<UnwindPlan>(eRegisterKindGeneric);
+  unwind_plan_sp->AppendRow(row);
+  unwind_plan_sp->SetSourceName("AArch64 Linux sigcontext");
+  unwind_plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
+  unwind_plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);
+  unwind_plan_sp->SetRegisterKind(eRegisterKindDWARF);
+
+  return unwind_plan_sp;
+}

lldb/source/Symbol/CMakeLists.txt

 set(LLVM_OPTIONAL_SOURCES LocateSymbolFileMacOSX.cpp)
 
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
   set(PLATFORM_SOURCES
     LocateSymbolFileMacOSX.cpp
     )
 endif()
 
 add_lldb_library(lldbSymbol
+  AArch64UnwindInfo.cpp
   ArmUnwindInfo.cpp
   Block.cpp
   CompactUnwindInfo.cpp
   CompileUnit.cpp
   CompilerDecl.cpp
   CompilerDeclContext.cpp
   CompilerType.cpp
   DWARFCallFrameInfo.cpp

lldb/source/Target/RegisterContextUnwind.cpp

 //===-- RegisterContextUnwind.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/Target/RegisterContextUnwind.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Core/AddressRange.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Expression/DWARFExpression.h"
+#include "lldb/Symbol/AArch64UnwindInfo.h"
 #include "lldb/Symbol/ArmUnwindInfo.h"
 #include "lldb/Symbol/CallFrameInfo.h"
 #include "lldb/Symbol/DWARFCallFrameInfo.h"
 #include "lldb/Symbol/FuncUnwinders.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/SymbolContext.h"
if (!func_unwinders_sp) {
     }
 
     return arch_default_unwind_plan_sp;
   }
 
   // If we're in _sigtramp(), unwinding past this frame requires special
   // knowledge.  On Mac OS X this knowledge is properly encoded in the eh_frame
   // section, so prefer that if available. On other platforms we may need to
   // provide a platform-specific UnwindPlan which encodes the details of how to
   // unwind out of sigtramp.
   if (m_frame_type == eTrapHandlerFrame && process) {
     m_fast_unwind_plan_sp.reset();
+
+    unwind_plan_sp = GetAArch64LinuxTrapHandlerUnwindPlan(process->GetTarget());
+    if (unwind_plan_sp)
+      return unwind_plan_sp;
+
     unwind_plan_sp =
         func_unwinders_sp->GetEHFrameUnwindPlan(process->GetTarget());
     if (!unwind_plan_sp)
       unwind_plan_sp =
           func_unwinders_sp->GetObjectFileUnwindPlan(process->GetTarget());
     if (unwind_plan_sp && unwind_plan_sp->PlanValidAtAddress(m_current_pc) &&
         unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes) {
       return unwind_plan_sp;

lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

 
 class HandleAbortTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     NO_DEBUG_INFO_TESTCASE = True
 
     @skipIfWindows  # signals do not exist on Windows
-    # Fails on Ubuntu Focal
-    @skipIf(archs=["aarch64"], oslist=["linux"])
     @expectedFailureNetBSD
     def test_inferior_handle_sigabrt(self):
         """Inferior calls abort() and handles the resultant SIGABRT.
            Stopped at a breakpoint in the handler, verify that the backtrace
            includes the function that called abort()."""
         self.build()
         exe = self.getBuildArtifact("a.out")
 
def test_inferior_handle_sigabrt(self):
             thread and thread.IsValid(),
             "Thread should be stopped due to a signal")
         self.assertTrue(
             thread.GetStopReasonDataCount() >= 1,
             "There should be data in the event.")
         self.assertEqual(thread.GetStopReasonDataAtIndex(0),
                          signo, "The stop signal should be SIGABRT")
 
+        # Save the backtrace frames to compare to the handler backtrace later.
+        signal_frames = thread.get_thread_frames()
+
         # Continue to breakpoint in abort handler
         bkpt = target.FindBreakpointByID(
             lldbutil.run_break_set_by_source_regexp(self, "Set a breakpoint here"))
         threads = lldbutil.continue_to_breakpoint(process, bkpt)
         self.assertEqual(len(threads), 1, "Expected single thread")
         thread = threads[0]
 
         # Expect breakpoint in 'handler'
         frame = thread.GetFrameAtIndex(0)
         self.assertEqual(frame.GetDisplayFunctionName(), "handler", "Unexpected break?")
 
+        handler_frames = thread.get_thread_frames()
+
         # Expect that unwinding should find 'abort_caller'
-        foundFoo = False
-        for frame in thread:
+        found_caller = False
+        for frame in handler_frames:
             if frame.GetDisplayFunctionName() == "abort_caller":
-                foundFoo = True
+                found_caller = True
+                break
+
+        self.assertTrue(found_caller, "Unwinding did not find func that called abort")
 
-        self.assertTrue(foundFoo, "Unwinding did not find func that called abort")
+        # Check that frames present in both backtraces have the same addresses.
+        # The signal handler backtrace has extra frames at the start.
+        self.assertEqual(signal_frames, handler_frames[-len(signal_frames):],
+          "Common frames for signal and signal handler backtrace do not match")
 
         # Continue until we exit.
         process.Continue()
         self.assertEqual(process.GetState(), lldb.eStateExited)
         self.assertEqual(process.GetExitStatus(), 0)