[PATCH v3 2/5] elf: Add SFrame stack tracing
Claudiu Zissulescu-Ianculescu
claudiu.zissulescu-ianculescu@oracle.com
Thu Apr 17 09:53:55 GMT 2025
Hello Jens,
>> + location. */
>
> Nit: Bad indentation.
>
>> + ra_stack_loc = cfa + ra_offset;
>> + return_addr = read_stack_value (ra_stack_loc);
>> +
>> + ra_lst[i] = (void *)return_addr;
>> +
>> + /* Set up for the next frame. */
>
> Maybe move comment (see below)?
>
>> + /* Get the FP offset from the FRE. If the offset is
>> + unavailable, sets err. */
>> + fp_offset = sframe_fre_get_fp_offset (dctx, frep, &err);
>> + if (err == _URC_NO_REASON)
>> + {
>> + /* Frame is aavailable, get the value stored in the stack
>> + location. */
>
> Nit: s/aavailable/available/. Maybe also s/Frame/FP offset/?
>
>> + fp_stack_loc = cfa + fp_offset;
>> + frame->fp = read_stack_value (fp_stack_loc);
>
> This is the only frame field being set immediately. While it cannot
> occur that frame->fp gets set and below is not executed, I wonder
> whether introducing a local variable frame_ptr (or fp and renaming
> return_addr to ra) and changing below as follows would be cleaner?
>
>> + }
>> +
>
> /* Set up for the next frame. */
>
>> + frame->sp = cfa + SFRAME_SP_VAL_OFFSET;
>
> frame->fp = frame_ptr;
>
Thank you for your feedback! I will send a new version asap, which
includes your fixes and 32bit build & run fixes.
Best wishes,
Claudiu
More information about the Libc-alpha
mailing list