Merge pull request #30 from ClickHouse/sigret

Fix unwind from signal handler on ARM

src/DwarfInstructions.hpp

@@ -381,10 +381,13 @@ int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, pint_t pc,
       // Return address is address after call site instruction, so setting IP to
       // that simulates a return.
       //
-      // In case of this is frame of signal handler, the IP should be
-      // incremented, because the IP saved in the signal handler points to
-      // first non-executed instruction, while FDE/CIE expects IP to be after
-      // the first non-executed instruction.
+      // The +-1 situation is subtle.
+      // Return address points to the next instruction after the `call`
+      // instruction, but logically we're "inside" the call instruction, and
+      // FDEs are constructed accordingly.
+      // So our FDE parsing implicitly subtracts 1 from the address.
+      // But for signal return, there's no `call` instruction, and
+      // subtracting 1 would be incorrect. So we add 1 here to compensate.
       newRegisters.setIP(returnAddress + cieInfo.isSignalFrame);
 
       // Simulate the step by replacing the register set with the new ones.

src/DwarfParser.hpp

@@ -452,7 +452,14 @@ bool CFI_Parser<A>::parseFDEInstructions(A &addressSpace,
                            ")\n",
                            static_cast<uint64_t>(instructionsEnd));
 
-    // see DWARF Spec, section 6.4.2 for details on unwind opcodes
+    // see DWARF Spec, section 6.4.2 for details on unwind opcodes;
+    //
+    // Note that we're looking for the PrologInfo for address `codeOffset - 1`,
+    // hence '<' instead of '<=" in `codeOffset < pcoffset`
+    // (compare to DWARF Spec section 6.4.3 "Call Frame Instruction Usage").
+    // The -1 accounts for the fact that function return address points to the
+    // next instruction *after* the `call` instruction, while control is
+    // logically "inside" the `call` instruction.
     while ((p < instructionsEnd) && (codeOffset < pcoffset)) {
       uint64_t reg;
       uint64_t reg2;

src/UnwindCursor.hpp

@@ -2760,7 +2760,15 @@ int UnwindCursor<A, R>::stepThroughSigReturn(Registers_arm64 &) {
     _registers.setRegister(UNW_AARCH64_X0 + i, value);
   }
   _registers.setSP(_addressSpace.get64(sigctx + kOffsetSp));
-  _registers.setIP(_addressSpace.get64(sigctx + kOffsetPc));
+
+  // The +1 story is the same as in DwarfInstructions::stepWithDwarf()
+  // (search for "returnAddress + cieInfo.isSignalFrame" or "Return address points to the next instruction").
+  // This is probably not the right place for this because this function is not necessarily used
+  // with DWARF. Need to research whether the other unwind methods have the same +-1 situation or
+  // are off by one.
+  pint_t returnAddress = _addressSpace.get64(sigctx + kOffsetPc);
+  _registers.setIP(returnAddress + 1);
+
   _isSignalFrame = true;
   return UNW_STEP_SUCCESS;
 }