[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9db2541d4c30: [lldb][AArch64] Add UnwindPlan for Linux 
sigreturn (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
  lldb/test/API/linux/aarch64/unwind_signal/Makefile
  lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
  lldb/test/API/linux/aarch64/unwind_signal/main.c

Index: lldb/test/API/linux/aarch64/unwind_signal/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/unwind_signal/main.c
@@ -0,0 +1,64 @@
+#include 
+#include 
+#include 
+
+void handler(int sig) {
+  // The kernel only changes a few registers so set them all to something other
+  // than the values in sigill() so that we can't fall back to real registers
+  // and still pass the test.
+#define SETREG(N) "mov x" N ", #" N "+1\n\t"
+  asm volatile(
+  /* clang-format off */
+  /* x0 is used for a parameter */
+   SETREG("1")  SETREG("2")  SETREG("3")
+  SETREG("4")  SETREG("5")  SETREG("6")  SETREG("7")
+  SETREG("8")  SETREG("9")  SETREG("10") SETREG("11")
+  SETREG("12") SETREG("13") SETREG("14") SETREG("15")
+  SETREG("16") SETREG("17") SETREG("18") SETREG("19")
+  SETREG("20") SETREG("21") SETREG("22") SETREG("23")
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") // fp/x29 needed for unwiding
+  SETREG("30") // 31 is xzr/sp
+  /* clang-format on */
+  ::
+  : /* skipped x0 */ "x1", "x2", "x3", "x4", "x5", "x6", "x7", "x8",
+"x9", "x10", "x11", "x12", "x13", "x14", "x15", "x16", "x17", "x18",
+"x19", "x20", "x21", "x22", "x23", "x24", "x25", "x26", "x27",
+"x28",
+/* skipped fp/x29 */ "x30");
+  printf("Set a breakpoint here.\n");
+  exit(0);
+}
+
+static void sigill() {
+  // Set all general registers to known values to check
+  // that the signal unwind plan sets their locations correctly.
+#define SETREG(N) "mov x" N ", #" N "\n\t"
+  asm volatile(
+  /* clang-format off */
+  SETREG("0")  SETREG("1")  SETREG("2")  SETREG("3")
+  SETREG("4")  SETREG("5")  SETREG("6")  SETREG("7")
+  SETREG("8")  SETREG("9")  SETREG("10") SETREG("11")
+  SETREG("12") SETREG("13") SETREG("14") SETREG("15")
+  SETREG("16") SETREG("17") SETREG("18") SETREG("19")
+  SETREG("20") SETREG("21") SETREG("22") SETREG("23")
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") SETREG("29") SETREG("30") /* 31 is xzr/sp */
+  /* clang-format on */
+  ".inst   0x\n\t" // udf #0 (old binutils don't support udf)
+  ::
+  : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7", "x8", "x9", "x10",
+"x11", "x12", "x13", "x14", "x15", "x16", "x17", "x18", "x19",
+"x20", "x21", "x22", "x23", "x24", "x25", "x26", "x27", "x28",
+"x29", "x30");
+}
+
+int main() {
+  if (signal(SIGILL, handler) == SIG_ERR) {
+perror("signal");
+return 1;
+  }
+
+  sigill();
+  return 2;
+}
Index: lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
===
--- lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
+++ lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
@@ -1,6 +1,5 @@
-"""Test that we can unwind out of a SIGABRT handler"""
-
-
+"""Test that we can unwind out of a signal handler.
+   Which for AArch64 Linux requires a specific unwind plan."""
 
 
 import lldb
@@ -9,33 +8,29 @@
 from lldbsuite.test import lldbutil
 
 
-class HandleAbortTestCase(TestBase):
+class UnwindSignalTestCase(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()."""
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+def test_unwind_signal(self):
+"""Inferior calls sigill() and handles the resultant SIGILL.
+   Stopped at a breakpoint in the handler, check that we can unwind
+   back to sigill() and get the expected register contents there."""
 self.build()
 exe = self.getBuildArtifact("a.out")
 
-

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 386466.
DavidSpickett added a comment.

- 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
  lldb/test/API/linux/aarch64/unwind_signal/Makefile
  lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
  lldb/test/API/linux/aarch64/unwind_signal/main.c

Index: lldb/test/API/linux/aarch64/unwind_signal/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/unwind_signal/main.c
@@ -0,0 +1,64 @@
+#include 
+#include 
+#include 
+
+void handler(int sig) {
+  // The kernel only changes a few registers so set them all to something other
+  // than the values in sigill() so that we can't fall back to real registers
+  // and still pass the test.
+#define SETREG(N) "mov x" N ", #" N "+1\n\t"
+  asm volatile(
+  /* clang-format off */
+  /* x0 is used for a parameter */
+   SETREG("1")  SETREG("2")  SETREG("3")
+  SETREG("4")  SETREG("5")  SETREG("6")  SETREG("7")
+  SETREG("8")  SETREG("9")  SETREG("10") SETREG("11")
+  SETREG("12") SETREG("13") SETREG("14") SETREG("15")
+  SETREG("16") SETREG("17") SETREG("18") SETREG("19")
+  SETREG("20") SETREG("21") SETREG("22") SETREG("23")
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") // fp/x29 needed for unwiding
+  SETREG("30") // 31 is xzr/sp
+  /* clang-format on */
+  ::
+  : /* skipped x0 */ "x1", "x2", "x3", "x4", "x5", "x6", "x7", "x8",
+"x9", "x10", "x11", "x12", "x13", "x14", "x15", "x16", "x17", "x18",
+"x19", "x20", "x21", "x22", "x23", "x24", "x25", "x26", "x27",
+"x28",
+/* skipped fp/x29 */ "x30");
+  printf("Set a breakpoint here.\n");
+  exit(0);
+}
+
+static void sigill() {
+  // Set all general registers to known values to check
+  // that the signal unwind plan sets their locations correctly.
+#define SETREG(N) "mov x" N ", #" N "\n\t"
+  asm volatile(
+  /* clang-format off */
+  SETREG("0")  SETREG("1")  SETREG("2")  SETREG("3")
+  SETREG("4")  SETREG("5")  SETREG("6")  SETREG("7")
+  SETREG("8")  SETREG("9")  SETREG("10") SETREG("11")
+  SETREG("12") SETREG("13") SETREG("14") SETREG("15")
+  SETREG("16") SETREG("17") SETREG("18") SETREG("19")
+  SETREG("20") SETREG("21") SETREG("22") SETREG("23")
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") SETREG("29") SETREG("30") /* 31 is xzr/sp */
+  /* clang-format on */
+  ".inst   0x\n\t" // udf #0 (old binutils don't support udf)
+  ::
+  : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7", "x8", "x9", "x10",
+"x11", "x12", "x13", "x14", "x15", "x16", "x17", "x18", "x19",
+"x20", "x21", "x22", "x23", "x24", "x25", "x26", "x27", "x28",
+"x29", "x30");
+}
+
+int main() {
+  if (signal(SIGILL, handler) == SIG_ERR) {
+perror("signal");
+return 1;
+  }
+
+  sigill();
+  return 2;
+}
Index: lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
===
--- lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
+++ lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
@@ -1,6 +1,5 @@
-"""Test that we can unwind out of a SIGABRT handler"""
-
-
+"""Test that we can unwind out of a signal handler.
+   Which for AArch64 Linux requires a specific unwind plan."""
 
 
 import lldb
@@ -9,33 +8,29 @@
 from lldbsuite.test import lldbutil
 
 
-class HandleAbortTestCase(TestBase):
+class UnwindSignalTestCase(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()."""
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+def test_unwind_signal(self):
+"""Inferior calls sigill() and handles the resultant SIGILL.
+   Stopped at a breakpoint in the handler, check that we can unwind
+   back to 

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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).




Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:322
+
+  unwind_plan_sp = std::make_shared(eRegisterKindGeneric);
+  unwind_plan_sp->AppendRow(row);

Change to `eRegisterKindDWARF` and remove the `SetRegisterKind` call below.



Comment at: lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py:82-83
+
+# Save the backtrace frames to compare to the handler backtrace later.
+signal_frames = [make_frame_info(f) for f in 
thread.get_thread_frames()]
+

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.



Comment at: lldb/test/API/linux/aarch64/unwind_signal/main.c:23
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") SETREG("29") SETREG("30") /* 31 is xzr/sp */
+  /* clang-format on */

DavidSpickett wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/test/API/linux/aarch64/unwind_signal/main.c:23
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") SETREG("29") SETREG("30") /* 31 is xzr/sp */
+  /* clang-format on */

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 386108.
DavidSpickett added a comment.

- 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
  lldb/test/API/linux/aarch64/unwind_signal/Makefile
  lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
  lldb/test/API/linux/aarch64/unwind_signal/main.c

Index: lldb/test/API/linux/aarch64/unwind_signal/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/unwind_signal/main.c
@@ -0,0 +1,41 @@
+#include 
+#include 
+#include 
+
+void handler(int sig) {
+  printf("Set a breakpoint here.\n");
+  exit(0);
+}
+
+static void sigill() {
+  // Set all general registers to known values to check
+  // that the signal unwind plan sets their locations correctly.
+#define SETREG(N) "mov x" N ", #" N "\n\t"
+  asm(
+  /* clang-format off */
+  SETREG("0")  SETREG("1")  SETREG("2")  SETREG("3")
+  SETREG("4")  SETREG("5")  SETREG("6")  SETREG("7")
+  SETREG("8")  SETREG("9")  SETREG("10") SETREG("11")
+  SETREG("12") SETREG("13") SETREG("14") SETREG("15")
+  SETREG("16") SETREG("17") SETREG("18") SETREG("19")
+  SETREG("20") SETREG("21") SETREG("22") SETREG("23")
+  SETREG("24") SETREG("25") SETREG("26") SETREG("27")
+  SETREG("28") SETREG("29") SETREG("30") /* 31 is xzr/sp */
+  /* clang-format on */
+  ".inst   0x\n\t" // udf #0 (old binutils don't support do udf)
+  ::
+  : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7", "x8", "x9", "x10",
+"x11", "x12", "x13", "x14", "x15", "x16", "x17", "x18", "x19",
+"x20", "x21", "x22", "x23", "x24", "x25", "x26", "x27", "x28",
+"x29", "x30");
+}
+
+int main() {
+  if (signal(SIGILL, handler) == SIG_ERR) {
+perror("signal");
+return 1;
+  }
+
+  sigill();
+  return 2;
+}
Index: lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/unwind_signal/TestUnwindSignal.py
@@ -0,0 +1,115 @@
+"""Test that we can unwind out of a signal handler.
+   Which for AArch64 Linux requires a specific unwind plan."""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from collections import namedtuple
+
+
+# Since frames internally point to ExecutionContextRef which itself points
+# to something else, we can't make a deep copy of them from Python.
+# Instead save what we care about for comparison purposes.
+# Ignoring the frame ID because what is frame 0 in the first catch will
+# be frame 2 in the handler backtrace.
+class FrameInfo(namedtuple('FrameInfo', ['function_name', 'sp', 'gp_regs'])):
+# So assert failures are more readable
+def __repr__(self):
+reg_lines = []
+for k, v in self.gp_regs.items():
+if v is None:
+  value = "{}".format(v)
+else:
+  value = "0x{:x}".format(v)
+
+reg_lines.append("{}: {}".format(k, value))
+
+return "Fn: {} SP: 0x{:x}\n{}".format(self.function_name,
+  self.sp,
+  "\n".join(reg_lines))
+
+
+def make_frame_info(frame):
+gp_regs = frame.GetRegisters().GetValueAtIndex(0)
+reg_values = {}
+err = lldb.SBError()
+for i in range(0, 31):
+name = 'x' + str(i)
+# This will fail if we have no location information
+value = gp_regs.GetChildMemberWithName(name).GetValueAsUnsigned(err)
+reg_values[name] = value if err.Success() else None
+
+return FrameInfo(frame.GetDisplayFunctionName(), frame.GetSP(), reg_values)
+
+
+class UnwindSignalTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 382591.
DavidSpickett added a comment.

Update comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/API/SBFrame.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

Index: lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
===
--- lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
+++ lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
@@ -7,7 +7,21 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
-
+from collections import namedtuple
+
+# Since frames internally point to ExecutionContextRef which itself points
+# to something else, we can't make a deep copy of them from Python.
+# Instead save what we care about for comparison purposes.
+# Ignoring the frame ID because what is frame 0 in the first catch will
+# be frame 2 in the handler backtrace.
+class FrameInfo(namedtuple('FrameInfo', ['sp', 'fp', 'function_name'])):
+# So assert failures are more readable
+def __repr__(self):
+return "SP: 0x{:x} FP: 0x{:x} Fn: {}".format(
+self.sp, self.fp, self.function_name)
+
+def make_frame_info(frame):
+return FrameInfo(frame.GetSP(), frame.GetFP(), frame.GetDisplayFunctionName())
 
 class HandleAbortTestCase(TestBase):
 
@@ -16,8 +30,6 @@
 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.
@@ -47,6 +59,9 @@
 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 = [make_frame_info(f) for f in 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"))
@@ -59,12 +74,20 @@
 self.assertEqual(frame.GetDisplayFunctionName(), "handler", "Unexpected break?")
 
 # Expect that unwinding should find 'abort_caller'
-foundFoo = False
-for frame in thread:
+found_caller = False
+for frame in thread.get_thread_frames():
 if frame.GetDisplayFunctionName() == "abort_caller":
-foundFoo = True
+found_caller = True
+break
+
+self.assertTrue(found_caller, "Unwinding did not find func that called abort")
+
+# The signal handler backtrace has extra frames at the start, remove those
+handler_frames = thread.get_thread_frames()[-len(signal_frames):]
+handler_frames = [make_frame_info(f) for f in handler_frames]
 
-self.assertTrue(foundFoo, "Unwinding did not find func that called abort")
+# Check that frames present in both backtraces have the same addresses.
+self.assertEqual(signal_frames, handler_frames, "Common backtrace frames do not match")
 
 # Continue until we exit.
 process.Continue()
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -893,13 +893,22 @@
 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();
+
+// 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.
+if (m_sym_ctx_valid) {
+  lldb::PlatformSP platform = process->GetTarget().GetPlatform();
+  unwind_plan_sp = platform->GetTrapHandlerUnwindPlan(
+  process->GetTarget().GetArchitecture().GetTriple(),
+  GetSymbolOrFunctionName(m_sym_ctx));
+
+  if (unwind_plan_sp)
+return unwind_plan_sp;
+}
+
 unwind_plan_sp =
 

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 382590.
DavidSpickett added a comment.

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

Pass triple instead of machine and use isAArch64.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/API/SBFrame.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

Index: lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
===
--- lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
+++ lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
@@ -7,7 +7,21 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
-
+from collections import namedtuple
+
+# Since frames internally point to ExecutionContextRef which itself points
+# to something else, we can't make a deep copy of them from Python.
+# Instead save what we care about for comparison purposes.
+# Ignoring the frame ID because what is frame 0 in the first catch will
+# be frame 2 in the handler backtrace.
+class FrameInfo(namedtuple('FrameInfo', ['sp', 'fp', 'function_name'])):
+# So assert failures are more readable
+def __repr__(self):
+return "SP: 0x{:x} FP: 0x{:x} Fn: {}".format(
+self.sp, self.fp, self.function_name)
+
+def make_frame_info(frame):
+return FrameInfo(frame.GetSP(), frame.GetFP(), frame.GetDisplayFunctionName())
 
 class HandleAbortTestCase(TestBase):
 
@@ -16,8 +30,6 @@
 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.
@@ -47,6 +59,9 @@
 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 = [make_frame_info(f) for f in 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"))
@@ -59,12 +74,20 @@
 self.assertEqual(frame.GetDisplayFunctionName(), "handler", "Unexpected break?")
 
 # Expect that unwinding should find 'abort_caller'
-foundFoo = False
-for frame in thread:
+found_caller = False
+for frame in thread.get_thread_frames():
 if frame.GetDisplayFunctionName() == "abort_caller":
-foundFoo = True
+found_caller = True
+break
+
+self.assertTrue(found_caller, "Unwinding did not find func that called abort")
+
+# The signal handler backtrace has extra frames at the start, remove those
+handler_frames = thread.get_thread_frames()[-len(signal_frames):]
+handler_frames = [make_frame_info(f) for f in handler_frames]
 
-self.assertTrue(foundFoo, "Unwinding did not find func that called abort")
+# Check that frames present in both backtraces have the same addresses.
+self.assertEqual(signal_frames, handler_frames, "Common backtrace frames do not match")
 
 # Continue until we exit.
 process.Continue()
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -900,6 +900,17 @@
   // unwind out of sigtramp.
   if (m_frame_type == eTrapHandlerFrame && process) {
 m_fast_unwind_plan_sp.reset();
+
+if (m_sym_ctx_valid) {
+  lldb::PlatformSP platform = process->GetTarget().GetPlatform();
+  unwind_plan_sp = platform->GetTrapHandlerUnwindPlan(
+  process->GetTarget().GetArchitecture().GetTriple(),
+  GetSymbolOrFunctionName(m_sym_ctx));
+
+  if (unwind_plan_sp)
+return unwind_plan_sp;
+}
+
 unwind_plan_sp =
 func_unwinders_sp->GetEHFrameUnwindPlan(process->GetTarget());
 if (!unwind_plan_sp)
Index: lldb/source/Plugins/Platform/Linux/PlatformLinux.h
===
--- lldb/source/Plugins/Platform/Linux/PlatformLinux.h
+++ lldb/source/Plugins/Platform/Linux/PlatformLinux.h
@@ -50,6 +50,9 @@
 
   void CalculateTrapHandlerSymbolNames() override;
 
+  lldb::UnwindPlanSP GetTrapHandlerUnwindPlan(const llvm::Triple ,
+  

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.




Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:295
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);
+  unwind_plan_sp->SetRegisterKind(eRegisterKindDWARF);
+

labath wrote:
> 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?
Except of course, if you end up setting all of the registers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

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

Will respond to these shortly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 382575.
DavidSpickett added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/API/SBFrame.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

Index: lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
===
--- lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
+++ lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
@@ -7,7 +7,21 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
-
+from collections import namedtuple
+
+# Since frames internally point to ExecutionContextRef which itself points
+# to something else, we can't make a deep copy of them from Python.
+# Instead save what we care about for comparison purposes.
+# Ignoring the frame ID because what is frame 0 in the first catch will
+# be frame 2 in the handler backtrace.
+class FrameInfo(namedtuple('FrameInfo', ['sp', 'fp', 'function_name'])):
+# So assert failures are more readable
+def __repr__(self):
+return "SP: 0x{:x} FP: 0x{:x} Fn: {}".format(
+self.sp, self.fp, self.function_name)
+
+def make_frame_info(frame):
+return FrameInfo(frame.GetSP(), frame.GetFP(), frame.GetDisplayFunctionName())
 
 class HandleAbortTestCase(TestBase):
 
@@ -16,8 +30,6 @@
 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.
@@ -47,6 +59,9 @@
 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 = [make_frame_info(f) for f in 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"))
@@ -59,12 +74,20 @@
 self.assertEqual(frame.GetDisplayFunctionName(), "handler", "Unexpected break?")
 
 # Expect that unwinding should find 'abort_caller'
-foundFoo = False
-for frame in thread:
+found_caller = False
+for frame in thread.get_thread_frames():
 if frame.GetDisplayFunctionName() == "abort_caller":
-foundFoo = True
+found_caller = True
+break
+
+self.assertTrue(found_caller, "Unwinding did not find func that called abort")
+
+# The signal handler backtrace has extra frames at the start, remove those
+handler_frames = thread.get_thread_frames()[-len(signal_frames):]
+handler_frames = [make_frame_info(f) for f in handler_frames]
 
-self.assertTrue(foundFoo, "Unwinding did not find func that called abort")
+# Check that frames present in both backtraces have the same addresses.
+self.assertEqual(signal_frames, handler_frames, "Common backtrace frames do not match")
 
 # Continue until we exit.
 process.Continue()
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -900,6 +900,17 @@
   // unwind out of sigtramp.
   if (m_frame_type == eTrapHandlerFrame && process) {
 m_fast_unwind_plan_sp.reset();
+
+if (m_sym_ctx_valid) {
+  lldb::PlatformSP platform = process->GetTarget().GetPlatform();
+  unwind_plan_sp = platform->GetTrapHandlerUnwindPlan(
+  process->GetTarget().GetArchitecture().GetMachine(),
+  GetSymbolOrFunctionName(m_sym_ctx));
+
+  if (unwind_plan_sp)
+return unwind_plan_sp;
+}
+
 unwind_plan_sp =
 func_unwinders_sp->GetEHFrameUnwindPlan(process->GetTarget());
 if (!unwind_plan_sp)
Index: lldb/source/Plugins/Platform/Linux/PlatformLinux.h

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:295
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);
+  unwind_plan_sp->SetRegisterKind(eRegisterKindDWARF);
+

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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




Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:288
+
+  unwind_plan_sp = std::make_shared(eRegisterKindGeneric);
+  unwind_plan_sp->AppendRow(row);

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.



Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:303-305
+  if ((machine == llvm::Triple::ArchType::aarch64 ||
+   machine == llvm::Triple::ArchType::aarch64_be ||
+   machine == llvm::Triple::ArchType::aarch64_32))

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.



Comment at: lldb/source/Target/RegisterContextUnwind.cpp:900
   // 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.

labath wrote:
> this comment
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.
```
?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 382354.
DavidSpickett added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

Index: lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
===
--- lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
+++ lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
@@ -16,8 +16,6 @@
 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.
@@ -47,6 +45,9 @@
 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"))
@@ -58,13 +59,21 @@
 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()
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -900,6 +900,17 @@
   // unwind out of sigtramp.
   if (m_frame_type == eTrapHandlerFrame && process) {
 m_fast_unwind_plan_sp.reset();
+
+if (m_sym_ctx_valid) {
+  lldb::PlatformSP platform = process->GetTarget().GetPlatform();
+  unwind_plan_sp = platform->GetTrapHandlerUnwindPlan(
+  process->GetTarget().GetArchitecture().GetMachine(),
+  GetSymbolOrFunctionName(m_sym_ctx));
+
+  if (unwind_plan_sp)
+return unwind_plan_sp;
+}
+
 unwind_plan_sp =
 func_unwinders_sp->GetEHFrameUnwindPlan(process->GetTarget());
 if (!unwind_plan_sp)
Index: lldb/source/Plugins/Platform/Linux/PlatformLinux.h
===
--- lldb/source/Plugins/Platform/Linux/PlatformLinux.h
+++ lldb/source/Plugins/Platform/Linux/PlatformLinux.h
@@ -50,6 +50,9 @@
 
   void CalculateTrapHandlerSymbolNames() override;
 
+  lldb::UnwindPlanSP GetTrapHandlerUnwindPlan(llvm::Triple::ArchType machine,
+  ConstString name) override;
+
   MmapArgList GetMmapArgumentList(const ArchSpec , lldb::addr_t addr,
   lldb::addr_t length, unsigned prot,
   unsigned flags, lldb::addr_t fd,
Index: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
===
--- lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
+++ lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
@@ -14,9 +14,11 @@
 #include 
 #endif
 
+#include "Utility/ARM64_DWARF_Registers.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Target/Process.h"
 #include 

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:258
+  UnwindPlanSP unwind_plan_sp;
+  if (name != "__kernel_rt_sigreturn")
+return unwind_plan_sp;

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 382287.
DavidSpickett added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

Index: lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
===
--- lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
+++ lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
@@ -16,8 +16,6 @@
 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.
@@ -47,6 +45,9 @@
 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"))
@@ -58,13 +59,21 @@
 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()
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -900,6 +900,17 @@
   // unwind out of sigtramp.
   if (m_frame_type == eTrapHandlerFrame && process) {
 m_fast_unwind_plan_sp.reset();
+
+if (m_sym_ctx_valid) {
+  lldb::PlatformSP platform = process->GetTarget().GetPlatform();
+  unwind_plan_sp = platform->GetTrapHandlerUnwindPlan(
+  process->GetTarget().GetArchitecture().GetMachine(),
+  GetSymbolOrFunctionName(m_sym_ctx));
+
+  if (unwind_plan_sp)
+return unwind_plan_sp;
+}
+
 unwind_plan_sp =
 func_unwinders_sp->GetEHFrameUnwindPlan(process->GetTarget());
 if (!unwind_plan_sp)
Index: lldb/source/Plugins/Platform/Linux/PlatformLinux.h
===
--- lldb/source/Plugins/Platform/Linux/PlatformLinux.h
+++ lldb/source/Plugins/Platform/Linux/PlatformLinux.h
@@ -50,6 +50,9 @@
 
   void CalculateTrapHandlerSymbolNames() override;
 
+  lldb::UnwindPlanSP GetTrapHandlerUnwindPlan(llvm::Triple::ArchType machine,
+  ConstString name) override;
+
   MmapArgList GetMmapArgumentList(const ArchSpec , lldb::addr_t addr,
   lldb::addr_t length, unsigned prot,
   unsigned flags, lldb::addr_t fd,
Index: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
===
--- lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
+++ lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
@@ -14,9 +14,11 @@
 #include 
 #endif
 
+#include "Utility/ARM64_DWARF_Registers.h"
 #include "lldb/Core/Debugger.h"
 #include 

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Symbol/AArch64UnwindInfo.cpp:58
+  unwind_plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
+  unwind_plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);

labath wrote:
> DavidSpickett wrote:
> > If this means "at all instructions within that function" this could be yes, 
> > but no one seems to read this field anyway.
> 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.
So this would be valid at all instructions because at any point in 
`__kernel_rt_sigreturn` the stack pointer is the same.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.




Comment at: lldb/source/Symbol/AArch64UnwindInfo.cpp:58
+  unwind_plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
+  unwind_plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);

DavidSpickett wrote:
> If this means "at all instructions within that function" this could be yes, 
> but no one seems to read this field anyway.
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.



Comment at: lldb/source/Target/RegisterContextUnwind.cpp:900
   // 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.

this comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a reviewer: jasonmolenda.
labath added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid accepted this revision.
omjavaid added a comment.
This revision is now accepted and ready to land.

This look great. Thanks for doing the legit fix.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: omjavaid.
DavidSpickett added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: lldb/source/Symbol/AArch64UnwindInfo.cpp:40
+  // [1]
+  // https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/signal.c
+  int32_t offset = 128 + 8 + 8 + 24 + 128 + 8;

This comment comes from the libunwind change.



Comment at: lldb/source/Symbol/AArch64UnwindInfo.cpp:58
+  unwind_plan_sp->SetSourcedFromCompiler(eLazyBoolYes);
+  unwind_plan_sp->SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
+  unwind_plan_sp->SetUnwindPlanForSignalTrap(eLazyBoolYes);

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112069/new/

https://reviews.llvm.org/D112069

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: kristof.beyls, mgorny.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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

This also updates TestHandleAbort to check that common frames
between signal and signal handler backtrace are in fact the same.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112069

Files:
  lldb/include/lldb/Symbol/AArch64UnwindInfo.h
  lldb/source/Symbol/AArch64UnwindInfo.cpp
  lldb/source/Symbol/CMakeLists.txt
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py

Index: lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
===
--- lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
+++ lldb/test/API/functionalities/signal/handle-abrt/TestHandleAbort.py
@@ -16,8 +16,6 @@
 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.
@@ -47,6 +45,9 @@
 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"))
@@ -58,13 +59,21 @@
 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()
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -12,6 +12,7 @@
 #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"
@@ -900,6 +901,11 @@
   // 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)
Index: lldb/source/Symbol/CMakeLists.txt
===
--- lldb/source/Symbol/CMakeLists.txt
+++ lldb/source/Symbol/CMakeLists.txt
@@ -7,6 +7,7 @@
 endif()
 
 add_lldb_library(lldbSymbol
+  AArch64UnwindInfo.cpp
   ArmUnwindInfo.cpp
   Block.cpp
   CompactUnwindInfo.cpp
Index: lldb/source/Symbol/AArch64UnwindInfo.cpp
===
--- /dev/null
+++ lldb/source/Symbol/AArch64UnwindInfo.cpp
@@ -0,0 +1,63 @@