Correctly update isSignalFrame when unwinding the stack via dwarf.

[saugustine]

Summary

A "signal frame" is a function or block of code where execution arrives via a signal or interrupt, rather than via a normal call instruction. In fact, a particular instruction is interrupted by the signal and needs to be restarted. Therefore, when the signal handler is complete, execution needs to return to the interrupted instruction, rather than the instruction immediately following the call instruction, as in a normal call.

Stack unwinders need to know this to correctly unwind signal frames. Dwarf handily provides an "S" in the CIE augmentation string to describe this case, and the libunwind API provides various functions to for unwinders to determine it,.

The llvm libunwind implementation correctly sets it's internal variable "isSignalFrame" when initializing an unwind context. However, upon stepping up the stack, the current implementation correctly reads the augmentation string and sets it in the CIE info (which it then discards), libunwind doesn't update it's internal unwind context data structure.

This change fixes that, and provides compatibility with both the canonical libunwind and the libgcc implementation.

[lhames]

Looks sane to me, but I'm not a cfi_signal_frame expert.

Do you know the ipBefore == isSignalFrame relationship is true on all platforms?

[saugustine]

In D69677#1736320, @lhames wrote:

Looks sane to me, but I'm not a cfi_signal_frame expert.

Do you know the ipBefore == isSignalFrame relationship is true on all platforms?

I think so. The default inside the canonical libunwind on github has it that way, as does the default inside libgcc (unwind-dw2.c). Strangely, inside libgcc several targets override the default implementation of Unwind_GetIPInfo, but all set ipBefore equivalent to isSignalFrame.

So if there is an exception. I don't know what it would be.

[echristo]

Outside of the code factoring which I haven't looked at I thought I'd link this:

https://www.airs.com/blog/archives/460

as the best description I've got for what cfi_signal_frame is supposed to be doing here - look for the bit with the augmentation value of 'S' in there for the description.

Sterling: Probably wouldn't hurt to describe this much more in the commit/patch message for future archaeologists.

[echristo]

LGTM. I know that lhames was in the process of reviewing so wait for an OK there please.

[lhames]

Ok. So the documentation (sparse as it is) syncs up with what you're seeing in existing implementations. Looks good to me. :)

[MaskRay]

libunwind/src/UnwindLevel1-gcc-ext.c

226

"Negative" in the comment but <= 0 in the code?

[MaskRay]

libunwind/src/UnwindLevel1-gcc-ext.c

226

_LIBUNWIND_HIDDEN int __unw_is_signal_frame(unw_cursor_t *cursor) {
  _LIBUNWIND_TRACE_API("__unw_is_signal_frame(cursor=%p)",
                       static_cast<void *>(cursor));
  AbstractUnwindCursor *co = (AbstractUnwindCursor *)cursor;
  return co->isSignalFrame(); /////// this returns bool
}

Does it fail if we simply do *ipBefore = __unw_is_signal_frame((unw_cursor_t *)context);, like libgcc/unwind-dw2.c?

[MaskRay]

libunwind/test/signal_frame.pass.cpp

2

It'd be nice to add a file-level comment explaining the purpose of the test. The filename signal_frame.pass encodes some information but it is probably not very obvious.

[saugustine]

• Add file header and explanatory comment.

[saugustine]

Updated for all comments, and committing shortly.

libunwind/src/UnwindLevel1-gcc-ext.c

226

The documented interface for __unw_is_signal_frame is an integer, with negative meaning any of several possible errors. I could split this conditional into two:

if (isSignalFrame < 0 || isSignalFrame == 0)...

but that seems a little absurd.

This implementation of __unw_is_signal_frame doesn't actually return negative values: True, but relying on that would be ignoring the documented interface.

Libgcc is subtly different than the code you actually proposed. It uses _Unwind_isSignalFrame (not __unw_is_signal_frame) which has a documented interface of zero or one, and there have been various cleanups in gcc land when various bits of code treat the two the same.

I could switch to _Unwind_isSignalFrame, of course, but most of the code in this file uses the __unw_ interface and I chose to be consistent with the surrounding code.

[MaskRay]

libunwind/test/signal_frame.pass.cpp

2

I don't know why we need the header. Most clang/lld/... tests don't have a license notice.

17

Delete void (this is C++)/

[lhames]

libunwind/src/UnwindLevel1-gcc-ext.c

226

FWIW

if (!isSignalFrame || isSignalFrame < 0) ...

seems totally reasonable to me. I actually did a double-take on the <= 0 test the first time I saw it too.

