[libcxx-commits] [PATCH] D124765: [libunwind][SystemZ] Unwind out of signal handlers

[Fangrui Song via Phabricator via libcxx-commits]

MaskRay added inline comments.


================
Comment at: libunwind/src/UnwindCursor.hpp:2667
+  // specific instruction (see below). Typically the trampoline comes from the
+  // vDSO (i.e. the __kernel_[rt_]sigreturn function). A libc might provide its
+  // own restorer function, though, or user-mode QEMU might write a trampoline
----------------
Does s390 use both `__kernel_[rt_]sigreturn`? It seems that the Linux kernel source references both. For aarch64 there is only the `rt_` version.

Just a note for you to double check the comment:)


================
Comment at: libunwind/src/UnwindCursor.hpp:2684
+int UnwindCursor<A, R>::stepThroughSigReturn(Registers_s390x &) {
+  // Determine current SP and CFA (offset by 160).
+  const pint_t sp = static_cast<pint_t>(this->getReg(UNW_REG_SP));
----------------
Best to leave a comment where 160 comes from. It can be a kernel source filename.


================
Comment at: libunwind/src/UnwindCursor.hpp:2691
+  const pint_t pc = static_cast<pint_t>(this->getReg(UNW_REG_IP));
+  uint16_t inst = _addressSpace.get16(pc);
+
----------------



================
Comment at: libunwind/src/UnwindCursor.hpp:2731
+  // Restore all registers.
+  for (int i = 0; i <= 15; ++i) {
+    uint64_t value = _addressSpace.get64(pSigctx + kOffsetGprs +
----------------
Prefer [a,b) ranges


================
Comment at: libunwind/src/UnwindCursor.hpp:2736
+  }
+  for (int i = 0; i <= 15; ++i) {
+    static int fpr[16] = {
----------------
Prefer [a,b) ranges


================
Comment at: libunwind/src/UnwindCursor.hpp:2737
+  for (int i = 0; i <= 15; ++i) {
+    static int fpr[16] = {
+      UNW_S390X_F0, UNW_S390X_F1, UNW_S390X_F2, UNW_S390X_F3,
----------------



================
Comment at: libunwind/test/signal_unwind.pass.cpp:11
 // Ensure that the unwinder can cope with the signal handler.
-// REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}})
+// REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}} || target={{s390x-.+}})
 
----------------
Use an alphabetical order: aarch64, s390x, x86_64


================
Comment at: libunwind/test/unwind_leaffunction.pass.cpp:11
 // Ensure that leaf function can be unwund.
-// REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}})
+// REQUIRES: linux && (target={{aarch64-.+}} || target={{x86_64-.+}} || target={{s390x-.+}})
 
----------------
Use an alphabetical order: aarch64, s390x, x86_64


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

https://reviews.llvm.org/D124765