[patch] [ia64] Fix (#2) shadowing of breakpoints

[patch] [ia64] Fix (#2) shadowing of breakpoints

[Jan Kratochvil]

Hi,

found my previous fix was incomplete:
	[patch] ia64: Fix breakpoints memory shadow
	http://sourceware.org/ml/gdb-patches/2008-10/msg00678.html
	http://sourceware.org/ml/gdb-patches/2008-11/msg00001.html

Hiding of breakpoints in multiple slots of a single bundle could fail with
"set breakpoint always-inserted on":

original code:
0x4000000000000690 <main+16>:   [MII]       st4.rel [r2]=r14
0x4000000000000691 <main+17>:               mov r14=2;;
0x4000000000000692 <main+18>:               nop.i 0x0

displayed code:
0x4000000000000690 <main+16>:   [MII]       st4.rel [r2]=r14
0x4000000000000691 <main+17>:               break.i 0xccccc;;
0x4000000000000692 <main+18>:               nop.i 0x0

The insert breakpoints must read the memory in a both shadowed and unshadowed
way for the two different use cases there.  I think there is no guarantee of
order of restoration of shadow_contents so its content must be always fully
breakpoint-free.

Regression tested on ia64-rhel54-linux-gnu together with the next patch.
Patch has no code changes on non-ia64 arches.

A bit weird VAL error handling is going to be fixed up in the next patch.


Thanks,
Jan


gdb/
2009-09-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix ia64 shadowing of breakpoints in multiple slots of a single bundle.
	* ia64-tdep.c (ia64_memory_insert_breakpoint): New call
	of make_show_memory_breakpoints_cleanup with parameter 0.  Move the
	reading of SHADOW_CONTENTS to this memory state point of code.

gdb/testsuite/
2009-09-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/breakpoint-shadow.exp (Second breakpoint placed): Initialize
	$bpt2address.
	(Second breakpoint address is valid on ia64)
	(ia64 breakpoint in the Second breakpoint bundle): New.

--- gdb/ia64-tdep.c	25 Aug 2009 14:06:47 -0000	1.196
+++ gdb/ia64-tdep.c	5 Sep 2009 16:49:03 -0000
@@ -622,13 +622,28 @@ ia64_memory_insert_breakpoint (struct gd
 
   addr &= ~0x0f;
 
+  /* Enable the automatic memory restoration from breakpoints while
+     we read our instruction bundle for the purpose of SHADOW_CONTENTS.
+     Otherwise, we could possibly store into the shadow 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 (0);
+  val = target_read_memory (addr, bundle, BUNDLE_LEN);
+
+  /* Slot number 2 may skip at most 2 bytes at the beginning.  */
+  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);
+
   /* 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);
+  make_show_memory_breakpoints_cleanup (1);
+  val |= target_read_memory (addr, bundle, BUNDLE_LEN);
 
   /* Check for L type instruction in slot 1, if present then bump up the slot
      number to the slot 2.  */
@@ -636,13 +651,6 @@ ia64_memory_insert_breakpoint (struct gd
   if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
     slotnum = 2;
 
-  /* 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
@@ -654,6 +662,8 @@ ia64_memory_insert_breakpoint (struct gd
 		    paddress (gdbarch, bp_tgt->placed_address));
   replace_slotN_contents (bundle, IA64_BREAKPOINT, slotnum);
 
+  bp_tgt->placed_size = bp_tgt->shadow_len;
+
   if (val == 0)
     val = target_write_memory (addr + slotnum, bundle + slotnum,
 			       bp_tgt->shadow_len);
--- gdb/testsuite/gdb.base/breakpoint-shadow.exp	3 Jan 2009 05:58:03 -0000	1.2
+++ gdb/testsuite/gdb.base/breakpoint-shadow.exp	5 Sep 2009 16:49:03 -0000
@@ -49,6 +49,26 @@ gdb_test_multiple "disass main" $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 "Second breakpoint placed"
+gdb_test_multiple "b [gdb_get_line_number "break-second"]" $test {
+    -re "Breakpoint \[0-9\] at (0x\[0-9a-f\]*):.*" {
+	pass $test
+	set bpt2address $expect_out(1,string)
+    }
+}
+
+if [istarget "ia64-*-*"] then {
+    # Unoptimized code should not use the 3rd slot for the first instruction of
+    # a source line.
+    set test "Second breakpoint address is valid on ia64"
+    if [string match "*\[01\]" $bpt2address] {
+	pass $test
+
+	gdb_test "b *($bpt2address + 1)" "Breakpoint \[0-9\] at .*" "ia64 breakpoint in the Second breakpoint bundle"
+    } else {
+	unresolved $test
+    }
+}
 
 set test "disassembly with breakpoints"
 gdb_test_multiple "disass main" $test {

[patch] [ia64] Fix (#2) shadowing of breakpoints [testcase fixup]

[Jan Kratochvil]

[ Fixed testcase forgotten former-duplicate test. ]

Hi,

found my previous fix was incomplete:
	[patch] ia64: Fix breakpoints memory shadow
	http://sourceware.org/ml/gdb-patches/2008-10/msg00678.html
	http://sourceware.org/ml/gdb-patches/2008-11/msg00001.html

Hiding of breakpoints in multiple slots of a single bundle could fail with
"set breakpoint always-inserted on":

original code:
0x4000000000000690 <main+16>:   [MII]       st4.rel [r2]=r14
0x4000000000000691 <main+17>:               mov r14=2;;
0x4000000000000692 <main+18>:               nop.i 0x0

displayed code:
0x4000000000000690 <main+16>:   [MII]       st4.rel [r2]=r14
0x4000000000000691 <main+17>:               break.i 0xccccc;;
0x4000000000000692 <main+18>:               nop.i 0x0

The insert breakpoints must read the memory in a both shadowed and unshadowed
way for the two different use cases there.  I think there is no guarantee of
order of restoration of shadow_contents so its content must be always fully
breakpoint-free.

Regression tested on ia64-rhel54-linux-gnu together with the next patch.
Patch has no code changes on non-ia64 arches.

A bit weird VAL error handling is going to be fixed up in the next patch.


Thanks,
Jan


gdb/
2009-09-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix ia64 shadowing of breakpoints in multiple slots of a single bundle.
	* ia64-tdep.c (ia64_memory_insert_breakpoint): New call
	of make_show_memory_breakpoints_cleanup with parameter 0.  Move the
	reading of SHADOW_CONTENTS to this memory state point of code.

gdb/testsuite/
2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/breakpoint-shadow.exp (Second breakpoint placed): Initialize
	$bpt2address.
	(Second breakpoint address is valid on ia64)
	(ia64 breakpoint in the Second breakpoint bundle): New.

--- gdb/ia64-tdep.c
+++ gdb/ia64-tdep.c
@@ -622,13 +622,28 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
 
   addr &= ~0x0f;
 
+  /* Enable the automatic memory restoration from breakpoints while
+     we read our instruction bundle for the purpose of SHADOW_CONTENTS.
+     Otherwise, we could possibly store into the shadow 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 (0);
+  val = target_read_memory (addr, bundle, BUNDLE_LEN);
+
+  /* Slot number 2 may skip at most 2 bytes at the beginning.  */
+  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);
+
   /* 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);
+  make_show_memory_breakpoints_cleanup (1);
+  val |= target_read_memory (addr, bundle, BUNDLE_LEN);
 
   /* Check for L type instruction in slot 1, if present then bump up the slot
      number to the slot 2.  */
@@ -636,13 +651,6 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (slotnum == 1 && template_encoding_table[template][slotnum] == L)
     slotnum = 2;
 
-  /* 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
@@ -654,6 +662,8 @@ ia64_memory_insert_breakpoint (struct gdbarch *gdbarch,
 		    paddress (gdbarch, bp_tgt->placed_address));
   replace_slotN_contents (bundle, IA64_BREAKPOINT, slotnum);
 
+  bp_tgt->placed_size = bp_tgt->shadow_len;
+
   if (val == 0)
     val = target_write_memory (addr + slotnum, bundle + slotnum,
 			       bp_tgt->shadow_len);
--- gdb/testsuite/gdb.base/breakpoint-shadow.exp
+++ gdb/testsuite/gdb.base/breakpoint-shadow.exp
@@ -48,7 +48,26 @@ gdb_test_multiple "disass main" $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 "Second breakpoint placed"
+gdb_test_multiple "b [gdb_get_line_number "break-second"]" $test {
+    -re "Breakpoint \[0-9\] at (0x\[0-9a-f\]*):.*" {
+	pass $test
+	set bpt2address $expect_out(1,string)
+    }
+}
+
+if [istarget "ia64-*-*"] then {
+    # Unoptimized code should not use the 3rd slot for the first instruction of
+    # a source line.
+    set test "Second breakpoint address is valid on ia64"
+    if [string match "*\[01\]" $bpt2address] {
+	pass $test
+
+	gdb_test "b *($bpt2address + 1)" "Breakpoint \[0-9\] at .*" "ia64 breakpoint in the Second breakpoint bundle"
+    } else {
+	unresolved $test
+    }
+}
 
 set test "disassembly with breakpoints"
 gdb_test_multiple "disass main" $test {

Re: [patch] [ia64] Fix (#2) shadowing of breakpoints [testcase fixup]

[Joel Brobecker]

> gdb/
> 2009-09-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix ia64 shadowing of breakpoints in multiple slots of a single bundle.
> 	* ia64-tdep.c (ia64_memory_insert_breakpoint): New call
> 	of make_show_memory_breakpoints_cleanup with parameter 0.  Move the
> 	reading of SHADOW_CONTENTS to this memory state point of code.

Patch is approved. I had a hard time understanding what was going on
(relatively speaking), but eventually got it. Perhaps a little comment
updated as suggested below might help; your call.

> gdb/testsuite/
> 2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/breakpoint-shadow.exp (Second breakpoint placed): Initialize
> 	$bpt2address.
> 	(Second breakpoint address is valid on ia64)
> 	(ia64 breakpoint in the Second breakpoint bundle): New.

Approved as well :).

>    /* 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

This is where I think it would be worth explaining that we are *re*
reading the bundle except that, this time, we are reading it in order
to compute the new bundle inside which we'll be inserting the
breakpoint. Therefore, we have to disable the automatic memory
restoration, bla bla bla.

> +if [istarget "ia64-*-*"] then {
> +    # Unoptimized code should not use the 3rd slot for the first instruction of
> +    # a source line.

This is important for our test, because we want both breakpoints
(break-first and break-second) to be in the same bundle.

-- 
Joel

Re: [patch] [ia64] Fix (#2) shadowing of breakpoints [testcase fixup]

[Jan Kratochvil]

On Mon, 07 Sep 2009 20:17:50 +0200, Joel Brobecker wrote:
> Patch is approved.

Checked-in.


> This is where I think it would be worth explaining that we are *re*
> reading the bundle except that, this time, we are reading it in order

OK, I agree, thanks, updated.


> > +if [istarget "ia64-*-*"] then {
> > +    # Unoptimized code should not use the 3rd slot for the first instruction of
> > +    # a source line.
> 
> This is important for our test, because we want both breakpoints
> (break-first and break-second) to be in the same bundle.

In fact there are two breakpoints on non-ia64 and three breakpoints on ia64.

I do not remember much why there were two breakpoints already before, maybe
they were intended to be in the same bundle (which they are never in).
Comment on their reason is missing there (from me).

(+In real there are three/four breakpoints - the first one is from runto_main.)


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2009-09/msg00032.html

--- src/gdb/ChangeLog	2009/09/08 00:50:42	1.10848
+++ src/gdb/ChangeLog	2009/09/08 17:39:19	1.10849
@@ -1,3 +1,11 @@
+2009-09-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Fix ia64 shadowing of breakpoints in multiple slots of a single bundle.
+	* ia64-tdep.c (ia64_memory_insert_breakpoint): New call
+	of make_show_memory_breakpoints_cleanup with parameter 0.  Move the
+	reading of SHADOW_CONTENTS to this memory state point of code.  Update
+	comment for the memory re-read.
+
 2009-09-07  Michael Snyder  <msnyder@vmware.com>
 
 	* record.c: Minor comment and white space fix-ups.
--- src/gdb/ia64-tdep.c	2009/08/25 14:06:47	1.196
+++ src/gdb/ia64-tdep.c	2009/09/08 17:39:21	1.197
@@ -622,27 +622,37 @@
 
   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
+  /* Enable the automatic memory restoration from breakpoints while
+     we read our instruction bundle for the purpose of SHADOW_CONTENTS.
+     Otherwise, we could possibly store into the shadow 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);
+  cleanup = make_show_memory_breakpoints_cleanup (0);
   val = target_read_memory (addr, bundle, BUNDLE_LEN);
 
-  /* 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;
-
   /* Slot number 2 may skip at most 2 bytes at the beginning.  */
-  bp_tgt->placed_size = bp_tgt->shadow_len = BUNDLE_LEN - 2;
+  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);
 
+  /* Re-read the same bundle as above except that, this time, read it in order
+     to compute the new bundle inside which we will be inserting the
+     breakpoint.  Therefore, 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.  */
+  make_show_memory_breakpoints_cleanup (1);
+  val |= target_read_memory (addr, bundle, BUNDLE_LEN);
+
+  /* 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;
+
   /* 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
@@ -654,6 +664,8 @@
 		    paddress (gdbarch, bp_tgt->placed_address));
   replace_slotN_contents (bundle, IA64_BREAKPOINT, slotnum);
 
+  bp_tgt->placed_size = bp_tgt->shadow_len;
+
   if (val == 0)
     val = target_write_memory (addr + slotnum, bundle + slotnum,
 			       bp_tgt->shadow_len);
--- src/gdb/testsuite/ChangeLog	2009/09/03 22:03:20	1.1952
+++ src/gdb/testsuite/ChangeLog	2009/09/08 17:39:21	1.1953
@@ -1,3 +1,10 @@
+2009-09-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/breakpoint-shadow.exp (Second breakpoint placed): Initialize
+	$bpt2address.
+	(Second breakpoint address is valid on ia64)
+	(Third breakpoint on ia64 in the Second breakpoint's bundle): New.
+
 2009-09-03  Joseph Myers  <joseph@codesourcery.com>
 
 	* gdb.base/ending-run.exp: Restrict regular expression matching
--- src/gdb/testsuite/gdb.base/breakpoint-shadow.exp	2009/01/03 05:58:03	1.2
+++ src/gdb/testsuite/gdb.base/breakpoint-shadow.exp	2009/09/08 17:39:22	1.3
@@ -48,7 +48,29 @@
 }
 
 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 "Second breakpoint placed"
+gdb_test_multiple "b [gdb_get_line_number "break-second"]" $test {
+    -re "Breakpoint \[0-9\] at (0x\[0-9a-f\]*):.*" {
+	pass $test
+	set bpt2address $expect_out(1,string)
+    }
+}
+
+if [istarget "ia64-*-*"] then {
+    # Unoptimized code should not use the 3rd slot for the first instruction of
+    # a source line.  This is important for our test, because we want both
+    # breakpoints ("Second breakpoint" and the following one) to be in the same
+    # bundle.
+
+    set test "Second breakpoint address is valid on ia64"
+    if [string match "*\[01\]" $bpt2address] {
+	pass $test
+
+	gdb_test "b *($bpt2address + 1)" "Breakpoint \[0-9\] at .*" "Third breakpoint on ia64 in the Second breakpoint's bundle"
+    } else {
+	unresolved $test
+    }
+}
 
 set test "disassembly with breakpoints"
 gdb_test_multiple "disass main" $test {