From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27804 invoked by alias); 3 May 2012 21:09:30 -0000 Received: (qmail 27786 invoked by uid 22791); 3 May 2012 21:09:28 -0000 X-SWARE-Spam-Status: No, hits=-4.2 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; Thu, 03 May 2012 21:09:14 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1SQ3H0-0006Bs-2E from Maciej_Rozycki@mentor.com ; Thu, 03 May 2012 14:09:14 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 3 May 2012 14:09:07 -0700 Received: from [172.30.14.29] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Thu, 3 May 2012 22:09:12 +0100 Date: Thu, 03 May 2012 21:09:00 -0000 From: "Maciej W. Rozycki" To: Joel Brobecker CC: Subject: Re: [RFA 1/2] mips: Switch inferior function calls to ON_STACK method. In-Reply-To: <1336071802-13599-2-git-send-email-brobecker@adacore.com> Message-ID: References: <1336071802-13599-1-git-send-email-brobecker@adacore.com> <1336071802-13599-2-git-send-email-brobecker@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/msg00104.txt.bz2 Joel, > This patch switches the mips code to use the ON_STACK method > for function calls instead of AT_SYMBOL, which we want to remove. Thanks for this work -- can you give me a reference to some background information as to why exactly we want to remove the AT_SYMBOL method? > Another little detail on the implementation of mips_push_dummy_code. > It starts by aligning the stack. AFAIK, the stack is supposed to > always be aligned to at least 4 bytes (4 bytes for mips32, 8 bytes > for mips64). So, the initial alignment shouldn't be necessary, since > that's good enough aligment for our breakpoint instruction. But > in the end, I chose to keep it, JIC. We could possibly change the > code to align to 4 instead of 16 like mips_frame_align does, if > we want to. For the record: the respective ABIs mandate that the stack is aligned to 8 bytes for 32-bit targets and to 16 bytes for 64-bit targets. However the user may have fiddled with SP, so I think it's better to stay safe and therefore I agree it's better if we prealign the stack and avoid crashing the debuggee in this context. > gdb/ChangeLog: > > * mips-tdep.c (mips_push_dummy_code): New function. > (mips_gdbarch_init): Set the gdbarch call_dummy_location to > ON_STACK and install mips_push_dummy_code as our gdbarch > push_dummy_code routine. > > Tested on mips-irix. It might be nice to test on other mips targets > as well, although it should not be necessary. OK to commit? There's one fundamental problem with this change -- software breakpoints must not be used unconditionally, because some configurations do not expect or support them. This affects remote JTAG targets and simulators, appropriate `Z0' packets should be used -- this is because JTAG uses SDBBP instructions rather than BREAK instructions -- the latters are ordinary instructions as far as JTAG debugging is considered and do not trap into the debug mode (this is so that you can debug an OS that uses BREAK instructions for user debugging); simulators may use yet other means, typically they don't use memory breakpoints at all (again, to allow users to debug their software within the simulator, e.g. the simulator runs Linux and the user debugs some userland app within that instance of Linux). > diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c > index 9a3c7fb..3e6b00b 100644 > --- a/gdb/mips-tdep.c > +++ b/gdb/mips-tdep.c > @@ -3009,6 +3009,36 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr) > return align_down (addr, 16); > } > > +/* Implement the push_dummy_code gdbarch method for mips targets. */ > + > +static CORE_ADDR > +mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, > + CORE_ADDR funaddr, struct value **args, > + int nargs, struct type *value_type, > + CORE_ADDR *real_pc, CORE_ADDR *bp_addr, > + struct regcache *regcache) > +{ > + int bp_len; > + gdb_byte null_insn[4] = {0}; > + > + *bp_addr = mips_frame_align (gdbarch, sp); > + gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len); > + > + /* The breakpoint layer automatically adjusts the address of > + breakpoints inserted in a branch delay slot. With enough > + bad luck, the 4 bytes located just before our breakpoint > + instruction could look like a branch instruction, and thus > + trigger the adjustement, and break the function call entirely. > + So, we reserve those 4 bytes and write a null instruction > + to prevent that from happening. */ > + write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn)); > + sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len); > + > + /* Inferior resumes at the function entry point. */ > + *real_pc = funaddr; > + > + return sp; > +} > static CORE_ADDR > mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > struct regcache *regcache, CORE_ADDR bp_addr, So essentially gdbarch_breakpoint_from_pc may only be used on targets known to use software breakpoints. Additionally once microMIPS support is in the above must execute as microMIPS code where appropriate to support processors that do not have the standard MIPS mode implemented -- no such processor currently exists, but the architecture spec explicitly permits pure-microMIPS implementations and some are expected to happen sooner or later. IOW the callee must return to an address that has the ISA bit set (that'll be the address of the breakpoint above ORed with 1) or otherwise SIGBUS may be thrown (hardware signals an Address Error exception if the ISA bit is clear). Is that going to work with an update to the piece above like: [...] if (APPROPRIATE) sp = make_compact_addr (sp); return sp; } (plus a corresponding update to the breakpoint address) or is more surgery required, especially to any generic code? I guess not as I gather the return value will not only be used as the return address, but as the stack pointer as well, right? For simplicity I think: #define APPROPRIATE is_micromips_addr (gdbarch, funaddr) will do. Finally, I'd be happier if the unpredictability of the microMIPS breakpoint kind selection was avoided (there are two software and JTAG breakpoints each, BREAK16/BREAK32 and SDBBP16/SDBBP32, used by GDB like-for-like with 16-bit and 32-bit microMIPS instructions being replaced), so I suggest that the stack frame is preallocated and preinitialised along the lines of: { gdb_byte *null_insn; CORE_ADDR new_sp; sp = mips_frame_align (gdbarch, sp); new_sp = mips_frame_align (gdbarch, sp - 2 * MIPS_INSN32_SIZE); null_insn = alloca (sp - new_sp); memset (null_insn, 0, sp - new_sp); write_memory (new_sp, null_insn, sp - new_sp); [...] -- that applies to remote targets that use `Z0' too as the expected breakpoint length will be encoded by GDB as appropriate in the request. I can address all these microMIPS issues if you like as I'm likely more familiar with that stuff, but please find a solution to the software breakpoint problem. > @@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > > /* MIPS version of CALL_DUMMY. */ > > - /* NOTE: cagney/2003-08-05: Eventually call dummy location will be > - replaced by a command, and all targets will default to on stack > - (regardless of the stack's execute status). */ > - set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL); > + set_gdbarch_call_dummy_location (gdbarch, ON_STACK); > + set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code); > set_gdbarch_frame_align (gdbarch, mips_frame_align); > > set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p); So what if the stack pages are indeed not executable (their page entries have the XI aka Execute Inhibit bit set)? Maciej