From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2323 invoked by alias); 24 May 2007 18:31:37 -0000 Received: (qmail 2309 invoked by uid 22791); 24 May 2007 18:31:35 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 24 May 2007 18:31:33 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 1EF2A982D0; Thu, 24 May 2007 18:31:31 +0000 (GMT) Received: from caradoc.them.org (dsl093-172-095.pit1.dsl.speakeasy.net [66.93.172.95]) by nan.false.org (Postfix) with ESMTP id A0F3D982CF; Thu, 24 May 2007 18:31:30 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.67) (envelope-from ) id 1HrI5x-0002mU-HK; Thu, 24 May 2007 14:31:29 -0400 Date: Thu, 24 May 2007 18:31:00 -0000 From: Daniel Jacobowitz To: "Maciej W. Rozycki" Cc: gdb-patches@sourceware.org, Chris Dearman , "Maciej W. Rozycki" Subject: Re: mips-tdep.c: Adjust breakpoints in branch delay slots Message-ID: <20070524183129.GA10094@caradoc.them.org> Mail-Followup-To: "Maciej W. Rozycki" , gdb-patches@sourceware.org, Chris Dearman , "Maciej W. Rozycki" References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.15 (2007-04-09) X-IsSubscribed: yes 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: 2007-05/txt/msg00378.txt.bz2 On Thu, May 24, 2007 at 07:07:46PM +0100, Maciej W. Rozycki wrote: > +/* Return non-zero if the ADDR instruction has a branch delay slot > + (i.e. it is a jump or branch instruction). This function was based > + on mips32_next_pc(). */ > + > +static int > +mips32_instruction_has_delay_slot (CORE_ADDR addr) > +{ > + gdb_byte buf[MIPS_INSN32_SIZE]; > + int status; > + > + status = read_memory_nobpt (addr, buf, MIPS_INSN32_SIZE); > + return !status && mips32_next_pc (addr) != addr + 4; > +} That's not what you want. mips32_next_pc calls read_register to figure out if the branch condition is true, and we don't have any idea yet (and might not be connected to a target). > + /* If a breakpoint is set on the instruction in a branch delay slot, > + GDB gets confused. When the breakpoint is hit, the PC isn't on > + the instruction in the branch delay slot, the PC will point to > + the branch instruction. Since the PC doesn't match any known > + breakpoints, GDB reports a trap exception. Just checking, but does this happen in a userspace environment like Linux too? > + There are two possible fixes for this problem. > + > + 1) When the breakpoint gets hit, see if the BD bit is set in the > + Cause register (which indicates the last exception occurred in a > + branch delay slot). If the BD bit is set, fix the PC to point to > + the instruction in the branch delay slot. > + > + 2) When the user sets the breakpoint, don't allow him to set the > + breakpoint on the instruction in the branch delay slot. Instead > + move the breakpoint to the branch instruction (which will have > + the same result). > + > + The problem with the first solution is that if the user then > + single-steps the processor, the branch instruction will get > + skipped (since GDB thinks the PC is on the instruction in the > + branch delay slot). > + > + So, we'll use the second solution. To do this we need to know if > + the instruction we're trying to set the breakpoint on is in the > + branch delay slot. */ Abstractly, which of these solutions would you say was better? I bet we could make the first one work if we tried hard enough. One way would be to fake the PC, but that's pretty gross. Still, it might be possible if it was worthwhile. BTW, see mips_single_step_through_delay, which could probably be improved. Doesn't its comment about MIPS16 having no delay slots disagree with this patch? Which is right? > + /* Make sure we don't scan back before the beginning of the > + current function, since we may fetch constant data or MIPS32 > + insns that looks like a MIPS16 jump. Of course we might do > + that anyway if the compiler has moved constants inline. :-( */ This can happen for MIPS32 too at the start of a function. All in all I'm a little bit nervous about this adjustment, since it will fail very confusingly if you get unlucky with constant pool data. That's why I was asking about the alternative solution above. Oh, and I wholeheartedly agree with Mark about testcases. It shouldn't be hard to add this to gdb.arch/. Thanks for the patch! -- Daniel Jacobowitz CodeSourcery