This is the mail archive of the archer@sourceware.org mailing list for the Archer 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]

[jankratochvil-misc] ia64: Fix breakpoints memory shadow


Discussed upstream but committed already to <archer-jankratochvil-misc>.
http://sourceware.org/ml/gdb-patches/2008-10/threads.html#00678
http://sourceware.org/ml/gdb-patches/2008-11/threads.html#00001


>From e99a71f9d675fabf196bd24f526cfaf9d14babee Mon Sep 17 00:00:00 2001
From: Jan Kratochvil <jan.kratochvil@redhat.com>
Date: Tue, 28 Oct 2008 20:09:55 +0100
Subject: [PATCH] Fix automatic restoration of breakpoints memory for ia64.
 Red Hat private: https://bugzilla.redhat.com/show_bug.cgi?id=467502
 ------------------------------------------------------------------------------
 http://sourceware.org/ml/gdb-patches/2008-10/msg00678.html

Hi,

fix a corruption of ia64 `disass' with breakpoints placed in the target.

It is another part of the Joel Brobecker's ia64-specific breakpoints fix:
[commit] SIGILL in ld.so when running program from GDB on ia64-linux
http://sourceware.org/ml/gdb-patches/2008-04/msg00674.html

Currently ia64-tdep.c considers SHADOW_CONTENTS as a private memory space. But
it is in use by breakpoint_restore_shadows to undo the breakpoints for display.

Not going to speculate here if it may fix a breakpoints functionality.  It is
at least not a regression.  No regressions found (on ia64).  New testcase.

Thanks,
Jan
---
 gdb/ChangeLog.archer-jankratochvil-misc           |   15 ++++
 gdb/ia64-tdep.c                                   |   87 ++++++++++++++++-----
 gdb/testsuite/ChangeLog.archer-jankratochvil-misc |    3 +
 gdb/testsuite/gdb.base/breakpoint-shadow.c        |   27 +++++++
 gdb/testsuite/gdb.base/breakpoint-shadow.exp      |   64 +++++++++++++++
 5 files changed, 176 insertions(+), 20 deletions(-)
 create mode 100644 gdb/ChangeLog.archer-jankratochvil-misc
 create mode 100644 gdb/testsuite/ChangeLog.archer-jankratochvil-misc
 create mode 100644 gdb/testsuite/gdb.base/breakpoint-shadow.c
 create mode 100644 gdb/testsuite/gdb.base/breakpoint-shadow.exp

diff --git a/gdb/ChangeLog.archer-jankratochvil-misc b/gdb/ChangeLog.archer-jankratochvil-misc
new file mode 100644
index 0000000..926928f
--- /dev/null
+++ b/gdb/ChangeLog.archer-jankratochvil-misc
@@ -0,0 +1,15 @@
+2008-10-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Fix automatic restoration of breakpoints memory for ia64.
+	* ia64-tdep.c (ia64_memory_insert_breakpoint): New comment part for
+	SHADOW_CONTENTS content.  Remova variable instr.  New variable cleanup.
+	Force automatic breakpoints restoration.  PLACED_SIZE and SHADOW_LEN
+	are now set larger, to BUNDLE_LEN - 2.
+	(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.
+	(ia64_breakpoint_from_pc): Array breakpoint extended to BUNDLE_LEN.
+	Sanity check the PCPTR parameter SLOTNUM value.  New #if check on
+	BREAKPOINT_MAX vs. BUNDLE_LEN.  Increase LENPTR to BUNDLE_LEN - 2.
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 2f2e261..7d52fd7 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -545,7 +545,21 @@ fetch_instruction (CORE_ADDR addr, instruction_type *it, long long *instr)
    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 ones 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 0..2 which gets
+   incorrectly interpreted by the generic GDB code as the byte offset of
+   SHADOW_CONTENTS.  We store whole BUNDLE_LEN bytes just without these two
+   possibly skipped bytes.  */
 
 #if 0
 #define IA64_BREAKPOINT 0x00002000040LL
@@ -559,15 +573,21 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
   CORE_ADDR addr = bp_tgt->placed_address;
   char bundle[BUNDLE_LEN];
   int slotnum = (int) (addr & 0x0f) / SLOT_MULTIPLIER;
-  long long instr;
   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.  Otherwise, we could store into
+     SHADOW_CONTENTS an already stored breakpoint at the same location.
+     In practice it is already being prevented by the DUPLICATE field and
+     update_global_location_list.  */
+  cleanup = make_show_memory_breakpoints_cleanup (0);
   val = target_read_memory (addr, bundle, BUNDLE_LEN);
 
   /* Check for L type instruction in 2nd slot, if present then
@@ -578,13 +598,18 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
       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);
+
   replace_slotN_contents (bundle, IA64_BREAKPOINT, slotnum);
   if (val == 0)
-    target_write_memory (addr, bundle, BUNDLE_LEN);
+    target_write_memory (addr + slotnum, bundle + slotnum, bp_tgt->shadow_len);
 
+  do_cleanups (cleanup);
   return val;
 }
 
@@ -593,9 +618,9 @@ ia64_memory_remove_breakpoint (struct gdbarch *gdbarch,
 			       struct bp_target_info *bp_tgt)
 {
   CORE_ADDR addr = bp_tgt->placed_address;
-  char bundle[BUNDLE_LEN];
+  char 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;
@@ -604,23 +629,39 @@ ia64_memory_remove_breakpoint (struct gdbarch *gdbarch,
 
   /* 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);
+  template = extract_bit_field (bundle_mem, 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);
+  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 (_("Breakpoint removal cannot find the placed breakpoint at %s"),
+             paddr_nz (bp_tgt->placed_address));
+
+  /* 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);
+    target_write_memory (addr, bundle_mem, BUNDLE_LEN);
 
   do_cleanups (cleanup);
   return val;
@@ -631,12 +672,18 @@ ia64_memory_remove_breakpoint (struct gdbarch *gdbarch,
 const unsigned char *
 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;
+  static unsigned char breakpoint[BUNDLE_LEN];
+  int slotnum = (int) (*pcptr & 0x0f) / SLOT_MULTIPLIER;
+
+  if (slotnum > 2)
+    error (_("Can't insert breakpoint for slot numbers greater than 2."));
+
+#if BREAKPOINT_MAX < BUNDLE_LEN
+# error "BREAKPOINT_MAX < BUNDLE_LEN"
 #endif
+
+  *lenptr = BUNDLE_LEN - 2;
+
   return breakpoint;
 }
 
diff --git a/gdb/testsuite/ChangeLog.archer-jankratochvil-misc b/gdb/testsuite/ChangeLog.archer-jankratochvil-misc
new file mode 100644
index 0000000..06e4a60
--- /dev/null
+++ b/gdb/testsuite/ChangeLog.archer-jankratochvil-misc
@@ -0,0 +1,3 @@
+2008-10-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/breakpoint-shadow.exp, gdb.base/breakpoint-shadow.c: New.
diff --git a/gdb/testsuite/gdb.base/breakpoint-shadow.c b/gdb/testsuite/gdb.base/breakpoint-shadow.c
new file mode 100644
index 0000000..81ffc76
--- /dev/null
+++ b/gdb/testsuite/gdb.base/breakpoint-shadow.c
@@ -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;
+}
diff --git a/gdb/testsuite/gdb.base/breakpoint-shadow.exp b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
new file mode 100644
index 0000000..6aa62c1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/breakpoint-shadow.exp
@@ -0,0 +1,64 @@
+# 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 { [gdb_start_cmd] < 0 } {
+    untested start
+    return -1
+}
+gdb_test "" "main \\(\\) at .*" "start"
+
+# 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
+	}
+    }
+}
-- 
1.6.0.3


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