[PATCH,V2 3/3] doc: sframe: add appendix for generating stack traces

[Indu Bhagat]

On 7/1/24 06:39, Jens Remus wrote:
> Hello Indu,
> 
> I very much appreciate your effort in enhancing the SFrame 
> documentation! In general it looks great! There are just a few minor 
> flaws in the unwinding logic sample code.
> 
> Am 29.06.2024 um 08:30 schrieb Indu Bhagat:
>> [Changes in V2]
>>   - Use @example instead of @code.
>>   - Use Title case consistently.
>> [No changes in V2]
>>
>> Add an appendix to provide a rough outline to show how to generate stack
>> traces using the SFrame format.  Such content should hopefully aid the
>> reader assimmilate the information in the specification.
>>
>> libsframe/
>>     * doc/sframe-spec.texi: Add new appendix.
>> ---
>>   libsframe/doc/sframe-spec.texi | 60 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/libsframe/doc/sframe-spec.texi 
>> b/libsframe/doc/sframe-spec.texi
>> index 4d1d12c8598..fccb404f59e 100644
>> --- a/libsframe/doc/sframe-spec.texi
>> +++ b/libsframe/doc/sframe-spec.texi
>> @@ -54,6 +54,9 @@ low-overhead mechanism to generate stack traces.
>>   * SFrame Section::
>>   * ABI/arch-specific Definition::
>> +Appendices
>> +* Generating Stack Traces using SFrame::
>> +
>>   Indices
>>   * Concept Index::
>>   @end menu
>> @@ -824,6 +827,63 @@ Hence, in summary:
>>   @item 3 @tab FP = CFA + offset3
>>   @end multitable
>> +@node Generating Stack Traces using SFrame
>> +@appendix Generating Stack Traces using SFrame
>> +
>> +Following pseudocode highlights how SFrame provides a simple, fast and
>> +low-overhead mechanism to generate stack traces.
>> +
>> +For sake of brevity, only the core algorithm to get the @emph{next 
>> frame}
>> +(given the current frame) is outlined below.  Needless to say that for
>> +generating accurate and useful stack traces, several other aspects 
>> will need
>> +attention: finding and decoding bits of SFrame section(s) in the program
>> +binary, symbolization of addresses, to name a few.  That said, 
>> establishing the
>> +first frame should be trivial:
>> +
>> +@example
>> +    // frame 0
>> +    frame->pc = current_IP;
>> +    frame->sp = get_reg_value (REG_SP);
>> +    frame->fp = get_reg_value (REG_FP);
>> +@end example
>> +
>> +Next, given frame N, generating stack trace needs us to get frame 
>> N+1.  This
>> +can be done as follows:
>> +
>> +@code {int err = get_next_frame (&frame, pc, sp, fp);}
>> +
>> +where @code{get_next_frame} populates the RA, SP, and FP values in 
>> the provided
>> +@code{frame} object and return the error code, if any.  In the 
>> following code,
>> +the @code{sframe_*} functions fetch information from the SFrame section.
>> +
>> +@example
>> +    fre = sframe_find_fre (pc);
>> +    if (fre)
> 
>          prev_frame = frame;
> 
>> +        base_reg = fre->base_fp_p ? REG_FP : REG_SP;
> 
> My understanding of the previous code is that REG_FP and REG_SP identify 
> register numbers and not their respective contents.
> 
>          base_reg_val = fre->base_fp_p ? prev_frame->fp : prev_frame->sp;
> 

Ah yes, I should use the passed fp, and sp args to the function.

>> +        cfa_offset = sframe_fre_get_cfa_offset (fre);
>> +        // Get fixed RA offset or stack offset from FRE as applicable.
>> +        ra_offset = sframe_fre_get_ra_offset (fre);
>> +        fp_offset = sframe_fre_get_fp_offset (fre);
>> +
>> +        cfa = base_reg + cfa_offset;
> 
> Using above notion this calculates the sum of a register number with the 
> CFA offset.
> 
>          cfa = base_reg_val + cfa_offset;
> 

Done.

>> +        frame->sp = cfa;
>> +
>> +        ra_stack_loc = cfa + ra_offset;
>> +        // Get the address stored in the stack location.
>> +        frame->pc = read_value (ra_stack_loc);
>> +
>> +        if (fp_offset is VALID)
>> +            fp_stack_loc = cfa + fp_offset;
>> +            // Get the value stored in the stack location.
>> +            frame->fp = read_value (fp_stack_loc);
>> +        else
>> +            // continue to use the value of fp as it has not
>> +            // been clobbered by the current frame yet.
>> +            frame->fp = fp;
> 
> fp has not been introduced so far.
> 
>              frame->fp = prev_frame->fp;
> 

I have adapted this pseudocode to address these concerns and committed 
the series.

Thanks for reviewing.
Indu

>> +    else
>> +        ret = ERR_NO_SFRAME_FRE;
>> +@end example
>> +
>>   @node Concept Index
>>   @unnumbered Concept Index
> 
> Regards,
> Jens