From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10913 invoked by alias); 6 Apr 2010 18:55:45 -0000 Received: (qmail 10850 invoked by uid 22791); 6 Apr 2010 18:55:37 -0000 X-SWARE-Spam-Status: No, hits=1.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SARE_BAYES_5x8,SARE_BAYES_6x8,SARE_BAYES_7x8,SARE_MSGID_LONG45,TW_EQ X-Spam-Check-By: sourceware.org Received: from mail-fx0-f217.google.com (HELO mail-fx0-f217.google.com) (209.85.220.217) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 06 Apr 2010 18:55:32 +0000 Received: by fxm9 with SMTP id 9so237642fxm.4 for ; Tue, 06 Apr 2010 11:55:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.239.172.199 with HTTP; Tue, 6 Apr 2010 11:55:28 -0700 (PDT) In-Reply-To: <20100405154507.GC19194@adacore.com> References: <737ad3551003271055o91a78i3f5ff305b927e441@mail.gmail.com> <20100405154507.GC19194@adacore.com> Date: Tue, 06 Apr 2010 18:55:00 -0000 Received: by 10.239.193.84 with SMTP id h20mr671008hbi.8.1270580128776; Tue, 06 Apr 2010 11:55:28 -0700 (PDT) Message-ID: Subject: Re: patch: fix stack unwind through uClibc syscall() on mips From: =?UTF-8?B?SsOhbiBTdGFuxI1law==?= To: Joel Brobecker Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-04/txt/msg00105.txt.bz2 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 000000= 00 t0 t1 t2 t3 t4 t5 t6 t7 R8 0000fd00 85fdef60 00000000 00004000 00000000 10003264 00000000 10006f= 58 s0 s1 s2 s3 s4 s5 s6 s7 R16 001ff000 00000000 00017051 00000000 2ab39330 75801000 00000000 2ab398= 50 t8 t9 k0 k1 gp sp s8 ra R24 00000000 2ae95c20 00000000 00000000 2aee94b0 759ffb38 00200000 2e326c= 60 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 : lui gp,0x5 0x2ae95c24 : addiu gp,gp,14480 0x2ae95c28 : addu gp,gp,t9 0x2ae95c2c : addiu sp,sp,-40 0x2ae95c30 : sw ra,36(sp) 0x2ae95c34 : sw s0,32(sp) 0x2ae95c38 : sw gp,16(sp) 0x2ae95c3c : lw v0,56(sp) 0x2ae95c40 : sw v0,24(sp) 0x2ae95c44 : lw v0,24(sp) 0x2ae95c48 : addiu sp,sp,-32 0x2ae95c4c : sw v0,16(sp) 0x2ae95c50 : li v0,4142 0x2ae95c54 : syscall 0x2ae95c58 : addiu sp,sp,32 0x2ae95c5c : lw t9,-32352(gp) 0x2ae95c60 : beqz a3,0x2ae95c7c 0x2ae95c64 : move s0,v0 0x2ae95c68 : jalr t9 0x2ae95c6c : nop 0x2ae95c70 : lw gp,16(sp) 0x2ae95c74 : sw s0,0(v0) 0x2ae95c78 : li v0,-1 0x2ae95c7c : lw ra,36(sp) 0x2ae95c80 : lw s0,32(sp) 0x2ae95c84 : jr ra 0x2ae95c88 : addiu sp,sp,40 0x2ae95c8c : 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 =3D 0x2e326c60 On Mon, Apr 5, 2010 at 5:45 PM, Joel Brobecker wrot= e: > 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 =A0Jan Stancek =A0 >> =A0 =A0 =A0 =A0 * 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 =A0look at the GNU Coding > Standards, which is available at > http://www.gnu.org/prep/standards/standards.html). > > =A0/* Fix the $sp by looking around actual $pc. =A0Currently this handles > =A0 =A0 only uClibc syscalls, which adjust $sp before syscall itself. =A0= */ > > In particular, use of capital letter for the first word in the sentence, > use of a period at the end of the a sentence. =A0And 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. =A0But 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 s= tart_pc) > > Style: > > int > mips32_get_sp_adjustment (struct frame_info *this_frame, CORE_ADDR start_= pc) > > And the function should be declared static. > >> +{ >> + =A0CORE_ADDR pc =3D get_frame_address_in_block (this_frame); >> + =A0struct gdbarch *gdbarch =3D get_frame_arch (this_frame); >> + =A0unsigned long inst, high_word, low_word, ret; >> + =A0int reg; >> + >> + =A0inst =3D (unsigned long) mips_fetch_instruction (gdbarch, pc); >> + >> + =A0ret =3D 0; >> + =A0high_word =3D (inst >> 16) & 0xffff; >> + =A0low_word =3D inst & 0xffff; >> + =A0reg =3D high_word & 0x1f; >> + >> + =A0if (high_word =3D=3D 0x27bd =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* addiu $s= p,$sp,-i */ >> + =A0 =A0 =A0|| high_word =3D=3D 0x23bd =A0 =A0 =A0 =A0 =A0 =A0/* addi $= sp,$sp,-i */ >> + =A0 =A0 =A0|| high_word =3D=3D 0x67bd) =A0 =A0 =A0 =A0 =A0 /* daddiu $= sp,$sp,-i */ >> + =A0 =A0{ >> + =A0 =A0 =A0if ( reg =3D=3D MIPS_SP_REGNUM > =A0 =A0 =A0 =A0 =A0 ^^^^ > =A0 =A0 =A0 =A0 =A0 Style: No space after '(' > >> + =A0 =A0 =A0 =A0 =A0&& (low_word & 0x8000) =3D=3D 0 =A0 /* positive sta= ck adjustment */ >> + =A0 =A0 =A0 =A0 =A0&& (pc-4) > start_pc ) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^^^^^^^^ =A0 =A0 =A0 =A0 ^^^ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| =A0 =A0 =A0 =A0 =A0 =A0 Style: No s= pace before ')' > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| > =A0 =A0 =A0 =A0 =A0 =A0Style: remove useless parens, and add space around= '-'. > >> + =A0 =A0 =A0 =A0{ >> + =A0 =A0 =A0 =A0 =A0pc =3D pc - 4; >> + =A0 =A0 =A0 =A0 =A0inst =3D (unsigned long) mips_fetch_instruction (gd= barch, pc); >> + =A0 =A0 =A0 =A0 =A0if (inst=3D=3D0x0000000c) =A0 =A0 =A0 =A0 /* syscal= l */ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^^^^ Style: spaces around all bina= ry operators > >> + =A0 =A0 =A0 =A0 =A0 =A0{ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D low_word; >> + =A0 =A0 =A0 =A0 =A0 =A0} > > Style: Remove useless curly braces for if block. > > >> @@ -1920,9 +1958,12 @@ mips32_scan_prologue (struct gdbarch *gd >> =A0 =A0/* Can be called when there's no process, and hence when there's = no >> =A0 =A0 =A0 THIS_FRAME. =A0*/ >> =A0 =A0if (this_frame !=3D NULL) >> - =A0 =A0sp =3D get_frame_register_signed (this_frame, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gd= barch_num_regs (gdbarch) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 + = MIPS_SP_REGNUM); >> + =A0 =A0{ >> + =A0 =A0 =A0sp =3D get_frame_register_signed (this_frame, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0gdbarch_num_regs (gdbarch) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0+ MIPS_SP_REGNUM); >> + =A0 =A0 =A0sp +=3D mips32_get_sp_adjustment(this_frame, start_pc); >> + =A0 =A0} >> =A0 =A0else >> =A0 =A0 =A0sp =3D 0; > > The formatting for the call to get_frame_register_signed is incorrect. > > -- > Joel >