On Thu, Jul 17, 2025 at 11:27:45AM +0200, Jens Remus wrote: > On 16.07.2025 23:32, Josh Poimboeuf wrote: > > On Thu, Jul 10, 2025 at 06:35:12PM +0200, Jens Remus wrote: > >> 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! > >> +++ 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
