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] ia64: Fix breakpoints memory shadow


On Tue, 25 Nov 2008 07:12:35 +0100, Joel Brobecker wrote:
> > Still I rather found another defect compared to mem-break.c in the ia64
> > breakpoints code.  It is another problem unrelated to the original intention
> > of this ia64 patch.
> 
> Jan, please don't take it personally, I really appreciate the way
> you are trying to make your patch better and better.  But since
> this was unrelated, I really wished that you committed the first part
> as approved, and then sent a followup patch that dealt with that part
> on its own.

As I got also this mail before the commit
On Sat, 22 Nov 2008 12:30:04 +0100, Eli Zaretskii wrote:
> However, I'm slightly worried by these parts in your code:

I considered it as a valid veto of your approval until Eli's concerns get
resolved.  I appreciate your time although I do not see I would make mistakes
in the unwritten formal approval process I try to follow.


> With the approach you took, I had to fish out the previous version of the
> patch, and then compare the two patches to see exactly what changed.

Using this VIM macro for reviewing "metadiffs":
  noremap <Esc>D :set hlsearch<cr>/^[+-][+-]\([^+-].*\\|\)$<cr>

The committed patch has these changes against the last post here:
ia64_memory_insert_breakpoint: make_show_memory_breakpoints_cleanup (0 -> 1);
 - A real Bug of my former patch (although it was not a regression).
ia64_memory_insert_breakpoint: == IA64_BREAKPOINT => internal_error
 - New sanity check, unaware it would be possible to reach it as a user.


> I propose a warning saying something like this:
> 
>    cannot remove breakpoint at address %s, no break instruction at such address.

Therefore included this warning again:
ia64_memory_remove_breakpoint: != IA64_BREAKPOINT => warning
 - Happens for: gdb.base/checkpoint.exp gdb.base/foll-fork.exp
 - IMO such sanity check is missing even in default_memory_remove_breakpoint
   and these warnings would be then present also on x86.  Not checked, though.



Thanks,
Jan
gdb/
2008-11-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix automatic restoration of breakpoints memory for ia64.
	* ia64-tdep.c: New #if check on BREAKPOINT_MAX vs. BUNDLE_LEN.  
	(ia64_memory_insert_breakpoint): New comment part for SHADOW_CONTENTS
	content.  Remove variable instr.  New variable cleanup.  Disable
	automatic breakpoints restoration.  PLACED_SIZE and SHADOW_LEN are now
	set larger, to BUNDLE_LEN - 2.  Variable `bundle' type update.  Return
	error if even just final target_write_memory has failed.
	(ia64_memory_remove_breakpoint): Rename variables bundle to bundle_mem
	and instr to instr_saved.  New variables bundle_saved and
	instr_breakpoint.  Comment new reasons why we need to disable automatic
	restoration of breakpoints.  Assert PLACED_SIZE and SHADOW_LEN.  New
	check of the original memory content.  Return error if even just final
	target_write_memory has failed.
	(ia64_breakpoint_from_pc): Implement the emulation of permanent
	breakpoints compatible with current bp_loc_is_permanent.
	(template_encoding_table): Make it `const'.
	* breakpoint.c (bp_loc_is_permanent): Support unsupported software
	breakpoints.  New variables `cleanup' and `retval'.
	* monitor.c (monitor_insert_breakpoint): Remove unused variable `bp'.

gdb/doc/
2008-11-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdbint.texinfo (Target Conditionals): Extend the
	gdbarch_breakpoint_from_pc description.

gdb/testsuite/
2008-11-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/breakpoint-shadow.exp, gdb.base/breakpoint-shadow.c: New.
	* gdb.base/start.exp: New comment about an alternative - `runto_main'.

===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.362
retrieving revision 1.363
diff -u -r1.362 -r1.363
--- src/gdb/breakpoint.c	2008/11/22 04:41:45	1.362
+++ src/gdb/breakpoint.c	2008/11/26 05:27:48	1.363
@@ -5010,19 +5010,32 @@
   CORE_ADDR addr;
   const gdb_byte *brk;
   gdb_byte *target_mem;
+  struct cleanup *cleanup;
+  int retval = 0;
 
   gdb_assert (loc != NULL);
 
   addr = loc->address;
   brk = gdbarch_breakpoint_from_pc (current_gdbarch, &addr, &len);
 
+  /* Software breakpoints unsupported?  */
+  if (brk == NULL)
+    return 0;
+
   target_mem = alloca (len);
 