libunwind/src/DwarfInstructions.hpp

 /// architecture
 template <typename A, typename R>
 class DwarfInstructions {
 public:
   typedef typename A::pint_t pint_t;
   typedef typename A::sint_t sint_t;
 
   static int stepWithDwarf(A &addressSpace, pint_t pc, pint_t fdeStart,
-                           R &registers);
+                           R &registers, bool &isSignalFrame);
 
 private:
 
   enum {
     DW_X86_64_RET_ADDR = 16
   };
 
   enum {
case CFI_Parser<A>::kRegisterInRegister:
     // FIX ME
     break;
   }
   _LIBUNWIND_ABORT("unsupported restore location for vector register");
 }
 
 template <typename A, typename R>
 int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, pint_t pc,
-                                           pint_t fdeStart, R &registers) {
+                                           pint_t fdeStart, R &registers,
+                                           bool &isSignalFrame) {
   FDE_Info fdeInfo;
   CIE_Info cieInfo;
   if (CFI_Parser<A>::decodeFDE(addressSpace, fdeStart, &fdeInfo,
                                &cieInfo) == NULL) {
     PrologInfo prolog;
     if (CFI_Parser<A>::parseFDEInstructions(addressSpace, fdeInfo, cieInfo, pc,
                                             R::getArch(), &prolog)) {
       // get pointer to cfa (architecture specific)
if (CFI_Parser<A>::parseFDEInstructions(addressSpace, fdeInfo, cieInfo, pc,
             return UNW_EBADREG;
         }
       }
 
       // By definition, the CFA is the stack pointer at the call site, so
       // restoring SP means setting it to CFA.
       newRegisters.setSP(cfa);
 
+      isSignalFrame = cieInfo.isSignalFrame;
+
 #if defined(_LIBUNWIND_TARGET_AARCH64)
       // If the target is aarch64 then the return address may have been signed
       // using the v8.3 pointer authentication extensions. The original
       // return address needs to be authenticated before the return address is
       // restored. autia1716 is used instead of autia as autia1716 assembles
       // to a NOP on pre-v8.3a architectures.
       if ((R::getArch() == REGISTERS_ARM64) &&
           prolog.savedRegisters[UNW_ARM64_RA_SIGN_STATE].value) {

libunwind/src/UnwindCursor.hpp

 
 #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
   bool getInfoFromDwarfSection(pint_t pc, const UnwindInfoSections &sects,
                                             uint32_t fdeSectionOffsetHint=0);
   int stepWithDwarfFDE() {
     return DwarfInstructions<A, R>::stepWithDwarf(_addressSpace,
                                               (pint_t)this->getReg(UNW_REG_IP),
                                               (pint_t)_info.unwind_info,
-                                              _registers);
+                                              _registers, _isSignalFrame);
   }
 #endif
 
 #if defined(_LIBUNWIND_SUPPORT_COMPACT_UNWIND)
   bool getInfoFromCompactEncodingSection(pint_t pc,
                                             const UnwindInfoSections &sects);
   int stepWithCompactEncoding() {
   #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)

libunwind/src/UnwindLevel1-gcc-ext.c

 
 
 /// Called by personality handler during phase 2 to get instruction pointer.
 /// ipBefore is a boolean that says if IP is already adjusted to be the call
 /// site address.  Normally IP is the return address.
 _LIBUNWIND_EXPORT uintptr_t _Unwind_GetIPInfo(struct _Unwind_Context *context,
                                               int *ipBefore) {
   _LIBUNWIND_TRACE_API("_Unwind_GetIPInfo(context=%p)", (void *)context);
+  int isSignalFrame = __unw_is_signal_frame((unw_cursor_t *)context);
+  // Negative means some kind of error (probably UNW_ENOINFO), but we have no
+  // good way to report that, and this maintains backward compatibility with the
+  // implementation that hard-coded zero in every case, even signal frames.
+  if (isSignalFrame <= 0)
     *ipBefore = 0;
+  else
+    *ipBefore = 1;
   return _Unwind_GetIP(context);
 }
 
 #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
 
 /// Called by programs with dynamic code generators that want
 /// to register a dynamically generated FDE.
 /// This function has existed on Mac OS X since 10.4, but

libunwind/test/signal_frame.pass.cpp

+// -*- 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
+//
+//===----------------------------------------------------------------------===//
+
+// Ensure that functions marked as signal frames are reported as such.
+
+#include <assert.h>
+#include <stdlib.h>
+#include <libunwind.h>
+
+int main(void) {
+  asm(".cfi_signal_frame");
+  unw_cursor_t cursor;
+  unw_context_t uc;
+  unw_getcontext(&uc);
+  unw_init_local(&cursor, &uc);
+  assert(unw_step(&cursor) > 0);
+  assert(unw_is_signal_frame(&cursor));
+  return 0;
+}