From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28720 invoked by alias); 5 Apr 2010 15:45:32 -0000 Received: (qmail 28367 invoked by uid 22791); 5 Apr 2010 15:45:19 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 05 Apr 2010 15:45:12 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3F8EC2BAB51; Mon, 5 Apr 2010 11:45:11 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id GSXs6NBkCJNH; Mon, 5 Apr 2010 11:45:11 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 0036B2BAB43; Mon, 5 Apr 2010 11:45:10 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 25DF4F58C2; Mon, 5 Apr 2010 08:45:07 -0700 (PDT) Date: Mon, 05 Apr 2010 15:45:00 -0000 From: Joel Brobecker To: J?n Stan??ek Cc: gdb-patches@sourceware.org Subject: Re: patch: fix stack unwind through uClibc syscall() on mips Message-ID: <20100405154507.GC19194@adacore.com> References: <737ad3551003271055o91a78i3f5ff305b927e441@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <737ad3551003271055o91a78i3f5ff305b927e441@mail.gmail.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00074.txt.bz2 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 > * 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