This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] MIPS: Avoid breakpoints in branch delay slots


Hi Joel,

> > 2011-11-22  Chris Dearman  <chris@mips.com>
> >             Nathan Froyd  <froydnj@codesourcery.com>
> >             Maciej W. Rozycki  <macro@codesourcery.com>
> > 
> > 	gdb/
> > 	* mips-tdep.c (mips32_instruction_has_delay_slot): New function.
> > 	(mips16_instruction_has_delay_slot): Likewise.
> > 	(mips_segment_boundary): Likewise.
> > 	(mips_adjust_breakpoint_address): Likewise.
> > 	(mips_gdbarch_init): Use mips_adjust_breakpoint_address.
> 
> Just a few minor comments.

 Always welcome! :)

> > +    return (op >> 2 == 5)	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
> > +	   || (op == 29)	/* JALX: bits 011101 */
> > +	   || (op == 17 && itype_rs (inst) == 8);
> > +				/* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */
> 
> In this case, can you add a pair of parentheses around the return
> expression.  I know that are useless, but it is part of our coding
> standard, because they help formatting tools format the expression
> correctly.

 Yes, I know, sorry about this.  These are pretty damn long-lived patches, 
comprising a mixture of very old code and some newer updates, and while I 
did try to catch all such nits while looking through them before posting, 
I must have obviously missed some places.  It's easier to write code 
following the standard from start than to chase such discrepancies later 
on, sigh...

>  Thus:
> 
>     return ((op >> 2 == 5)       /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
>             || (op == 29)        /* JALX: bits 011101 */
>             || (op == 17 && itype_rs (inst) == 8));
>                                  /* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */
> > +	  return (op == 8)	/* JR */
> > +		 || (op == 9);	/* JALR */
> 
> Same here, please.

 I'll look through all the pieces again and see if I haven't missed 
anything.  In this case I removed some redundant brackets too.

> > +static int
> > +mips16_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr,
> > +				   int mustbe32)
> 
> This function needs an introductory comment. In particular, mustbe32...

 Sure.

