* patch: fix stack unwind through uClibc syscall() on mips
@ 2010-03-27 17:55 Ján Stanček
2010-04-05 15:45 ` Joel Brobecker
2010-04-05 15:51 ` Daniel Jacobowitz
0 siblings, 2 replies; 6+ messages in thread
From: Ján Stanček @ 2010-03-27 17:55 UTC (permalink / raw)
To: gdb-patches
uClibc syscall() is macro which modifies stack before syscall
instruction, gdb is only looking at function prologue and misses the
stack modification made in syscall(). Because of this unwind doesn't
work. Attached is a patch, which is looking at actual $pc and $pc-4,
and in case of syscall it modifies $sp, so mip32_scan_prologue finds
correct values.
Description of bug is also available here:
http://www.listware.net/201003/gnu-gdb/26893.html
2010-03-27 Jan Stancek <jan.stancek@gmail.com>
* mips-tdep.c: fix stack unwind through uClibc syscall() on mips
--- cut ---
--- mips-tdep.c.orig 2010-03-27 17:04:57.000000000 +0100
+++ mips-tdep.c 2010-03-27 18:47:46.000000000 +0100
@@ -1895,6 +1895,44 @@ reset_saved_regs (struct gdbarch *gdbarc
}
}
+/*
+ * fix the $sp by looking around actual $pc
+ * Currently this handles only uClibc syscalls,
+ * which adjust $sp before syscall itsels
+ */
+int mips32_get_sp_adjustment(struct frame_info *this_frame, CORE_ADDR start_pc)
+{
+ CORE_ADDR pc = get_frame_address_in_block (this_frame);
+ struct gdbarch *gdbarch = get_frame_arch (this_frame);
+ unsigned long inst, high_word, low_word, ret;
+ int reg;
+
+ inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
+
+ ret = 0;
+ high_word = (inst >> 16) & 0xffff;
+ low_word = inst & 0xffff;
+ reg = high_word & 0x1f;
+
+ if (high_word == 0x27bd /* addiu $sp,$sp,-i */
+ || high_word == 0x23bd /* addi $sp,$sp,-i */
+ || high_word == 0x67bd) /* daddiu $sp,$sp,-i */
+ {
+ if ( reg == MIPS_SP_REGNUM
+ && (low_word & 0x8000) == 0 /* positive stack adjustment */
+ && (pc-4) > start_pc )
+ {
+ pc = pc - 4;
+ inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
+ if (inst==0x0000000c) /* syscall */
+ {
+ ret = low_word;
+ }
+ }
+ }
+ return ret;
+}
+
/* Analyze the function prologue from START_PC to LIMIT_PC. Builds
the associated FRAME_CACHE if not null.
Return the address of the first instruction past the prologue. */
@@ -1920,9 +1958,12 @@ mips32_scan_prologue (struct gdbarch *gd
/* Can be called when there's no process, and hence when there's no
THIS_FRAME. */
if (this_frame != NULL)
- sp = get_frame_register_signed (this_frame,
- gdbarch_num_regs (gdbarch)
- + MIPS_SP_REGNUM);
+ {
+ sp = get_frame_register_signed (this_frame,
+ gdbarch_num_regs (gdbarch)
+ + MIPS_SP_REGNUM);
+ sp += mips32_get_sp_adjustment(this_frame, start_pc);
+ }
else
sp = 0;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: patch: fix stack unwind through uClibc syscall() on mips
2010-03-27 17:55 patch: fix stack unwind through uClibc syscall() on mips Ján Stanček
@ 2010-04-05 15:45 ` Joel Brobecker
2010-04-06 18:55 ` Ján Stanček
2010-04-05 15:51 ` Daniel Jacobowitz
1 sibling, 1 reply; 6+ messages in thread
From: Joel Brobecker @ 2010-04-05 15:45 UTC (permalink / raw)
To: J?n Stan??ek; +Cc: gdb-patches
Jan,
> uClibc syscall() is macro which modifies stack before syscall
> instruction, gdb is only looking at function prologue and misses the
> stack modification made in syscall(). Because of this unwind doesn't
> work. Attached is a patch, which is looking at actual $pc and $pc-4,
> and in case of syscall it modifies $sp, so mip32_scan_prologue finds
> correct values.
Can you give us a disassembly of the code, please, so that I can
understand a little better what your problem is?
> 2010-03-27 Jan Stancek <jan.stancek@gmail.com>
> * mips-tdep.c: fix stack unwind through uClibc syscall() on mips
Do you have a copyright assignment on file? I could not locate you in
the FSF database, so it looks like not. We cannot accept your patch
until you have one on file, so let me know.
You also need to indicate how you tested this change. In particular,
did you run the testsuite before and after your change?
> +/*
> + * fix the $sp by looking around actual $pc
> + * Currently this handles only uClibc syscalls,
> + * which adjust $sp before syscall itsels
> + */
This is not the GNU style (please have a look at the GNU Coding
Standards, which is available at
http://www.gnu.org/prep/standards/standards.html).
/* Fix the $sp by looking around actual $pc. Currently this handles
only uClibc syscalls, which adjust $sp before syscall itself. */
In particular, use of capital letter for the first word in the sentence,
use of a period at the end of the a sentence. And two spaces after a
period.
I think that the comment can be improved - it seems to be assuming some
form of context that the reader might not have. But we'll see about
that later, if the patch is correct; right now, I really am not sure,
because you are doing the adjustment unconditionally, and perhaps we
should not.
> +int mips32_get_sp_adjustment(struct frame_info *this_frame, CORE_ADDR start_pc)
Style:
int
mips32_get_sp_adjustment (struct frame_info *this_frame, CORE_ADDR start_pc)
And the function should be declared static.
> +{
> + CORE_ADDR pc = get_frame_address_in_block (this_frame);
> + struct gdbarch *gdbarch = get_frame_arch (this_frame);
> + unsigned long inst, high_word, low_word, ret;
> + int reg;
> +
> + inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
> +
> + ret = 0;
> + high_word = (inst >> 16) & 0xffff;
> + low_word = inst & 0xffff;
> + reg = high_word & 0x1f;
> +
> + if (high_word == 0x27bd /* addiu $sp,$sp,-i */
> + || high_word == 0x23bd /* addi $sp,$sp,-i */
> + || high_word == 0x67bd) /* daddiu $sp,$sp,-i */
> + {
> + if ( reg == MIPS_SP_REGNUM
^^^^
Style: No space after '('
> + && (low_word & 0x8000) == 0 /* positive stack adjustment */
> + && (pc-4) > start_pc )
^^^^^^^^ ^^^
|| Style: No space before ')'
||
||
Style: remove useless parens, and add space around '-'.
> + {
> + pc = pc - 4;
> + inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
> + if (inst==0x0000000c) /* syscall */
^^^^ Style: spaces around all binary operators
> + {
> + ret = low_word;
> + }
Style: Remove useless curly braces for if block.
> @@ -1920,9 +1958,12 @@ mips32_scan_prologue (struct gdbarch *gd
> /* Can be called when there's no process, and hence when there's no
> THIS_FRAME. */
> if (this_frame != NULL)
> - sp = get_frame_register_signed (this_frame,
> - gdbarch_num_regs (gdbarch)
> - + MIPS_SP_REGNUM);
> + {
> + sp = get_frame_register_signed (this_frame,
> + gdbarch_num_regs (gdbarch)
> + + MIPS_SP_REGNUM);
> + sp += mips32_get_sp_adjustment(this_frame, start_pc);
> + }
> else
> sp = 0;
The formatting for the call to get_frame_register_signed is incorrect.
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: patch: fix stack unwind through uClibc syscall() on mips
2010-03-27 17:55 patch: fix stack unwind through uClibc syscall() on mips Ján Stanček
2010-04-05 15:45 ` Joel Brobecker
@ 2010-04-05 15:51 ` Daniel Jacobowitz
2010-04-06 20:03 ` Ján Stanček
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Jacobowitz @ 2010-04-05 15:51 UTC (permalink / raw)
To: Ján Stanček; +Cc: gdb-patches
On Sat, Mar 27, 2010 at 06:55:18PM +0100, Ján StanÄek wrote:
> uClibc syscall() is macro which modifies stack before syscall
> instruction, gdb is only looking at function prologue and misses the
> stack modification made in syscall(). Because of this unwind doesn't
> work. Attached is a patch, which is looking at actual $pc and $pc-4,
> and in case of syscall it modifies $sp, so mip32_scan_prologue finds
> correct values.
>
> Description of bug is also available here:
> http://www.listware.net/201003/gnu-gdb/26893.html
Have you considered just annotating the syscall routine with DWARF-2
tables? That's how GLIBC solves this problem. And it doesn't take up
any space in a stripped binary.
[Hmm, good wiki topic?]
It looks like this patch detects the syscall instruction followed by a
single instruction that adjusts sp. It will break if the opposite
SP adjustment was already found by the prologue analyzer.
--
Daniel Jacobowitz
CodeSourcery
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: patch: fix stack unwind through uClibc syscall() on mips
2010-04-05 15:45 ` Joel Brobecker
@ 2010-04-06 18:55 ` Ján Stanček
2010-04-07 17:11 ` Joel Brobecker
0 siblings, 1 reply; 6+ messages in thread
From: Ján Stanček @ 2010-04-06 18:55 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
Thank you for having look at the patch and pointing out the issues.
1. Disassembly
example is below
2. copyright assignment
I was hoping to fall into this category: "Small changes can be
accepted without a copyright assignment form on file."
3. tests
I ran the whole test suite, but I think that affected code is not
tested, the number of mips tests is small. I did most of my testing
manually on real world core files.
Here is an example with select:
(gdb) thread 4
[Switching to thread 4 (process 511)]
#0 0x2ae95c58 in select () from ./lib/libc.so.0
(gdb) bt
#0 0x2ae95c58 in select () from ./lib/libc.so.0
#1 0x00000000 in ?? ()
(gdb) info reg
zero at v0 v1 a0 a1 a2 a3
R0 00000000 80150710 0000102e 00000000 000000c5 759ffc20 00000000 00000000
t0 t1 t2 t3 t4 t5 t6 t7
R8 0000fd00 85fdef60 00000000 00004000 00000000 10003264 00000000 10006f58
s0 s1 s2 s3 s4 s5 s6 s7
R16 001ff000 00000000 00017051 00000000 2ab39330 75801000 00000000 2ab39850
t8 t9 k0 k1 gp sp s8 ra
R24 00000000 2ae95c20 00000000 00000000 2aee94b0 759ffb38 00200000 2e326c60
sr lo hi bad cause pc
0000fd13 00000058 00000000 7ffe7b6e 10800020 2ae95c58
fsr fir hi1 lo1 hi2 lo2 hi3 lo3
00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dspctl
00000000
(gdb) disassemble 0x2ae95c58
Dump of assembler code for function select:
0x2ae95c20 <select+0>: lui gp,0x5
0x2ae95c24 <select+4>: addiu gp,gp,14480
0x2ae95c28 <select+8>: addu gp,gp,t9
0x2ae95c2c <select+12>: addiu sp,sp,-40
0x2ae95c30 <select+16>: sw ra,36(sp)
0x2ae95c34 <select+20>: sw s0,32(sp)
0x2ae95c38 <select+24>: sw gp,16(sp)
0x2ae95c3c <select+28>: lw v0,56(sp)
0x2ae95c40 <select+32>: sw v0,24(sp)
0x2ae95c44 <select+36>: lw v0,24(sp)
0x2ae95c48 <select+40>: addiu sp,sp,-32
0x2ae95c4c <select+44>: sw v0,16(sp)
0x2ae95c50 <select+48>: li v0,4142
0x2ae95c54 <select+52>: syscall
0x2ae95c58 <select+56>: addiu sp,sp,32
0x2ae95c5c <select+60>: lw t9,-32352(gp)
0x2ae95c60 <select+64>: beqz a3,0x2ae95c7c <select+92>
0x2ae95c64 <select+68>: move s0,v0
0x2ae95c68 <select+72>: jalr t9
0x2ae95c6c <select+76>: nop
0x2ae95c70 <select+80>: lw gp,16(sp)
0x2ae95c74 <select+84>: sw s0,0(v0)
0x2ae95c78 <select+88>: li v0,-1
0x2ae95c7c <select+92>: lw ra,36(sp)
0x2ae95c80 <select+96>: lw s0,32(sp)
0x2ae95c84 <select+100>: jr ra
0x2ae95c88 <select+104>: addiu sp,sp,40
0x2ae95c8c <select+108>: nop
End of assembler dump.
(gdb) x/1x $sp+32+36
0x759ffb7c: 0x2e326c60
(gdb) x/1x $sp+36
0x759ffb5c: 0x00000000
(gdb) p/x $ra
$3 = 0x2e326c60
On Mon, Apr 5, 2010 at 5:45 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> Jan,
>
>> uClibc syscall() is macro which modifies stack before syscall
>> instruction, gdb is only looking at function prologue and misses the
>> stack modification made in syscall(). Because of this unwind doesn't
>> work. Attached is a patch, which is looking at actual $pc and $pc-4,
>> and in case of syscall it modifies $sp, so mip32_scan_prologue finds
>> correct values.
>
> Can you give us a disassembly of the code, please, so that I can
> understand a little better what your problem is?
>
>> 2010-03-27 Jan Stancek <jan.stancek@gmail.com>
>> * mips-tdep.c: fix stack unwind through uClibc syscall() on mips
>
> Do you have a copyright assignment on file? I could not locate you in
> the FSF database, so it looks like not. We cannot accept your patch
> until you have one on file, so let me know.
>
> You also need to indicate how you tested this change. In particular,
> did you run the testsuite before and after your change?
>
>> +/*
>> + * fix the $sp by looking around actual $pc
>> + * Currently this handles only uClibc syscalls,
>> + * which adjust $sp before syscall itsels
>> + */
>
> This is not the GNU style (please have a look at the GNU Coding
> Standards, which is available at
> http://www.gnu.org/prep/standards/standards.html).
>
> /* Fix the $sp by looking around actual $pc. Currently this handles
> only uClibc syscalls, which adjust $sp before syscall itself. */
>
> In particular, use of capital letter for the first word in the sentence,
> use of a period at the end of the a sentence. And two spaces after a
> period.
>
> I think that the comment can be improved - it seems to be assuming some
> form of context that the reader might not have. But we'll see about
> that later, if the patch is correct; right now, I really am not sure,
> because you are doing the adjustment unconditionally, and perhaps we
> should not.
>
>> +int mips32_get_sp_adjustment(struct frame_info *this_frame, CORE_ADDR start_pc)
>
> Style:
>
> int
> mips32_get_sp_adjustment (struct frame_info *this_frame, CORE_ADDR start_pc)
>
> And the function should be declared static.
>
>> +{
>> + CORE_ADDR pc = get_frame_address_in_block (this_frame);
>> + struct gdbarch *gdbarch = get_frame_arch (this_frame);
>> + unsigned long inst, high_word, low_word, ret;
>> + int reg;
>> +
>> + inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
>> +
>> + ret = 0;
>> + high_word = (inst >> 16) & 0xffff;
>> + low_word = inst & 0xffff;
>> + reg = high_word & 0x1f;
>> +
>> + if (high_word == 0x27bd /* addiu $sp,$sp,-i */
>> + || high_word == 0x23bd /* addi $sp,$sp,-i */
>> + || high_word == 0x67bd) /* daddiu $sp,$sp,-i */
>> + {
>> + if ( reg == MIPS_SP_REGNUM
> ^^^^
> Style: No space after '('
>
>> + && (low_word & 0x8000) == 0 /* positive stack adjustment */
>> + && (pc-4) > start_pc )
> ^^^^^^^^ ^^^
> || Style: No space before ')'
> ||
> ||
> Style: remove useless parens, and add space around '-'.
>
>> + {
>> + pc = pc - 4;
>> + inst = (unsigned long) mips_fetch_instruction (gdbarch, pc);
>> + if (inst==0x0000000c) /* syscall */
> ^^^^ Style: spaces around all binary operators
>
>> + {
>> + ret = low_word;
>> + }
>
> Style: Remove useless curly braces for if block.
>
>
>> @@ -1920,9 +1958,12 @@ mips32_scan_prologue (struct gdbarch *gd
>> /* Can be called when there's no process, and hence when there's no
>> THIS_FRAME. */
>> if (this_frame != NULL)
>> - sp = get_frame_register_signed (this_frame,
>> - gdbarch_num_regs (gdbarch)
>> - + MIPS_SP_REGNUM);
>> + {
>> + sp = get_frame_register_signed (this_frame,
>> + gdbarch_num_regs (gdbarch)
>> + + MIPS_SP_REGNUM);
>> + sp += mips32_get_sp_adjustment(this_frame, start_pc);
>> + }
>> else
>> sp = 0;
>
> The formatting for the call to get_frame_register_signed is incorrect.
>
> --
> Joel
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: patch: fix stack unwind through uClibc syscall() on mips
2010-04-05 15:51 ` Daniel Jacobowitz
@ 2010-04-06 20:03 ` Ján Stanček
0 siblings, 0 replies; 6+ messages in thread
From: Ján Stanček @ 2010-04-06 20:03 UTC (permalink / raw)
To: Ján Stanček, gdb-patches
On Mon, Apr 5, 2010 at 5:51 PM, Daniel Jacobowitz <dan@codesourcery.com> wrote:
> On Sat, Mar 27, 2010 at 06:55:18PM +0100, Ján Stanček wrote:
>> uClibc syscall() is macro which modifies stack before syscall
>> instruction, gdb is only looking at function prologue and misses the
>> stack modification made in syscall(). Because of this unwind doesn't
>> work. Attached is a patch, which is looking at actual $pc and $pc-4,
>> and in case of syscall it modifies $sp, so mip32_scan_prologue finds
>> correct values.
>>
>> Description of bug is also available here:
>> http://www.listware.net/201003/gnu-gdb/26893.html
>
> Have you considered just annotating the syscall routine with DWARF-2
> tables? That's how GLIBC solves this problem. And it doesn't take up
> any space in a stripped binary.
No, I haven't. I'm not sure I understand how this can be done. Also I
assume this wouldn't help with existing core files made with
unmodified uClibc.
>
> [Hmm, good wiki topic?]
>
> It looks like this patch detects the syscall instruction followed by a
> single instruction that adjusts sp. It will break if the opposite
> SP adjustment was already found by the prologue analyzer.
Isn't the opposite SP adjustment found each time? The loop goes from
start_pc to current pc (syscall instruction), so the opposite SP
adjustment should be found. As I understand it, the register offsets
are saved using current SP:
set_reg_offset (gdbarch, this_cache, reg, sp + low_word);
and all SP adjustments don't really have any effect on these. SP
adjustments affect only frame_offset (this_cache->base).
>
> --
> Daniel Jacobowitz
> CodeSourcery
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: patch: fix stack unwind through uClibc syscall() on mips
2010-04-06 18:55 ` Ján Stanček
@ 2010-04-07 17:11 ` Joel Brobecker
0 siblings, 0 replies; 6+ messages in thread
From: Joel Brobecker @ 2010-04-07 17:11 UTC (permalink / raw)
To: J?n Stan??ek; +Cc: gdb-patches
> 1. Disassembly
> example is below
Thanks - I actually got myself confused as to where the SP adjustment
happened, so it's good to see the assembly.
> 2. copyright assignment
> I was hoping to fall into this category: "Small changes can be
> accepted without a copyright assignment form on file."
Unfortunately, this one is beyond what can be accepted as a small
change (the guideline is 10-15 lines max).
Overall, I don't know mips well enough and I am a little nervous about
the approach you are taking (it feels too general an approach for such
a specific situation).
Hopefully Daniel can give you more insight?
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-07 17:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-27 17:55 patch: fix stack unwind through uClibc syscall() on mips Ján Stanček
2010-04-05 15:45 ` Joel Brobecker
2010-04-06 18:55 ` Ján Stanček
2010-04-07 17:11 ` Joel Brobecker
2010-04-05 15:51 ` Daniel Jacobowitz
2010-04-06 20:03 ` Ján Stanček
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).