+  /* Enable the automatic memory restoration from breakpoints while
+     we read the memory.  Otherwise we could say about our temporary
+     breakpoints they are permanent.  */
+  cleanup = make_show_memory_breakpoints_cleanup (0);
+
   if (target_read_memory (loc->address, target_mem, len) == 0
       && memcmp (target_mem, brk, len) == 0)
-    return 1;
+    retval = 1;
 
-  return 0;
+  do_cleanups (cleanup);
+
+  return retval;
 }
 
 
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.185
retrieving revision 1.186
diff -u -r1.185 -r1.186
--- src/gdb/ia64-tdep.c	2008/11/13 05:05:07	1.185
+++ src/gdb/ia64-tdep.c	2008/11/26 05:27:48	1.186
@@ -110,6 +110,12 @@
 
 #define BUNDLE_LEN 16
 
+/* See the saved memory layout comment for ia64_memory_insert_breakpoint.  */
+
+#if BREAKPOINT_MAX < BUNDLE_LEN - 2
+# error "BREAKPOINT_MAX < BUNDLE_LEN - 2"
+#endif
+
 static gdbarch_init_ftype ia64_gdbarch_init;
 
 static gdbarch_register_name_ftype ia64_register_name;
@@ -442,7 +448,7 @@
   replace_bit_field (bundle, instr, 5+41*slotnum, 41);
 }
 
-static enum instruction_type template_encoding_table[32][3] =
+static const enum instruction_type template_encoding_table[32][3] =
 {
   { M, I, I },				/* 00 */
   { M, I, I },				/* 01 */
@@ -545,7 +551,45 @@
    simulators.  So I changed the pattern slightly to do "break.i 0x080001"
    instead.  But that didn't work either (I later found out that this
    pattern was used by the simulator that I was using.)  So I ended up
