On Thu, Jul 17, 2025 at 11:27:45AM +0200, Jens Remus wrote:

On 16.07.2025 23:32, Josh Poimboeuf wrote:

quoted

On Thu, Jul 10, 2025 at 06:35:12PM +0200, Jens Remus wrote:

quoted

Most architectures define their CFA as the value of the stack pointer
(SP) at the call site in the previous frame, as suggested by the DWARF
standard:

  CFA = <SP at call site>

Enable unwinding of user space for architectures, such as s390, which
define their CFA as the value of the SP at the call site in the previous
frame with an offset:

  CFA = <SP at call site> + offset

This is a bit confusing, as the comment and code define it as

    SP = CFA + offset

Should the commit log be updated to match that?

I agree that the commit message is confusing. Would it help if I replace
it with the following:

Most architectures define their CFA as the value of the stack pointer
(SP) at the call site in the previous frame, as suggested by the DWARF
standard.  Therefore the SP at call site can be unwound using an
implicitly assumed value offset from CFA rule with an offset of zero:

  .cfi_val_offset <SP>, 0

As a result the SP at call site computes as follows:

  SP = CFA

Enable unwinding of user space for architectures, such as s390, which
define their CFA as the value of the SP at the call site in the previous
frame with an offset.  Do so by enabling architectures to override the
default SP value offset from CFA of zero with an architecture-specific
one:

  .cfi_val_offset <SP>, offset
  
So that the SP at call site computes as follows:

  SP = CFA + offset

Looks good to me, thanks!

quoted

quoted

+++ b/arch/x86/include/asm/unwind_user.h

@@ -8,6 +8,7 @@
 	.cfa_off	= (s32)sizeof(long) *  2,				\
 	.ra_off		= (s32)sizeof(long) * -1,				\
 	.fp_off		= (s32)sizeof(long) * -2,				\
+	.sp_val_off	= (s32)0,						\

IIUC, this is similar to ra_off and fp_off in that its an offset from
the CFA.  Can we call it "sp_off"?

My intent was to use the terminology from DWARF CFI (i.e. "offset(N)"
and "val_offset(N)") and the related assembler CFI directives:

  .cfi_offset register, offset:  Previous value of register is saved at
                                 offset from CFA.

  .cfi_val_offset register, offset:  Previous value of register is
                                     CFA + offset. 

The distinction between "cfi_offset" and "cfi_val_offset" is confusing,
unless one already happens to know CFI syntax (not likely for us kernel
developers).

We don't need to match the DWARF CFI directive naming.  Let's instead
optimize for readability.

I think "sp_off" is fine here, its semantics are similar to the existing
cfa_off field.

The semantics of ra_off and fp_off are different, but those are getting
removed in favor of nested structs in a later patch anyway.

-- 
Josh