This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch, nios2] clean up prologue/epilogue detection code
- From: Sandra Loosemore <sandra at codesourcery dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Mon, 24 Nov 2014 16:52:26 -0700
- Subject: Re: [patch, nios2] clean up prologue/epilogue detection code
- Authentication-results: sourceware.org; auth=none
- References: <545EA0C1 dot 6050104 at codesourcery dot com> <87bnoeif74 dot fsf at codesourcery dot 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