> > +static CORE_ADDR
> > +mips_segment_boundary (CORE_ADDR bpaddr)
> > +{
> > +#ifdef BFD64
> 
> Function needs an introductory comment. All functions do, in fact, so
> can you fix them all?

 Yes, I'll look through for any missing too.

> Why are the codes different if BFD64 is defined or not? That doesn't
> look right to me. Can you explain? Isn't there another dynamic property
> in the target that we can use for that?

 Well, 64-bit immediates may be truncated, unless we have 64-bit BFD 
support, due to the lack of a 64-bit integer data type I believe.  Please 
straighten me out if I am wrong though.

 Perhaps we could get away by using "sizeof (bpaddr)" instead, but I'm not 
sure if the compiler is not going to bail out on the truncation of 
out-of-range literals even if they appear in pieces of code that are 
effectively "if (0)".  Let me cook some code like this and we shall see.

 Perhaps we should actually think of phasing out pre-C9x compilers that 
lack the long long type (or any 64-bit integer equivalent) on 32-bit 
platforms.  That would help elsewhere too; in particular what I have in 
mind are some limitations in libopcodes that we recently hit with the 
addition of microMIPS instruction set support.

> > +      if (mips32_instruction_has_delay_slot (gdbarch, prev_addr))
> > +	{
> > +	  bpaddr = prev_addr;
> > +	}
> 
> Extraneous curly braces. In GDB, for one-statement if blocks, we
> prefer:
> 
>         if (mips32_instruction_has_delay_slot (gdbarch, prev_addr))
>           bpaddr = prev_addr;
> 
> Same elsewhere below. Can you fix them all?

 Yes, of course.  I have removed unnecessary curly braces around a switch 
statement earlier on too.

> > +      /* The only MIPS16 instructions with delay slots are jal, jalr
> > +         and jr.  An absolute jal is always 4 bytes long, so try for
> > +         that first, then try the 2 byte jalr/jal.
> > +         XXX We have to assume that bpaddr is not the second half of
> > +         an extended instruction.  */
> 
> I don't think there is a rule against that, but let's not use XXX.

 OK.

> Please use FIXME (I personally also like to put my name and date,
> to have it there, but optional). However, before letting a FIXME in,
> I'd like to understand what the extent of the problem is, and why
> it's not possible to fix it now.

 Oh, actually that comment is not accurate either -- the JALX instruction 
also has a delay slot, so I'll mention it here too 
(mips16_instruction_has_delay_slot handles it correctly already).  The 
comment is by Chris and from 2004-09-27 it would seem, umm...

 As to bpaddr pointing to the second half of an extended instruction -- 
if you specify breakpoint by address, e.g.:

(gdb) break *0xdeadbeef

then I guess there is no way to determine if that address points to the 
beginning or the middle of an instruction (and of course, if software 
breakpoints are used, then executing this patched instruction will make 
the program go astray rather than hitting the breakpoint).  So I'm not 
quite sure if that's a FIXME or something we need to live with.

 Long-term it might be a good idea to investigate if proper support for 
breakpoints in delay slots can be implemented instead.  The problem is not 
all debug stubs provide the necessary information -- a breakpoint in a 
delay slot can be hit as a result of falling through from the branch (in 
which case the branch has to be reexecuted or emulated upon resumption) or 
after a jump into the delay slot from elsewhere (in which case no special 
treatment must be applied).

 The two cases can be told apart by examining status, either the BD bit of 
the CP0 Cause register in OS debugging, or the DBD bit of the CP0 Debug 
register in JTAG debugging, or in a target-specific manner in the case of 
a simulator (some simulators do not maintain/expose full CP0 state at all) 
-- and even if that's available within the target itself, the remote debug 
stub or the OS may not expose the information to the debugger.

 Anyway, I believe I have addressed all your concerns now.

 I have rewritten mips_segment_boundary to avoid long long literals too -- 
at least x86 GCC version I use seems to produce reasonable code after the 
change, expanding all the shifts to inline masks as with original code. 

 I'm still feeling a bit uneasy about the double cast used to sign-extend 
bpaddr from bit #31, but I was unable to find another way of coding it 
that GCC would compile to decent code.  At least I replaced int with 
explicit int32_t.

 So far so good, but then I actually went as far as to force 32-bit BFD to 
check all is right -- it's always 64-bit with the configurations I use, 
but there are some MIPS targets that are not necessarily such.  That 
confirmed my concerns with a:

mips-tdep.c:5503: error: right shift count >= width of type

warning that -Werror converted to fatal.

 I agree that #ifdefs scattered throughout code are ugly and best avoided, 
so I have rewritten this piece so as to derive the shift count from the 
width of the type.  The result may not be the prettiest piece of code I 
have ever seen, but there you go.  As I say, it may be more reasonable to 
consider deprecating pre-C9x compilers, or at least those that do not 
provide a "long long" alternative.

 Here's the result.  Any further comments?

  Maciej

2011-12-07  Chris Dearman  <chris@mips.com>
            Nathan Froyd  <froydnj@codesourcery.com>
            Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* mips-tdep.c (mips32_instruction_has_delay_slot): New function.
	(mips16_instruction_has_delay_slot): Likewise.
	(mips_segment_boundary): Likewise.
	(mips_adjust_breakpoint_address): Likewise.
	(mips_gdbarch_init): Use mips_adjust_breakpoint_address.

gdb-mips-breakpoint-adjust.diff
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2011-12-07 00:24:01.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2011-12-07 18:23:48.725620992 +0000
@@ -5353,6 +5353,234 @@ mips_breakpoint_from_pc (struct gdbarch 
     }
 }
 
