From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18588 invoked by alias); 14 May 2012 09:44:39 -0000 Received: (qmail 18574 invoked by uid 22791); 14 May 2012 09:44:38 -0000 X-SWARE-Spam-Status: No, hits=-4.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_EG X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 May 2012 09:44:25 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1STrpG-00052i-JB from Maciej_Rozycki@mentor.com ; Mon, 14 May 2012 02:44:22 -0700 Received: from SVR-IES-FEM-02.mgc.mentorg.com ([137.202.0.106]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Mon, 14 May 2012 02:44:22 -0700 Received: from [172.30.0.49] (137.202.0.76) by SVR-IES-FEM-02.mgc.mentorg.com (137.202.0.106) with Microsoft SMTP Server id 14.1.289.1; Mon, 14 May 2012 10:44:19 +0100 Date: Mon, 14 May 2012 09:44:00 -0000 From: "Maciej W. Rozycki" To: Joel Brobecker CC: Mark Kettenis , Subject: Re: [RFA 1/2] mips: Switch inferior function calls to ON_STACK method. In-Reply-To: <20120509143537.GH15555@adacore.com> Message-ID: References: <201205042118.q44LIh3p018153@glazunov.sibelius.xs4all.nl> <201205051144.q45Bitv4006357@glazunov.sibelius.xs4all.nl> <20120508160542.GB15555@adacore.com> <20120508204257.GC15555@adacore.com> <20120508220805.GD15555@adacore.com> <201205090823.q498Njc7019605@glazunov.sibelius.xs4all.nl> <20120509143537.GH15555@adacore.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" 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: 2012-05/txt/msg00499.txt.bz2 On Wed, 9 May 2012, Joel Brobecker wrote: > I have now checked this patch in. Thanks. It looks like a typo has crept in in the review process and nobody noticed. I have now fixed it with the change below. Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c =================================================================== --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2012-05-12 22:50:59.375649586 +0100 +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2012-05-12 22:51:22.565626048 +0100 @@ -3009,7 +3009,7 @@ mips_frame_align (struct gdbarch *gdbarc return align_down (addr, 16); } -/* Implement the "push_dummy_call" gdbarch method. */ +/* Implement the "push_dummy_code" gdbarch method. */ static CORE_ADDR mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, > > Not too long ago, Jan Kratochvil pointed out a problem with > > AT_ENTRY_POINT. > > Yeah - I thought we had fixed that by switching the x86/amd64 targets > to using ON_STACK. But then when I grep'ed around, I didn't find that. > So I thought perhaps we went a different route, because I was sure we > had checked a fix in. But in fact, reviewing the history, I think > that this issue is still open, probably one of these low-priority > issues that one looks at when there is free moment. > > In my opinion, the point about not being able to write the breakpoint > if the program is executed from ROM, combined with the fact that entry > points may have CFI info, clinches it. I gave it yet more thinking and came to the conclusion that at least for the MIPS target, where it is safe to use either way, but both have some drawbacks, we should really apply both, switching dynamically. The reason is the stack may be unwritable for whatever reason (e.g. not correctly set up), so we should try ON_STACK first and if that fails (e.g. SP is NULL or writing to the stack has faulted), then fall back to AT_ENTRY_POINT. This is another corner case however and I don't feel compelled to implement it right now. Let's leave it for another sunny day in Cambridgeshire. ;) > OK - now that this is out of the way, which comments did we want to add? I've lost track, sorry, however here is the promised update I will include with the microMIPS change. I used a local variable to hold the address of the breakpoint slot after all lest the result be horrible. No regressions on mips-sde-elf or mips-linux-gnu, checked the usual set of standard MIPS, MIPS16 and microMIPS multilibs. Since this is recent code and I was changing it anyway I have reordered the local variables to follow the "reverse Christmas tree" style -- I find the definitions easier to read this way (of course initialisers may prevent this style to be fully applied in some cases). The style has been used in the various places of the Linux kernel recently and I believe there is nothing said about how local variable definitions are meant to be ordered in the GNU Coding Style, so it should be fine to use it in GDB (at least in places I am concerned about). I do not plan to specifically revamp existing code though. 2012-05-14 Maciej W. Rozycki gdb/ * mips-tdep.c (mips_push_dummy_code): Handle microMIPS code. Maciej gdb-micromips-on-stack.diff Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c =================================================================== --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2012-05-12 21:21:38.000000000 +0100 +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2012-05-12 22:45:37.355619042 +0100 @@ -4198,11 +4198,18 @@ mips_push_dummy_code (struct gdbarch *gd CORE_ADDR *real_pc, CORE_ADDR *bp_addr, struct regcache *regcache) { - CORE_ADDR nop_addr; static gdb_byte nop_insn[] = { 0, 0, 0, 0 }; + CORE_ADDR nop_addr; + CORE_ADDR bp_slot; /* Reserve enough room on the stack for our breakpoint instruction. */ - *bp_addr = sp - sizeof (nop_insn); + bp_slot = sp - sizeof (nop_insn); + + /* Return to microMIPS mode if calling microMIPS code to avoid + triggering an address error exception on processors that only + support microMIPS execution. */ + *bp_addr = (mips_pc_is_micromips (gdbarch, funaddr) + ? make_compact_addr (bp_slot) : bp_slot); /* The breakpoint layer automatically adjusts the address of breakpoints inserted in a branch delay slot. With enough @@ -4211,7 +4218,7 @@ mips_push_dummy_code (struct gdbarch *gd trigger the adjustement, and break the function call entirely. So, we reserve those 4 bytes and write a nop instruction to prevent that from happening. */ - nop_addr = *bp_addr - sizeof (nop_insn); + nop_addr = bp_slot - sizeof (nop_insn); write_memory (nop_addr, nop_insn, sizeof (nop_insn)); sp = mips_frame_align (gdbarch, nop_addr);