Re: [PATCH v2 01/21] powerpc: Create stackframe information on syscall

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 06/05/2018 23:49, Zack Weinberg wrote:
> On 26 Feb 2018, Adhemerval Zanella wrote:
>> This patch adds a minimal stackframe creation on powerpc syscall
>> implementation so backtrace works correctly on a signal handler.
> 
> I don't know powerpc well enough to know if this should be necessary.
> I think one of the powerpc arch maintainers should comment.  I do have
> a couple questions:
> 
>> +#ifdef __powerpc64__
>> +	stdu r1, -FRAME_MIN_SIZE (r1)
>> +	cfi_adjust_cfa_offset (FRAME_MIN_SIZE)
>> +#else
>> +	stwu r1,-16(1)
>> +	cfi_def_cfa_offset (16)
>> +#endif
> 
> Why does this use cfa_adjust_cfa_offset for 64-bit but _def_ for 32-bit?

I am not well versed in CFI definition, so I was basically followed what
compiler is spilling in a usual function call back when I coded it.

> 
>> +#ifdef __powerpc64__
>> +	addi r1, r1, FRAME_MIN_SIZE
>> +#else
>> +	addi r1,r1,16
>> +#endif
> 
> If FRAME_MIN_SIZE were defined for ppc32, we could reduce the
> ifdeffage here.
> 
>> +	cfi_def_cfa_offset (0)
>>       sc
>>       PSEUDO_RET
> 
> Shouldn't the stack adjustments be undone _after_ the 'sc'
> instruction?  Actually, is it possible that a single
> cfi_def_cfa_offset (0) at the beginning of the function is all that's
> really needed here?  It seems like backtrace should be able to handle
> leaf frames in general...

Indeed the single 'cfi_def_cfa_offset (0)' is suffice, I will change the
patch accordingly in next iteration.