Hi!
On Fri, Aug 13, 2021 at 10:46:37AM -0300, Raphael Moreira Zinsly wrote:
> * config/rs6000/linux-unwind.h (struct rt_sigframe): Move it to
> outside of get_regs() in order to use it in another function.
Say you do this twice, once for __powerpc64__, once for !__powerpc64__?
> (struct trace_arg): New struct.
> (struct layout): New struct.
> (ppc_backchain_fallback): New function.
> * unwind.inc (_Unwind_Backtrace): Look for _URC_NORMAL_STOP
> code state and call MD_BACKCHAIN_FALLBACK.
> +struct trace_arg
> +{
> + void **array;
> + struct unwind_link *unwind_link;
> + _Unwind_Word cfa;
> + int cnt;
> + int size;
> +};
Did you get this definition from elsewhere? If not, please spell out
"count". Also, count of what? This needs a comment, just like "array"
and "size".
> +/* This is the stack layout we see with every stack frame.
> + Note that every routine is required by the ABI to lay out the stack
> + like this.
> +
> + +----------------+ +-----------------+
> + %r1 -> | %r1 last frame--------> | %r1 last frame--->... --> NULL
> + | | | |
> + | cr save | | cr save |
> + | | | |
> + | (unused) | | return address |
> + +----------------+ +-----------------+
> +*/
"last frame" is an unfortunate choice of words... "previous frame"?
And "r1 previous frame" is nonsensical, r1 by definition points at the
_current_ frame. Just "previous frame" maybe?
Mention near the figure that the CR save is for the 64-bit ABIs only.
but the backchain and the LR save are there always?
> +struct layout
> +{
> + struct layout *next;
> +#ifdef __powerpc64__
> + long int condition_register;
> +#endif
> + void *return_address;
> +};
Call it "cr_save" perhaps, like ABIs do? And "lr_save" maybe? And
"backchain"? The name "layout" could be better, too... "frame_layout"
maybe? "layout" is just too generic.
> +void ppc_backchain_fallback (struct _Unwind_Context *context, void *a)
(Why "ppc"? Why not "power" or "powerpc" or "rs6000"?)
> +{
> + struct layout *current;
> + struct trace_arg *arg = a;
> + int count;
> +
> + /* Get the last address computed and start with the next. */
> + current = context->cfa;
> + current = current->next;
> +
> + for (count = arg->cnt; current != NULL && count < arg->size;
> + current = current->next, count++)
Either write all three expressions in a "for" statement on one line, or
each on its own line? You may want to have only one variable in here
("current" probably), update count in the loop body, for cleaner, easier
to read code.
> + {
> + arg->array[count] = current->return_address;
> +
> + /* Check if the symbol is the signal trampoline and get the interrupted
> + * symbol address from the trampoline saved area. */
No leading * in comments.
> + context->ra = current->return_address;
> + if (current->return_address != NULL && get_regs(context) != NULL)
You can just write
if (current->return_address && get_regs (context))
fwiw (note the space before the "(").
> + {
> + struct rt_sigframe *sigframe = (struct rt_sigframe*) current;
Space before *.
> + if (count + 1 == arg->size)
> + break;
> + arg->array[++count] = (void *) sigframe->uc.rsave.nip;
> + current = (void *) sigframe->uc.rsave.gpr[1];
No two spaces.
This needs a testcase (that can run on all subtargets).
Segher