-   using the pattern seen below. */
+   using the pattern seen below.
+
+   SHADOW_CONTENTS has byte-based addressing (PLACED_ADDRESS and SHADOW_LEN)
+   while we need bit-based addressing as the instructions length is 41 bits and
+   we must not modify/corrupt the adjacent slots in the same bundle.
+   Fortunately we may store larger memory incl. the adjacent bits with the
+   original memory content (not the possibly already stored breakpoints there).
+   We need to be careful in ia64_memory_remove_breakpoint to always restore
+   only the specific bits of this instruction ignoring any adjacent stored
+   bits.
+
+   We use the original addressing with the low nibble in the range <0..2> which
+   gets incorrectly interpreted by generic non-ia64 breakpoint_restore_shadows
+   as the direct byte offset of SHADOW_CONTENTS.  We store whole BUNDLE_LEN
+   bytes just without these two possibly skipped bytes to not to exceed to the
+   next bundle.
+
+   If we would like to store the whole bundle to SHADOW_CONTENTS we would have
+   to store already the base address (`address & ~0x0f') into PLACED_ADDRESS.
+   In such case there is no other place where to store
+   SLOTNUM (`adress & 0x0f', value in the range <0..2>).  We need to know
+   SLOTNUM in ia64_memory_remove_breakpoint.
+
+   ia64 16-byte bundle layout:
+   | 5 bits | slot 0 with 41 bits | slot 1 with 41 bits | slot 2 with 41 bits |
+   
+   The current addressing used by the code below:
+   original PC   placed_address   placed_size             required    covered
+                                  == bp_tgt->shadow_len   reqd \subset covered
+   0xABCDE0      0xABCDE0         0xE                     <0x0...0x5> <0x0..0xD>
+   0xABCDE1      0xABCDE1         0xE                     <0x5...0xA> <0x1..0xE>
+   0xABCDE2      0xABCDE2         0xE                     <0xA...0xF> <0x2..0xF>
+   
+   `objdump -d' and some other tools show a bit unjustified offsets:
+   original PC   byte where starts the instruction   objdump offset
+   0xABCDE0      0xABCDE0                            0xABCDE0
+   0xABCDE1      0xABCDE5                            0xABCDE6
+   0xABCDE2      0xABCDEA                            0xABCDEC
+   */
 
 #define IA64_BREAKPOINT 0x00003333300LL
 
@@ -554,34 +598,55 @@
 			       struct bp_target_info *bp_tgt)
 {
   CORE_ADDR addr = bp_tgt->placed_address;
-  char bundle[BUNDLE_LEN];
+  gdb_byte bundle[BUNDLE_LEN];
   int slotnum = (int) (addr & 0x0f) / SLOT_MULTIPLIER;
-  long long instr;
+  long long instr_breakpoint;
   int val;
   int template;
+  struct cleanup *cleanup;
 
   if (slotnum > 2)
     error (_("Can't insert breakpoint for slot numbers greater than 2."));
 
   addr &= ~0x0f;
 
+  /* Disable the automatic memory restoration from breakpoints while
+     we read our instruction bundle.  Otherwise, the general restoration
+     mechanism kicks in and we would possibly remove parts of the adjacent
+     placed breakpoints.  It is due to our SHADOW_CONTENTS overlapping the real
+     breakpoint instruction bits region.  */
+  cleanup = make_show_memory_breakpoints_cleanup (1);
   val = target_read_memory (addr, bundle, BUNDLE_LEN);
 
-  /* Check for L type instruction in 2nd slot, if present then
-     bump up the slot number to the 3rd slot */
+  /* Check for L type instruction in slot 1, if present then bump up the slot
+     number to the slot 2.  */
   template = extract_bit_field (bundle, 0, 5);
-  if (slotnum == 1 && template_encoding_table[template][1] == L)
-    {
-      slotnum = 2;
-    }
+  if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
+    slotnum = 2;
 
-  instr = slotN_contents (bundle, slotnum);
-  memcpy (bp_tgt->shadow_contents, &instr, sizeof (instr));
-  bp_tgt->placed_size = bp_tgt->shadow_len = sizeof (instr);
+  /* Slot number 2 may skip at most 2 bytes at the beginning.  */
+  bp_tgt->placed_size = bp_tgt->shadow_len = BUNDLE_LEN - 2;
+
+  /* Store the whole bundle, except for the initial skipped bytes by the slot
+     number interpreted as bytes offset in PLACED_ADDRESS.  */
+  memcpy (bp_tgt->shadow_contents, bundle + slotnum, bp_tgt->shadow_len);
+
+  /* Breakpoints already present in the code will get deteacted and not get
+     reinserted by bp_loc_is_permanent.  Multiple breakpoints at the same
+     location cannot induce the internal error as they are optimized into
+     a single instance by update_global_location_list.  */
+  instr_breakpoint = slotN_contents (bundle, slotnum);
+  if (instr_breakpoint == IA64_BREAKPOINT)
+    internal_error (__FILE__, __LINE__,
+		    _("Address %s already contains a breakpoint."),
+		    paddr_nz (bp_tgt->placed_address));
   replace_slotN_contents (bundle, IA64_BREAKPOINT, slotnum);
+
   if (val == 0)
-    target_write_memory (addr, bundle, BUNDLE_LEN);
+    val = target_write_memory (addr + slotnum, bundle + slotnum,
+			       bp_tgt->shadow_len);
 
+  do_cleanups (cleanup);
   return val;
 }
 
@@ -590,9 +655,9 @@
 			       struct bp_target_info *bp_tgt)
 {
   CORE_ADDR addr = bp_tgt->placed_address;
-  char bundle[BUNDLE_LEN];
+  gdb_byte bundle_mem[BUNDLE_LEN], bundle_saved[BUNDLE_LEN];
   int slotnum = (addr & 0x0f) / SLOT_MULTIPLIER;
-  long long instr;
+  long long instr_breakpoint, instr_saved;
   int val;
   int template;
   struct cleanup *cleanup;
@@ -601,40 +666,96 @@
 
   /* Disable the automatic memory restoration from breakpoints while
      we read our instruction bundle.  Otherwise, the general restoration
-     mechanism kicks in and ends up corrupting our bundle, because it
-     is not aware of the concept of instruction bundles.  */
+     mechanism kicks in and we would possibly remove parts of the adjacent
+     placed breakpoints.  It is due to our SHADOW_CONTENTS overlapping the real
+     breakpoint instruction bits region.  */
   cleanup = make_show_memory_breakpoints_cleanup (1);
-  val = target_read_memory (addr, bundle, BUNDLE_LEN);
+  val = target_read_memory (addr, bundle_mem, BUNDLE_LEN);
 
-  /* Check for L type instruction in 2nd slot, if present then
-     bump up the slot number to the 3rd slot */
-  template = extract_bit_field (bundle, 0, 5);
-  if (slotnum == 1 && template_encoding_table[template][1] == L)
-    {
-      slotnum = 2;
-    }
-
-  memcpy (&instr, bp_tgt->shadow_contents, sizeof instr);
-  replace_slotN_contents (bundle, instr, slotnum);
+  /* Check for L type instruction in slot 1, if present then bump up the slot
+     number to the slot 2.  */
+  template = extract_bit_field (bundle_mem, 0, 5);
+  if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
+    slotnum = 2;
+
+  gdb_assert (bp_tgt->placed_size == BUNDLE_LEN - 2);
+  gdb_assert (bp_tgt->placed_size == bp_tgt->shadow_len);
+
+  instr_breakpoint = slotN_contents (bundle_mem, slotnum);
+  if (instr_breakpoint != IA64_BREAKPOINT)
+    {
+      warning (_("Cannot remove breakpoint at address %s, "
+		 "no break instruction at such address."),
+	       paddr_nz (bp_tgt->placed_address));
+      return -1;
+    }
+
+  /* Extract the original saved instruction from SLOTNUM normalizing its
+     bit-shift for INSTR_SAVED.  */
+  memcpy (bundle_saved, bundle_mem, BUNDLE_LEN);
+  memcpy (bundle_saved + slotnum, bp_tgt->shadow_contents, bp_tgt->shadow_len);
+  instr_saved = slotN_contents (bundle_saved, slotnum);
+
+  /* In BUNDLE_MEM be careful to modify only the bits belonging to SLOTNUM and
+     never any other possibly also stored in SHADOW_CONTENTS.  */
+  replace_slotN_contents (bundle_mem, instr_saved, slotnum);
   if (val == 0)
-    target_write_memory (addr, bundle, BUNDLE_LEN);
+    val = target_write_memory (addr, bundle_mem, BUNDLE_LEN);
 
   do_cleanups (cleanup);
   return val;
 }
 
-/* We don't really want to use this, but remote.c needs to call it in order
-   to figure out if Z-packets are supported or not.  Oh, well. */
-const unsigned char *
+/* As gdbarch_breakpoint_from_pc ranges have byte granularity and ia64
+   instruction slots ranges are bit-granular (41 bits) we have to provide an
+   extended range as described for ia64_memory_insert_breakpoint.  We also take
+   care of preserving the `break' instruction 21-bit (or 62-bit) parameter to
+   make a match for permanent breakpoints.  */
+
+static const gdb_byte *
 ia64_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)
 {
-  static unsigned char breakpoint[] =
-    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
-  *lenptr = sizeof (breakpoint);
-#if 0
-  *pcptr &= ~0x0f;
-#endif
-  return breakpoint;
+  CORE_ADDR addr = *pcptr;
+  static gdb_byte bundle[BUNDLE_LEN];
+  int slotnum = (int) (*pcptr & 0x0f) / SLOT_MULTIPLIER;
+  long long instr_fetched;
+  int val;
+  int template;
+  struct cleanup *cleanup;
+
+  if (slotnum > 2)
+    error (_("Can't insert breakpoint for slot numbers greater than 2."));
+
+  addr &= ~0x0f;
+
+  /* Enable the automatic memory restoration from breakpoints while
+     we read our instruction bundle to match bp_loc_is_permanent.  */
+  cleanup = make_show_memory_breakpoints_cleanup (0);
+  val = target_read_memory (addr, bundle, BUNDLE_LEN);
+  do_cleanups (cleanup);
+
+  /* The memory might be unreachable.  This can happen, for instance,
+     when the user inserts a breakpoint at an invalid address.  */
+  if (val != 0)
+    return NULL;
+
+  /* Check for L type instruction in slot 1, if present then bump up the slot
+     number to the slot 2.  */
+  template = extract_bit_field (bundle, 0, 5);
+  if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
+    slotnum = 2;
+
+  /* A break instruction has its all its opcode bits cleared except for
+     the parameter value.  For L+X slot pair we are at the X slot (slot 2) so
+     we should not touch the L slot - the upper 41 bits of the parameter.  */
+  instr_fetched = slotN_contents (bundle, slotnum);
+  instr_fetched &= 0x1003ffffc0;
+  replace_slotN_contents (bundle, instr_fetched, slotnum);
+
+  *lenptr = BUNDLE_LEN - 2;
+
+  /* SLOTNUM is possibly already locally modified - use caller's *PCPTR.  */
+  return bundle + (*pcptr & 0x0f);
 }
 
 static CORE_ADDR
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.292
retrieving revision 1.293
diff -u -r1.292 -r1.293
--- src/gdb/doc/gdbint.texinfo	2008/10/27 11:37:40	1.292
+++ src/gdb/doc/gdbint.texinfo	2008/11/26 05:26:40	1.293
@@ -3430,16 +3430,23 @@
 @findex gdbarch_breakpoint_from_pc
 @anchor{gdbarch_breakpoint_from_pc} Use the program counter to determine the
 contents and size of a breakpoint instruction.  It returns a pointer to
