Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Joel Brobecker <brobecker@adacore.com>]

Jose,

(a small request to avoid the extra-large indentation when pinging -
thank you!)

>         2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
>         
>         	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
>         	(X_RETTURN): New macro.
>         	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
>         
>         	* sparc64-tdep.c (sparc64_init_abi): Hook
>         	sparc_in_function_epilogue_p.

Regarding testing this function on sparc32:

Can you tell us which testcase in our testsuite this patch fixes?
Although I am allowed to run the testsuite, I can still run individual
testcases by hand (if not too complex, of course). Otherwise, would
you have a small reproducer I could use to test on sparc32?

Comments about the patch below.

> +/* Macros to identify some instructions.  */
> +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))

Can your comment say a little more precisely what instruction it
identifies? I think the parens around the equality operators are
superfluous and should be removed.

> +/* Return true if we are in a function's epilogue, i.e. after an
> +   instruction that destroyed a function's stack frame.  */
> +
> +int
> +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{

The general principle for function meant to be used as gdbarch callbacks
is just to say:

/* Implement "in_function_epilogue_p".  */

That way, if we ever change the function's prototype, we don't have
to update the documentation everywhere.  This callback should only
be documented in gdbarch.sh (and repeated in gdbarch.h, which is
generated from gdbarch.sh).

In your case, since there are 32bit and 64bit versions, you can add
something like, for instance:

   This implementation works on both sparc32 and sparc64.

> +  /* This function must return true if we are one instruction after an
> +     instruction that destroyed the stack frame of the current
> +     function.  The SPARC instructions used to restore the callers
> +     stack frame are RESTORE and RETURN/RETT.
> +
> +     Of these RETURN/RETT is a branch instruction and thus we return
> +     true if we are in its delay slot.
> +
> +     RESTORE is almost always found in the delay slot of a branch
> +     instruction that transfers control to the caller, such as JMPL.
> +     Thus the next instruction is in the caller frame and we don't
> +     need to do anything about it.  */
> +
> +  unsigned int insn = sparc_fetch_instruction (pc - 4);  
> +  return X_RETTURN (insn);

Small quirk of the GDB Coding Style: We require an empty line between
local variable declarations and the statements after. Also, I notice
there are trailing spaces.

>    set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
>  
> +  /* Detect whether PC is in function epilogue.  */
> +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
> +

I would normally not comment on this, but since I've made other
comments, I'll ask that the comment be revmoved, and that you
avoid the empty line between the call to set_gdbarch_skip_prologue
just above and the call to set_gdbarch_in_function_epilogue_p that
you are adding.

I am asking because the comment in this case is only repeating
the obvious without explaining why it's necessary. So it feels
superfluous and better without.

Thank you,
-- 
Joel