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

Sandra Loosemore
2014-11-24T23:52:38Z

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