-a string of bytes that encode a breakpoint instruction, stores the
+a static string of bytes that encode a breakpoint instruction, stores the
 length of the string to @code{*@var{lenptr}}, and adjusts the program
 counter (if necessary) to point to the actual memory location where the
-breakpoint should be inserted.
+breakpoint should be inserted.  May return @code{NULL} to indicate that
+software breakpoints are not supported.
 
 Although it is common to use a trap instruction for a breakpoint, it's
 not required; for instance, the bit pattern could be an invalid
 instruction.  The breakpoint must be no longer than the shortest
 instruction of the architecture.
 
+Provided breakpoint bytes can be also used by @code{bp_loc_is_permanent} to
+detect permanent breakpoints.  @code{gdbarch_breakpoint_from_pc} should return
+an unchanged memory copy if it was called for a location with permanent
+breakpoint as some architectures use breakpoint instructions containing
+arbitrary parameter value.
+
 Replaces all the other @var{BREAKPOINT} macros.
 
 @item int gdbarch_memory_insert_breakpoint (@var{gdbarch}, @var{bp_tgt})
/cvs/src/src/gdb/testsuite/gdb.base/breakpoint-shadow.c,v  -->  standard output
revision 1.1
--- src/gdb/testsuite/gdb.base/breakpoint-shadow.c
+++ src/gdb/testsuite/gdb.base/breakpoint-shadow.c	2008-11-26 05:29:49.128279000 +0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  volatile int i;
+  
+  i = 1;	/* break-first */
+  i = 2;	/* break-second */
+
+  return 0;
+}
/cvs/src/src/gdb/testsuite/gdb.base/breakpoint-shadow.exp,v  -->  standard output
revision 1.1
--- src/gdb/testsuite/gdb.base/breakpoint-shadow.exp
+++ src/gdb/testsuite/gdb.base/breakpoint-shadow.exp	2008-11-26 05:29:49.855090000 +0000
@@ -0,0 +1,63 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile breakpoint-shadow
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+# We need to start the inferior to place the breakpoints in the memory at all.
+if ![runto_main] {
+    untested start
+    return -1
+}
+
+# The default "auto" mode removes all the breakpoints when we stop (and not
+# running the nonstop mode).  We would not be able to test the shadow.
+gdb_test "set breakpoint always-inserted on"
+gdb_test "show breakpoint always-inserted" "Always inserted breakpoint mode is on."
+
+set match "\nDump of assembler code for function main:\r\n(.*)End of assembler dump.\r\n$gdb_prompt $"
+
+set test "disassembly without breakpoints"
+gdb_test_multiple "disass main" $test {
+    -re $match {
+    	set orig $expect_out(1,string)
+	pass $test
+    }
+}
+
+gdb_test "b [gdb_get_line_number "break-first"]" "Breakpoint \[0-9\] at .*" "First breakpoint placed"
+gdb_test "b [gdb_get_line_number "break-second"]" "Breakpoint \[0-9\] at .*" "Second breakpoint placed"
+
+set test "disassembly with breakpoints"
+gdb_test_multiple "disass main" $test {
+    -re $match {
+    	set got $expect_out(1,string)
+	if [string equal -nocase $orig $got] {
+	    pass $test
+	} else {
+	    fail $test
+	}
+    }
+}
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/start.exp,v
retrieving revision 1.5
retrieving revision 1.6
diff -u -r1.5 -r1.6
--- src/gdb/testsuite/gdb.base/start.exp	2008/01/01 22:53:19	1.5
+++ src/gdb/testsuite/gdb.base/start.exp	2008/11/26 05:25:56	1.6
@@ -35,6 +35,9 @@
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
+# This is a testcase specifically for the `start' GDB command.  For regular
+# stop-in-main goal in the testcases consider using `runto_main' instead.
+
 # For C programs, "start" should stop in main().
 if { [gdb_start_cmd] < 0 } {
     untested start
@@ -44,4 +47,3 @@
 gdb_test "" \
          "main \\(\\) at .*start.c.*" \
          "start"
-

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