Re: [patch, nios2] clean up prologue/epilogue detection code

[Sandra Loosemore <sandra@codesourcery.com>]

On 11/10/2014 08:33 PM, Yao Qi wrote:

> These fixes should be in separate patches.  I'd like to see this patch
> is split to a series which includes the following patches,
>
>   1. add new helpers and rewrite existing functions with these new
>   helpers,
>   2. permit more than one stack adjustment instruction to appear in
>   epilogue
>   3. detect some additional prologue instruction patterns,

Hmmm, OK (although this required rewriting code for part 1 that is 
promptly going to get thrown away again with parts 2 and 3).

> There should be an empty line between the end of comment and the function.

Fixed throughout the patch.

>> +
>> +/* Match and disassemble an ADD-type instruction, with 3 register operands.
>> +   Returns true on success, and fills in the operand pointers.  */
>> +static int
>> +nios2_match_add (uint32_t insn,
>> +		 const struct nios2_opcode *op,
>> +		 unsigned long mach,
>> +		 int *ra, int *rb, int *rc)
>
> Is there any reason you put these parameters in different lines?  We can
> place them on the same line like:
>
> static int
> nios2_match_add (uint32_t insn, const struct nios2_opcode *op,
> 		 unsigned long mach, int *ra, int *rb, int *rc)
>
> then the function can be shorter.

Fixed here and elsewhere.

> An empty line is needed between declaration and statement.

Fixed.

>> +  /* We found a whole lot of stack adjustments.  Be safe, tell GDB that
>> +     the epilogue stack unwind is in progress even if we didn't see a
>> +     return yet.  */
>> +  if (ninsns == max_insns)
>> +    return 1;
>
> I am confused by the comments and code.  Infer from your code above that
> GCC emits arbitrary number of sp-adjusting continuous instructions in
> epilogue, is that true?

No, GCC doesn't emit an arbitrary number of SP-adjusting instructions, 
but what should GDB do if it gets some code that looks like that?  I've 
rewritten the comment to make it more clear that this is a "shouldn't 
happen" situation.

> Looks the epilogue detection isn't precise nor accurate.  Supposing we
> have an epilogue like this below,
>
>     1. ADDI sp, sp, n
>     2. RET
>
> if pc points at insn #1, nios2_in_epilogue_p returns true, which isn't
> precise, because the stack frame isn't destroyed at the moment pc points
> at insn #1.  gdbarch in_function_epilogue_p's name is misleading, and
> better to be renamed to stack_frame_destroyed_p (it's on my todo list).
>
> if pc points at insn #2, nios2_in_epilogue_p returns false, which isn't
> accurate.

I think that you have gotten confused by the pc and insn variables being 
updated out of sync in the scanning loop.  I have rewritten the loop 
code and the comments to make it more obvious that scanning for stack 
adjustments starts at the instruction before current_pc.  It doesn't 
matter if there have been additional stack adjustments in the sequence 
of instructions before that -- we only need to see one to know that the 
stack teardown is already in progress at current_pc.

> Why do we change from byte_order_for_code to byte_order?

Leftover bits from some other changes that aren't ready to commit yet. 
I've put it back the way it was.

New patch set coming up shortly.

-Sandra