[patch] [ia64] Fixup breakpoints errors handling

[patch] [ia64] Fixup breakpoints errors handling

[Jan Kratochvil]

Hi,

found out the code could already for example internal_error on uninitialized
memory after failed target_read_memory.  Do not try to continue after failed
memory read as the whole function would fail anyway.

This patch was regression tested together with the previous one.
This patch depends on the previous patch:
	[patch] [ia64] Fix (#2) shadowing of breakpoints
	http://sourceware.org/ml/gdb-patches/2009-09/msg00130.html


Thanks,
Jan


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

	* ia64-tdep.c (ia64_memory_insert_breakpoint)
	(ia64_memory_remove_breakpoint): Return immediately if any of memory
	reads fail.  Do not combine the VAL values.

--- gdb/ia64-tdep.c	2009-09-05 18:47:53.000000000 +0200
+++ gdb/ia64-tdep.c	2009-09-05 18:26:41.000000000 +0200
@@ -629,6 +629,11 @@ ia64_memory_insert_breakpoint (struct gd
      breakpoint instruction bits region.  */
   cleanup = make_show_memory_breakpoints_cleanup (0);
   val = target_read_memory (addr, bundle, BUNDLE_LEN);
+  if (val != 0)
+    {
+      do_cleanups (cleanup);
+      return val;
+    }
 
   /* Slot number 2 may skip at most 2 bytes at the beginning.  */
   bp_tgt->shadow_len = BUNDLE_LEN - 2;
@@ -643,7 +648,12 @@ ia64_memory_insert_breakpoint (struct gd
      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);
+  val = target_read_memory (addr, bundle, BUNDLE_LEN);
+  if (val != 0)
+    {
+      do_cleanups (cleanup);
+      return val;
+    }
 
   /* Check for L type instruction in slot 1, if present then bump up the slot
      number to the slot 2.  */
@@ -664,9 +674,8 @@ ia64_memory_insert_breakpoint (struct gd
 
   bp_tgt->placed_size = bp_tgt->shadow_len;
 
-  if (val == 0)
-    val = target_write_memory (addr + slotnum, bundle + slotnum,
-			       bp_tgt->shadow_len);
+  val = target_write_memory (addr + slotnum, bundle + slotnum,
+			     bp_tgt->shadow_len);
 
   do_cleanups (cleanup);
   return val;
@@ -693,6 +702,11 @@ ia64_memory_remove_breakpoint (struct gd
      breakpoint instruction bits region.  */
   cleanup = make_show_memory_breakpoints_cleanup (1);
   val = target_read_memory (addr, bundle_mem, BUNDLE_LEN);
+  if (val != 0)
+    {
+      do_cleanups (cleanup);
+      return val;
+    }
 
   /* Check for L type instruction in slot 1, if present then bump up the slot
      number to the slot 2.  */
@@ -721,8 +735,7 @@ ia64_memory_remove_breakpoint (struct gd
   /* 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)
-    val = target_write_memory (addr, bundle_mem, BUNDLE_LEN);
+  val = target_write_memory (addr, bundle_mem, BUNDLE_LEN);
 
   do_cleanups (cleanup);
   return val;

Re: [patch] [ia64] Fixup breakpoints errors handling

[Joel Brobecker]

> gdb/
> 2009-09-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* ia64-tdep.c (ia64_memory_insert_breakpoint)
> 	(ia64_memory_remove_breakpoint): Return immediately if any of memory
> 	reads fail.  Do not combine the VAL values.

The patch is OK. I'm just curious as to which internal_error was
triggered? Was it the one inside that function, or did it happen
elsewhere?

-- 
Joel

Re: [patch] [ia64] Fixup breakpoints errors handling

[Jan Kratochvil]

On Mon, 07 Sep 2009 20:32:59 +0200, Joel Brobecker wrote:
> On Sat, 05 Sep 2009 21:00:26 +0200, Jan Kratochvil wrote:
> > found out the code could already for example internal_error on uninitialized
> > memory after failed target_read_memory.
[...]
> > 	* ia64-tdep.c (ia64_memory_insert_breakpoint)
> > 	(ia64_memory_remove_breakpoint): Return immediately if any of memory
> > 	reads fail.  Do not combine the VAL values.
> 
> The patch is OK. I'm just curious as to which internal_error was
> triggered? Was it the one inside that function, or did it happen
> elsewhere?

I have not seen any such internal_error being triggered.

Still target_read_memory comment says:
	If an error occurs, no guarantee is made about the contents of the
	data at MYADDR.

Therefore:
  val = target_read_memory (addr, bundle, BUNDLE_LEN);
...
  instr_breakpoint = slotN_contents (bundle, slotnum);
  if (instr_breakpoint == IA64_BREAKPOINT)
    internal_error (__FILE__, __LINE__,

can induce the internal_error if either
(1) target_read_memory would just put random pattern to BUNDLE while failing
    with the bad luck of matching IA64_BREAKPOINT.
or
(2) the autovariable BUNDLE would get randomly initialized to a random
    pattern with the bad luck of matching IA64_BREAKPOINT while failing 
    target_read_memory would not touch the uninitialized content.

I do not think it could happen in the real world.


I will check it in with the other patch.


Thanks,
Jan

Re: [patch] [ia64] Fixup breakpoints errors handling

[Jan Kratochvil]

On Mon, 07 Sep 2009 20:32:59 +0200, Joel Brobecker wrote:
> > gdb/
> > 2009-09-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* ia64-tdep.c (ia64_memory_insert_breakpoint)
> > 	(ia64_memory_remove_breakpoint): Return immediately if any of memory
> > 	reads fail.  Do not combine the VAL values.
> 
> The patch is OK.

Checked-in.


Thanks,
Jan

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

--- src/gdb/ChangeLog	2009/09/08 17:39:19	1.10849
+++ src/gdb/ChangeLog	2009/09/08 17:52:26	1.10850
@@ -1,5 +1,11 @@
 2009-09-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* ia64-tdep.c (ia64_memory_insert_breakpoint)
+	(ia64_memory_remove_breakpoint): Return immediately if any of memory
+	reads fail.  Do not combine the VAL values.
+
+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
--- src/gdb/ia64-tdep.c	2009/09/08 17:39:21	1.197
+++ src/gdb/ia64-tdep.c	2009/09/08 17:52:27	1.198
@@ -629,6 +629,11 @@
      breakpoint instruction bits region.  */
   cleanup = make_show_memory_breakpoints_cleanup (0);
   val = target_read_memory (addr, bundle, BUNDLE_LEN);
+  if (val != 0)
+    {
+      do_cleanups (cleanup);
+      return val;
+    }
 
   /* Slot number 2 may skip at most 2 bytes at the beginning.  */
   bp_tgt->shadow_len = BUNDLE_LEN - 2;
@@ -645,7 +650,12 @@
      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);
+  val = target_read_memory (addr, bundle, BUNDLE_LEN);
+  if (val != 0)
+    {
+      do_cleanups (cleanup);
+      return val;
+    }
 
   /* Check for L type instruction in slot 1, if present then bump up the slot
      number to the slot 2.  */
@@ -666,9 +676,8 @@
 
   bp_tgt->placed_size = bp_tgt->shadow_len;
 
-  if (val == 0)
-    val = target_write_memory (addr + slotnum, bundle + slotnum,
-			       bp_tgt->shadow_len);
+  val = target_write_memory (addr + slotnum, bundle + slotnum,
+			     bp_tgt->shadow_len);
 
   do_cleanups (cleanup);
   return val;
@@ -695,6 +704,11 @@
      breakpoint instruction bits region.  */
   cleanup = make_show_memory_breakpoints_cleanup (1);
   val = target_read_memory (addr, bundle_mem, BUNDLE_LEN);
+  if (val != 0)
+    {
+      do_cleanups (cleanup);
+      return val;
+    }
 
   /* Check for L type instruction in slot 1, if present then bump up the slot
      number to the slot 2.  */
@@ -723,8 +737,7 @@
   /* 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)
-    val = target_write_memory (addr, bundle_mem, BUNDLE_LEN);
+  val = target_write_memory (addr, bundle_mem, BUNDLE_LEN);
 
   do_cleanups (cleanup);
   return val;