+/* Return non-zero if the ADDR instruction has a branch delay slot
+   (i.e. it is a jump or branch instruction).  This function is based
+   on mips32_next_pc.  */
+
+static int
+mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  gdb_byte buf[MIPS_INSN32_SIZE];
+  unsigned long inst;
+  int status;
+  int op;
+
+  status = target_read_memory (addr, buf, MIPS_INSN32_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (gdbarch, addr);
+  op = itype_op (inst);
+  if ((inst & 0xe0000000) != 0)
+    return (op >> 2 == 5	/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx  */
+	    || op == 29		/* JALX: bits 011101  */
+	    || (op == 17 && itype_rs (inst) == 8));
+				/* BC1F, BC1FL, BC1T, BC1TL: 010001 01000  */
+  else
+    switch (op & 0x07)		/* extract bits 28,27,26  */
+      {
+      case 0:			/* SPECIAL  */
+	op = rtype_funct (inst);
+	return (op == 8		/* JR  */
+		|| op == 9);	/* JALR  */
+	break;			/* end SPECIAL  */
+      case 1:			/* REGIMM  */
+	op = itype_rt (inst);	/* branch condition  */
+	return (op & 0xc) == 0;
+				/* BLTZ, BLTZL, BGEZ, BGEZL: bits 000xx  */
+				/* BLTZAL, BLTZALL, BGEZAL, BGEZALL: 100xx  */
+	break;			/* end REGIMM  */
+      default:			/* J, JAL, BEQ, BNE, BLEZ, BGTZ  */
+	return 1;
+	break;
+      }
+}
+
+/* Return non-zero if the ADDR instruction, which must be a 32-bit
+   instruction if MUSTBE32 is set or can be any instruction otherwise,
+   has a branch delay slot (i.e. it is a non-compact jump instruction).  */
+
+static int
+mips16_instruction_has_delay_slot (struct gdbarch *gdbarch, CORE_ADDR addr,
+				   int mustbe32)
+{
+  gdb_byte buf[MIPS_INSN16_SIZE];
+  unsigned short inst;
+  int status;
+
+  status = target_read_memory (addr, buf, MIPS_INSN16_SIZE);
+  if (status)
+    return 0;
+
+  inst = mips_fetch_instruction (gdbarch, addr);
+  if (!mustbe32)
+    return (inst & 0xf89f) == 0xe800;	/* JR/JALR (16-bit instruction)  */
+  return (inst & 0xf800) == 0x1800;	/* JAL/JALX (32-bit instruction)  */
+}
+
+/* Calculate the starting address of the MIPS memory segment BPADDR is in.
+   This assumes KSSEG exists.  */
+
+static CORE_ADDR
+mips_segment_boundary (CORE_ADDR bpaddr)
+{
+  CORE_ADDR mask = CORE_ADDR_MAX;
+  int segsize;
+
+  if (sizeof (CORE_ADDR) == 8)
+    /* Get the topmost two bits of bpaddr in a 32-bit safe manner.  */
+    switch (bpaddr >> ((sizeof (CORE_ADDR) << 3) - 2) & 3)
+      {
+      case 3:
+	if (bpaddr == (bfd_signed_vma) (int32_t) bpaddr)
+	  segsize = 29;			/* 32-bit compatibility segment  */
+	else
+	  segsize = 62;			/* xkseg  */
+	break;
+      case 2:				/* xkphys  */
+	segsize = 59;
+	break;
+      default:				/* xksseg (1), xkuseg/kuseg (0)  */
+	segsize = 62;
+	break;
+      }
+  else if (bpaddr & 0x80000000)		/* kernel segment  */
+    segsize = 29;
+  else
+    segsize = 31;			/* user segment  */
+  mask <<= segsize;
+  return bpaddr & mask;
+}
+
+/* Move the breakpoint at BPADDR out of any branch delay slot by shifting
+   it backwards if necessary.  Return the address of the new location.  */
+
+static CORE_ADDR
+mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
+{
+  CORE_ADDR prev_addr, next_addr;
+  CORE_ADDR boundary;
+  CORE_ADDR func_addr;
+
+  /* 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.
+
+     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.  */
+
+  boundary = mips_segment_boundary (bpaddr);
+
+  /* Make sure we don't scan back before the beginning of the current
+     function, since we may fetch constant data or insns that look like
+     a jump.  Of course we might do that anyway if the compiler has
+     moved constants inline. :-(  */
+  if (find_pc_partial_function (bpaddr, NULL, &func_addr, NULL)
+      && func_addr > boundary && func_addr <= bpaddr)
+    boundary = func_addr;
+
+  if (!mips_pc_is_mips16 (bpaddr))
+    {
+      if (bpaddr == boundary)
+	return bpaddr;
+
+      /* If the previous instruction has a branch delay slot, we have
+         to move the breakpoint to the branch instruction. */
+      prev_addr = bpaddr - 4;
+      if (mips32_instruction_has_delay_slot (gdbarch, prev_addr))
+	bpaddr = prev_addr;
+    }
+  else
+    {
+      struct minimal_symbol *sym;
+      CORE_ADDR addr, jmpaddr;
+      int i;
+
+      boundary = unmake_mips16_addr (boundary);
+
+      /* The only MIPS16 instructions with delay slots are JAL, JALX,
+         JALR and JR.  An absolute JAL/JALX is always 4 bytes long,
+         so try for that first, then try the 2 byte JALR/JR.
+         FIXME: We have to assume that bpaddr is not the second half
+         of an extended instruction.  */
+
+      jmpaddr = 0;
+      addr = bpaddr;
+      for (i = 1; i < 4; i++)
+	{
+	  if (unmake_mips16_addr (addr) == boundary)
+	    break;
+	  addr -= 2;
+	  if (i == 1 && mips16_instruction_has_delay_slot (gdbarch, addr, 0))
+	    /* Looks like a JR/JALR at [target-1], but it could be
+	       the second word of a previous JAL/JALX, so record it
+	       and check back one more.  */
+	    jmpaddr = addr;
+	  else if (i > 1
+		   && mips16_instruction_has_delay_slot (gdbarch, addr, 1))
+	    {
+	      if (i == 2)
+		/* Looks like a JAL/JALX at [target-2], but it could also
+		   be the second word of a previous JAL/JALX, record it,
+		   and check back one more.  */
+		jmpaddr = addr;
+	      else
+		/* Looks like a JAL/JALX at [target-3], so any previously
+		   recorded JAL/JALX or JR/JALR must be wrong, because:
+
+		   >-3: JAL
+		    -2: JAL-ext (can't be JAL/JALX)
+		    -1: bdslot (can't be JR/JALR)
+		     0: target insn
+
+		   Of course it could be another JAL-ext which looks
+		   like a JAL, but in that case we'd have broken out
+		   of this loop at [target-2]:
+
+		    -4: JAL
+		   >-3: JAL-ext
+		    -2: bdslot (can't be jmp)
+		    -1: JR/JALR
+		     0: target insn  */
+		jmpaddr = 0;
+	    }
+	  else
+	    {
+	      /* Not a jump instruction: if we're at [target-1] this
+	         could be the second word of a JAL/JALX, so continue;
+	         otherwise we're done.  */
+	      if (i > 1)
+		break;
+	    }
+	}
+
+      if (jmpaddr)
+	bpaddr = jmpaddr;
+    }
+
+  return bpaddr;
+}
+
 /* If PC is in a mips16 call or return stub, return the address of the target
    PC, which is either the callee or the caller.  There are several
    cases which must be handled:
@@ -6339,6 +6567,8 @@ mips_gdbarch_init (struct gdbarch_info i
 
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, mips_breakpoint_from_pc);
+  set_gdbarch_adjust_breakpoint_address (gdbarch,
+					 mips_adjust_breakpoint_address);
 
   set_gdbarch_skip_prologue (gdbarch, mips_skip_prologue);
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]