[PATCH] breakpoint remove fail handle bug fix

[PATCH] breakpoint remove fail handle bug fix

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 3488 bytes --]

Hi,

I sent this patch with the autoload-breakpoints patches because I found 
this issue when I test the autoload-breakpoints.  But I think sent it 
together with a feature patch list is not a good idea.  So I move it out 
for review.

This is bug was found when I when I test autoload-breakpint code.  And I 
found that it affect target-async too.
It can be reproduce:
(gdb) set target-async on
(gdb) start
Temporary breakpoint 1 at 0x4004c8: file 1.c, line 4.
Starting program: /home/teawater/tmp/a.out

Temporary breakpoint 1, main () at 1.c:4
4		sleep (20);
(gdb) disassemble
Dump of assembler code for function main:
    0x00000000004004c4 <+0>:	push   %rbp
    0x00000000004004c5 <+1>:	mov    %rsp,%rbp
=> 0x00000000004004c8 <+4>:	mov    $0x14,%edi
    0x00000000004004cd <+9>:	mov    $0x0,%eax
    0x00000000004004d2 <+14>:	callq  0x4003d0 <sleep@plt>
    0x00000000004004d7 <+19>:	mov    $0x0,%eax
    0x00000000004004dc <+24>:	pop    %rbp
    0x00000000004004dd <+25>:	retq
End of assembler dump.
(gdb) list
1	int
2	main()
3	{
4		sleep (20);
5	
6		return 0;
7	}
8	
(gdb) b 6
Breakpoint 2 at 0x4004d7: file 1.c, line 6.
(gdb) c&
Continuing.
(gdb) d
Delete all breakpoints? (y or n) y
warning: Error removing breakpoint 2
(gdb)
Program received signal SIGTRAP, Trace/breakpoint trap.
0x00000000004004d8 in main () at 1.c:6
6		return 0;
c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x00000000004004d8 in main () at 1.c:6
6		return 0;
(gdb) info reg pc
pc: 0x4004d8
(gdb) disassemble main
Dump of assembler code for function main:
    0x00000000004004c4 <+0>:	push   %rbp
    0x00000000004004c5 <+1>:	mov    %rsp,%rbp
    0x00000000004004c8 <+4>:	mov    $0x14,%edi
    0x00000000004004cd <+9>:	mov    $0x0,%eax
    0x00000000004004d2 <+14>:	callq  0x4003d0 <sleep@plt>
    0x00000000004004d7 <+19>:	int3
=> 0x00000000004004d8 <+20>:	add    %al,(%rax)
    0x00000000004004da <+22>:	add    %al,(%rax)
    0x00000000004004dc <+24>:	pop    %rbp
    0x00000000004004dd <+25>:	retq
End of assembler dump.

This because is when GDB got fail when it remove the breakpoint, it give 
up the control of this breakpoint.
There are 2 issues about it:
1. When the GDB stop, this breakpoint is not be removed.
2. If inferior is stoped by this breakpoint, adjust_pc_after_break 
didn't know this stop is beauce the breakpoint, so the pc address will 
not be adjust to the right value.

I add a list called bp_location_remove_fail_chain, when GDB got fail 
with remove a breakpoint, add it to this list.  When 
adjust_pc_after_break check if this address is the breakpint, check this 
list too.  And when gdb remve all breakpoints, try remove breakpint in 
this list.

Thanks,
Hui

2012-04-11  Hui Zhu  <hui_zhu@mentor.com>

	* breakpoint.c (ALL_BP_LOCATION_REMOV_FAIL): New macro.
	(ALL_BP_LOCATION_REMOV_FAIL_SAFE): New macro.
	(bp_location_remove_fail_chain_inserted_here_p): New function.
	(bp_location_remove_fail_chain_insert): New function.
	(bp_location_remove_fail_chain_remove): New function.
	(remove_breakpoints): Call remove_breakpoint with the bp_locations
	inside the ALL_BP_LOCATION_REMOV_FAIL_SAFE.
	(software_breakpoint_inserted_here_p): Call
	bp_location_remove_fail_chain_inserted_here_p.
	(update_global_location_list): Call
	bp_location_remove_fail_chain_insert.
	* breakpoint.h (bp_location_remove_fail_chain_remove): New extern.
	* target.c (target_kill): Call bp_location_remove_fail_chain_remove.
	(target_detach): Ditto.
	(target_disconnect): Ditto.

[-- Attachment #2: break-remove-error-change.txt --]
[-- Type: text/plain, Size: 5910 bytes --]

---
 breakpoint.c |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 breakpoint.h |    2 
 target.c     |   15 +++++-
 3 files changed, 139 insertions(+), 10 deletions(-)

--- a/breakpoint.c
+++ b/breakpoint.c
@@ -569,6 +569,100 @@ static struct cmd_list_element *breakpoi
 static struct cmd_list_element *breakpoint_show_cmdlist;
 struct cmd_list_element *save_cmdlist;
 
+/* Chains of bp_location that have remove issue. */
+static struct bp_location *bp_location_remove_fail_chain = NULL;
+
+#define ALL_BP_LOCATION_REMOV_FAIL(BP)			\
+	for (BP = bp_location_remove_fail_chain;	\
+	     BP;					\
+	     BP = BP->next)
+
+#define ALL_BP_LOCATION_REMOV_FAIL_SAFE(BP,TMP)		\
+	for (BP = bp_location_remove_fail_chain;	\
+	     BP ? (TMP=BP->next, 1): 0;			\
+	     BP = TMP)
+
+static int
+bp_location_remove_fail_chain_inserted_here_p (struct address_space *aspace,
+					       CORE_ADDR pc)
+{
+  struct bp_location *bl;
+
+  ALL_BP_LOCATION_REMOV_FAIL (bl)
+    {
+      if (bl->loc_type != bp_loc_software_breakpoint)
+	continue;
+
+      if (bl->inserted
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
+				       aspace, pc))
+	{
+	  if (overlay_debugging
+	      && section_is_overlay (bl->section)
+	      && !section_is_mapped (bl->section))
+	    continue;		/* unmapped overlay -- can't be a match */
+	  else
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
+static void
+bp_location_remove_fail_chain_insert (struct bp_location *bl)
+{
+  struct bp_location *bltmp;
+  struct breakpoint *b;
+
+  if (!bl->owner)
+    return;
+
+  ALL_BP_LOCATION_REMOV_FAIL (bltmp)
+    {
+      if (bltmp == bl)
+	return;
+    }
+
+  b = (struct breakpoint *) xzalloc (sizeof (struct breakpoint));
+  *b = *bl->owner;
+  bl->owner = b;
+
+  incref_bp_location (bl);
+  bl->next = bp_location_remove_fail_chain;
+  bp_location_remove_fail_chain = bl;
+}
+
+void
+bp_location_remove_fail_chain_remove (struct bp_location *bl)
+{
+  struct bp_location *bltmp, *bltmp2, *blprev = NULL;
+
+  ALL_BP_LOCATION_REMOV_FAIL_SAFE (bltmp, bltmp2)
+    {
+      if (!bl || bltmp == bl)
+	{
+	  if (bl)
+	    {
+	      if (blprev)
+		blprev->next = bl->next;
+	      else
+		bp_location_remove_fail_chain = bl->next;
+	    }
+	  xfree (bltmp->owner);
+	  bltmp->owner = NULL;
+	  decref_bp_location (&bltmp);
+	  if (bl)
+	    break;
+	}
+      else
+	blprev = bltmp;
+    }
+
+  if (!bl)
+    bp_location_remove_fail_chain = NULL;
+}
+
 /* Return whether a breakpoint is an active enabled breakpoint.  */
 static int
 breakpoint_enabled (struct breakpoint *b)
@@ -2594,7 +2688,7 @@ You may have requested too many hardware
 int
 remove_breakpoints (void)
 {
-  struct bp_location *bl, **blp_tmp;
+  struct bp_location *bl, *bltmp, **blp_tmp;
   int val = 0;
 
   ALL_BP_LOCATIONS (bl, blp_tmp)
@@ -2602,6 +2696,20 @@ remove_breakpoints (void)
     if (bl->inserted && !is_tracepoint (bl->owner))
       val |= remove_breakpoint (bl, mark_uninserted);
   }
+
+  ALL_BP_LOCATION_REMOV_FAIL_SAFE (bl, bltmp)
+    {
+      volatile struct gdb_exception e;
+      int ret;
+
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  ret = remove_breakpoint (bl, mark_uninserted);
+	}
+      if (!ret && e.reason >= 0)
+	bp_location_remove_fail_chain_remove (bl);
+    }
+
   return val;
 }
 
@@ -3536,6 +3644,9 @@ software_breakpoint_inserted_here_p (str
   if (single_step_breakpoint_inserted_here_p (aspace, pc))
     return 1;
 
+  if (bp_location_remove_fail_chain_inserted_here_p (aspace, pc))
+    return 1;
+
   return 0;
 }
 
@@ -11650,7 +11761,14 @@ update_global_location_list (int should_
 
 	  if (!keep_in_target)
 	    {
-	      if (remove_breakpoint (old_loc, mark_uninserted))
+	      volatile struct gdb_exception e;
+	      int ret;
+
+	      TRY_CATCH (e, RETURN_MASK_ERROR)
+		{
+		  ret = remove_breakpoint (old_loc, mark_uninserted);
+		}
+	      if (ret || e.reason < 0)
 		{
 		  /* This is just about all we can do.  We could keep
 		     this location on the global list, and try to
@@ -11663,8 +11781,11 @@ update_global_location_list (int should_
 		  printf_filtered (_("warning: Error removing "
 				     "breakpoint %d\n"), 
 				   old_loc->owner->number);
+		  removed = 0;
+		  bp_location_remove_fail_chain_insert (old_loc);
 		}
-	      removed = 1;
+	      else
+		removed = 1;
 	    }
 	}
 
@@ -11725,10 +11846,7 @@ update_global_location_list (int should_
 	      VEC_safe_push (bp_location_p, moribund_locations, old_loc);
 	    }
 	  else
-	    {
-	      old_loc->owner = NULL;
-	      decref_bp_location (&old_loc);
-	    }
+	    decref_bp_location (&old_loc);
 	}
     }
 
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -1480,4 +1480,6 @@ extern struct gdbarch *get_sal_arch (str
 
 extern void handle_solib_event (void);
 
+extern void bp_location_remove_fail_chain_remove (struct bp_location *bl);
+
 #endif /* !defined (BREAKPOINT_H) */
--- a/target.c
+++ b/target.c
@@ -454,6 +454,9 @@ target_kill (void)
 	  fprintf_unfiltered (gdb_stdlog, "target_kill ()\n");
 
         t->to_kill (t);
+
+	bp_location_remove_fail_chain_remove (NULL);
+
 	return;
       }
 
@@ -2568,9 +2571,13 @@ target_detach (char *args, int from_tty)
        disconnection from the target.  */
     ;
   else
-    /* If we're in breakpoints-always-inserted mode, have to remove
-       them before detaching.  */
-    remove_breakpoints_pid (PIDGET (inferior_ptid));
+    {
+      /* If we're in breakpoints-always-inserted mode, have to remove
+	 them before detaching.  */
+      remove_breakpoints_pid (PIDGET (inferior_ptid));
+
+      bp_location_remove_fail_chain_remove (NULL);
+    }
 
   prepare_for_detach ();
 
@@ -2599,6 +2606,8 @@ target_disconnect (char *args, int from_
      disconnecting.  */
   remove_breakpoints ();
 
+  bp_location_remove_fail_chain_remove (NULL);
+
   for (t = current_target.beneath; t != NULL; t = t->beneath)
     if (t->to_disconnect != NULL)
 	{

Re: [PATCH] breakpoint remove fail handle bug fix

[Yao Qi]

Here are some my thoughts on this problem.  I didn't read patch
carefully, so have no comments on it.

On 04/11/2012 05:07 PM, Hui Zhu wrote:
> This because is when GDB got fail when it remove the breakpoint, it give
> up the control of this breakpoint.

You are removing breakpoints when inferior is in background execution.
It is expected or reasonable that GBD is unable to remove breakpoint
instructions from inferior when it is running (due to ptrace limitation
IIRC).

> There are 2 issues about it:
> 1. When the GDB stop, this breakpoint is not be removed.

This is because breakpoint instances (not breakpoint instructions
written in inferior) was remove when inferior is running.  When inferior
stops, breakpoint instructions are not removed from inferior, because
there is no breakpoint instances in GDB.  Current policy in GDB is that
even we get something wrong in removing breakpoints, still remove them
from global list.  See the comment in breakpoint.c:

	      if (remove_breakpoint (old_loc, mark_uninserted))
		{
		  /* This is just about all we can do.  We could keep
		     this location on the global list, and try to
		     remove it next time, but there's no particular
		     reason why we will succeed next time.
		
		     Note that at this point, old_loc->owner is still
		     valid, as delete_breakpoint frees the breakpoint
		     only after calling us.  */
		  printf_filtered (_("warning: Error removing "
				     "breakpoint %d\n"),
				   old_loc->owner->number);
		}

If we change the policy to "keep breakpoint instances if fail to remove
from inferior", this problem may be fixed.

> 2. If inferior is stoped by this breakpoint, adjust_pc_after_break
> didn't know this stop is beauce the breakpoint, so the pc address will
> not be adjust to the right value.

adjust_pc_after_break doesn't know because there is no breakpoint
instance in GDB.

-- 
Yao (齐尧)

Re: [PATCH] breakpoint remove fail handle bug fix

[Jan Kratochvil]

On Wed, 11 Apr 2012 11:07:08 +0200, Hui Zhu wrote:
> (gdb) d
> Delete all breakpoints? (y or n) y
> warning: Error removing breakpoint 2

I would propose the attached patch instead.

It needs a testcase, would you write one?

Not sure if gdbserver also needs a fix or not.

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.


Thanks,
Jan


gdb/
2012-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linux-nat.c (linux_proc_xfer_partial): Do not check for LEN size and
	support also WRITEBUF.
	(linux_xfer_partial): Move here the LEN check from
	linux_proc_xfer_partial but also call linux_proc_xfer_partial as a last
	resort if super_xfer_partial fails.

--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4494,9 +4494,9 @@ linux_nat_make_corefile_notes (bfd *obfd, int *note_size)
 }
 
 /* Implement the to_xfer_partial interface for memory reads using the /proc
-   filesystem.  Because we can use a single read() call for /proc, this
-   can be much more efficient than banging away at PTRACE_PEEKTEXT,
-   but it doesn't support writes.  */
+   filesystem.  Because we can use a single read or write call for /proc, this
+   can be much more efficient than banging away at PTRACE_PEEKTEXT or
+   PTRACE_POKETEXT.  */
 
 static LONGEST
 linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
@@ -4508,29 +4508,35 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
   int fd;
   char filename[64];
 
-  if (object != TARGET_OBJECT_MEMORY || !readbuf)
-    return 0;
-
-  /* Don't bother for one word.  */
-  if (len < 3 * sizeof (long))
+  if (object != TARGET_OBJECT_MEMORY)
     return 0;
 
   /* We could keep this file open and cache it - possibly one per
      thread.  That requires some juggling, but is even faster.  */
   sprintf (filename, "/proc/%d/mem", PIDGET (inferior_ptid));
-  fd = open (filename, O_RDONLY | O_LARGEFILE);
+  fd = open (filename, (readbuf ? O_RDONLY : O_WRONLY) | O_LARGEFILE);
   if (fd == -1)
     return 0;
 
-  /* If pread64 is available, use it.  It's faster if the kernel
+  /* If pread64 or pwrite64 is available, use it.  It's faster if the kernel
      supports it (only one syscall), and it's 64-bit safe even on
      32-bit platforms (for instance, SPARC debugging a SPARC64
      application).  */
+  if ((readbuf != NULL
+#ifdef HAVE_PREAD64
+       && (pread64 (fd, readbuf, len, offset) != len)
+#else
+       && (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
+#endif
+       )
+      || (writebuf != NULL
 #ifdef HAVE_PREAD64
-  if (pread64 (fd, readbuf, len, offset) != len)
+	  && (pwrite64 (fd, writebuf, len, offset) != len)
 #else
-  if (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
+	  && (lseek (fd, offset, SEEK_SET) == -1
+	      || write (fd, writebuf, len) != len)
 #endif
+          ))
     ret = 0;
   else
     ret = len;
@@ -4759,13 +4765,24 @@ linux_xfer_partial (struct target_ops *ops, enum target_object object,
 	offset &= ((ULONGEST) 1 << addr_bit) - 1;
     }
 
-  xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
-				  offset, len);
+  /* Use more expensive linux_proc_xfer_partial only for larger transfers.  */
+  if (len >= 3 * sizeof (long))
+    {
+      xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
+				      offset, len);
+      if (xfer != 0)
+	return xfer;
+    }
+
+  xfer = super_xfer_partial (ops, object, annex, readbuf, writebuf,
+			     offset, len);
   if (xfer != 0)
     return xfer;
 
-  return super_xfer_partial (ops, object, annex, readbuf, writebuf,
-			     offset, len);
+  /* PTRACE_* of super_xfer_partial may not work if the inferior is running.
+     linux_proc_xfer_partial still may work in such case.  */
+  return linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
+				  offset, len);
 }
 
 static void

Re: [PATCH] breakpoint remove fail handle bug fix

[Hui Zhu]

On 04/12/12 04:35, Jan Kratochvil wrote:
> On Wed, 11 Apr 2012 11:07:08 +0200, Hui Zhu wrote:
>> (gdb) d
>> Delete all breakpoints? (y or n) y
>> warning: Error removing breakpoint 2
>
> I would propose the attached patch instead.
>
> It needs a testcase, would you write one?
>
> Not sure if gdbserver also needs a fix or not.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.

Hi Jan,

Thanks for your patch.  And it handle the local debug very well.
But it didn't handle the issue when the target is remote.  I am sorry 
that I didn't talk clear in my mail.  I use the gdb that patch your 
patch and do a test with remote:

(gdb) set target-async on
(reverse-i-search)`target': set ^CQuit-async on
(gdb) disassemble
No frame selected.
(gdb) target remote :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols 
found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7ddcaf0 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) tb main
Temporary breakpoint 1 at 0x4004c8: file tmp.c, line 4.
(gdb) disassemble
No function contains program counter for selected frame.
(gdb) c
Continuing.

Program received signal SIGINT, Interrupt.
0x00007ffff7ddcaf0 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) c
Continuing.

Temporary breakpoint 1, main () at tmp.c:4
4	sleep (20);
(gdb) disas
Dump of assembler code for function main:
    0x00000000004004c4 <+0>:	push   %rbp
    0x00000000004004c5 <+1>:	mov    %rsp,%rbp
=> 0x00000000004004c8 <+4>:	mov    $0x14,%edi
    0x00000000004004cd <+9>:	mov    $0x0,%eax
    0x00000000004004d2 <+14>:	callq  0x4003d0 <sleep@plt>
    0x00000000004004d7 <+19>:	mov    $0x0,%eax
    0x00000000004004dc <+24>:	pop    %rbp
    0x00000000004004dd <+25>:	retq
End of assembler dump.
(gdb) b 6
Breakpoint 2 at 0x4004d7: file tmp.c, line 6.
(gdb) c &
Continuing.
(gdb) d
Delete all breakpoints? (y or n) y
Cannot execute this command while the target is running.
(gdb)
Program received signal SIGTRAP, Trace/breakpoint trap.
0x00000000004004d8 in main () at tmp.c:6
6	return 0;
c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x00000000004004d8 in main () at tmp.c:6
6	return 0;
(gdb) c
Continuing.

Program terminated with signal SIGSEGV, Segmentation fault.
The program no longer exists.
(gdb)

Do you have some good idea on this part?


Thanks,
Hui

>
>
> Thanks,
> Jan
>
>
> gdb/
> 2012-04-11  Jan Kratochvil<jan.kratochvil@redhat.com>
>
> 	* linux-nat.c (linux_proc_xfer_partial): Do not check for LEN size and
> 	support also WRITEBUF.
> 	(linux_xfer_partial): Move here the LEN check from
> 	linux_proc_xfer_partial but also call linux_proc_xfer_partial as a last
> 	resort if super_xfer_partial fails.
>
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4494,9 +4494,9 @@ linux_nat_make_corefile_notes (bfd *obfd, int *note_size)
>   }
>
>   /* Implement the to_xfer_partial interface for memory reads using the /proc
> -   filesystem.  Because we can use a single read() call for /proc, this
> -   can be much more efficient than banging away at PTRACE_PEEKTEXT,
> -   but it doesn't support writes.  */
> +   filesystem.  Because we can use a single read or write call for /proc, this
> +   can be much more efficient than banging away at PTRACE_PEEKTEXT or
> +   PTRACE_POKETEXT.  */
>
>   static LONGEST
>   linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
> @@ -4508,29 +4508,35 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
>     int fd;
>     char filename[64];
>
> -  if (object != TARGET_OBJECT_MEMORY || !readbuf)
> -    return 0;
> -
> -  /* Don't bother for one word.  */
> -  if (len<  3 * sizeof (long))
> +  if (object != TARGET_OBJECT_MEMORY)
>       return 0;
>
>     /* We could keep this file open and cache it - possibly one per
>        thread.  That requires some juggling, but is even faster.  */
>     sprintf (filename, "/proc/%d/mem", PIDGET (inferior_ptid));
> -  fd = open (filename, O_RDONLY | O_LARGEFILE);
> +  fd = open (filename, (readbuf ? O_RDONLY : O_WRONLY) | O_LARGEFILE);
>     if (fd == -1)
>       return 0;
>
> -  /* If pread64 is available, use it.  It's faster if the kernel
> +  /* If pread64 or pwrite64 is available, use it.  It's faster if the kernel
>        supports it (only one syscall), and it's 64-bit safe even on
>        32-bit platforms (for instance, SPARC debugging a SPARC64
>        application).  */
> +  if ((readbuf != NULL
> +#ifdef HAVE_PREAD64
> +&&  (pread64 (fd, readbuf, len, offset) != len)
> +#else
> +&&  (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
> +#endif
> +       )
> +      || (writebuf != NULL
>   #ifdef HAVE_PREAD64
> -  if (pread64 (fd, readbuf, len, offset) != len)
> +	&&  (pwrite64 (fd, writebuf, len, offset) != len)
>   #else
> -  if (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
> +	&&  (lseek (fd, offset, SEEK_SET) == -1
> +	      || write (fd, writebuf, len) != len)
>   #endif
> +          ))
>       ret = 0;
>     else
>       ret = len;
> @@ -4759,13 +4765,24 @@ linux_xfer_partial (struct target_ops *ops, enum target_object object,
>   	offset&= ((ULONGEST) 1<<  addr_bit) - 1;
>       }
>
> -  xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
> -				  offset, len);
> +  /* Use more expensive linux_proc_xfer_partial only for larger transfers.  */
> +  if (len>= 3 * sizeof (long))
> +    {
> +      xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
> +				      offset, len);
> +      if (xfer != 0)
> +	return xfer;
> +    }
> +
> +  xfer = super_xfer_partial (ops, object, annex, readbuf, writebuf,
> +			     offset, len);
>     if (xfer != 0)
>       return xfer;
>
> -  return super_xfer_partial (ops, object, annex, readbuf, writebuf,
> -			     offset, len);
> +  /* PTRACE_* of super_xfer_partial may not work if the inferior is running.
> +     linux_proc_xfer_partial still may work in such case.  */
> +  return linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
> +				  offset, len);
>   }
>
>   static void

Re: [PATCH] breakpoint remove fail handle bug fix

[Jan Kratochvil]

On Thu, 12 Apr 2012 04:16:50 +0200, Hui Zhu wrote:
> But it didn't handle the issue when the target is remote.

So a similar fix is needed also for gdbserver, isn't it?

One needs to write a testcase for later check-in anyway, testing gdbserver
without the testcase/testsuite I find too painful.

I still do not see a reason to really handled failed breakpoint removal (or
insert), gdbserver also can access /proc/PID/mem etc., cannot it?


Thanks,
Jan

Re: [PATCH] breakpoint remove fail handle bug fix

[Hui Zhu]

On 04/16/12 17:49, Jan Kratochvil wrote:
> On Thu, 12 Apr 2012 04:16:50 +0200, Hui Zhu wrote:
>> But it didn't handle the issue when the target is remote.
>
> So a similar fix is needed also for gdbserver, isn't it?
>
> One needs to write a testcase for later check-in anyway, testing gdbserver
> without the testcase/testsuite I find too painful.
>
> I still do not see a reason to really handled failed breakpoint removal (or
> insert), gdbserver also can access /proc/PID/mem etc., cannot it?
>
>
> Thanks,
> Jan

I just try to handle this issue for current issue or other issue about 
breakpoint in the future for example that a remote target cannot delete 
breakpoint when the inferior is running.  But I am OK if you think it is 
not very important.

I suggest we can doc clear about this part for the gdbserver developer.

Thanks,
Hui

Re: [PATCH] breakpoint remove fail handle bug fix

[Jan Kratochvil]

On Thu, 12 Apr 2012 04:16:50 +0200, Hui Zhu wrote:
> But it didn't handle the issue when the target is remote.
[...]
> (gdb) c &
> Continuing.
> (gdb) d
> Delete all breakpoints? (y or n) y
> Cannot execute this command while the target is running.

This is because you did not turn on non-stop mode:
remote.c:
  if (!non_stop && target_can_async_p () && rs->waiting_for_stop_reply)
    error (_("Cannot execute this command while the target is running."));

It works then, gdbserver has this bug already fixed.

Just I find it a loss of time fixing linux-nat which should get obsoleted by
gdbserver anyway so posting this patch+testcase just FYI.


Regards,
Jan


diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index ac1a0ea..299b3ca 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4494,9 +4494,9 @@ linux_nat_make_corefile_notes (bfd *obfd, int *note_size)
 }
 
 /* Implement the to_xfer_partial interface for memory reads using the /proc
-   filesystem.  Because we can use a single read() call for /proc, this
-   can be much more efficient than banging away at PTRACE_PEEKTEXT,
-   but it doesn't support writes.  */
+   filesystem.  Because we can use a single read or write call for /proc, this
+   can be much more efficient than banging away at PTRACE_PEEKTEXT or
+   PTRACE_POKETEXT.  */
 
 static LONGEST
 linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
@@ -4508,29 +4508,35 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
   int fd;
   char filename[64];
 
-  if (object != TARGET_OBJECT_MEMORY || !readbuf)
-    return 0;
-
-  /* Don't bother for one word.  */
-  if (len < 3 * sizeof (long))
+  if (object != TARGET_OBJECT_MEMORY)
     return 0;
 
   /* We could keep this file open and cache it - possibly one per
      thread.  That requires some juggling, but is even faster.  */
   sprintf (filename, "/proc/%d/mem", PIDGET (inferior_ptid));
-  fd = open (filename, O_RDONLY | O_LARGEFILE);
+  fd = open (filename, (readbuf ? O_RDONLY : O_WRONLY) | O_LARGEFILE);
   if (fd == -1)
     return 0;
 
-  /* If pread64 is available, use it.  It's faster if the kernel
+  /* If pread64 or pwrite64 is available, use it.  It's faster if the kernel
      supports it (only one syscall), and it's 64-bit safe even on
      32-bit platforms (for instance, SPARC debugging a SPARC64
      application).  */
+  if ((readbuf != NULL
+#ifdef HAVE_PREAD64
+       && (pread64 (fd, readbuf, len, offset) != len)
+#else
+       && (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
+#endif
+       )
+      || (writebuf != NULL
 #ifdef HAVE_PREAD64
-  if (pread64 (fd, readbuf, len, offset) != len)
+	  && (pwrite64 (fd, writebuf, len, offset) != len)
 #else
-  if (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
+	  && (lseek (fd, offset, SEEK_SET) == -1
+	      || write (fd, writebuf, len) != len)
 #endif
+          ))
     ret = 0;
   else
     ret = len;
@@ -4759,13 +4765,24 @@ linux_xfer_partial (struct target_ops *ops, enum target_object object,
 	offset &= ((ULONGEST) 1 << addr_bit) - 1;
     }
 
-  xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
-				  offset, len);
+  /* Use more expensive linux_proc_xfer_partial only for larger transfers.  */
+  if (len >= 3 * sizeof (long))
+    {
+      xfer = linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
+				      offset, len);
+      if (xfer != 0)
+	return xfer;
+    }
+
+  xfer = super_xfer_partial (ops, object, annex, readbuf, writebuf,
+			     offset, len);
   if (xfer != 0)
     return xfer;
 
-  return super_xfer_partial (ops, object, annex, readbuf, writebuf,
-			     offset, len);
+  /* PTRACE_* of super_xfer_partial may not work if the inferior is running.
+     linux_proc_xfer_partial still may work in such case.  */
+  return linux_proc_xfer_partial (ops, object, annex, readbuf, writebuf,
+				  offset, len);
 }
 
 static void
diff --git a/gdb/testsuite/gdb.base/async-breakpoint.c b/gdb/testsuite/gdb.base/async-breakpoint.c
new file mode 100644
index 0000000..3cdb0c1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/async-breakpoint.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012 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/>.  */
+
+static volatile v;
+
+static int
+func (void)
+{
+  return ++v;
+}
+
+static volatile int stopme;
+
+int
+main (void)
+{
+  int timeout = 60 * 10;
+
+  while (!stopme && timeout-- > 0)
+    usleep (1000000 / 10);
+
+  return func ();
+}
diff --git a/gdb/testsuite/gdb.base/async-breakpoint.exp b/gdb/testsuite/gdb.base/async-breakpoint.exp
new file mode 100644
index 0000000..d2469cf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/async-breakpoint.exp
@@ -0,0 +1,59 @@
+# Copyright (C) 2012 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 async-breakpoint
+
+# Required for non-stop.
+if ![support_displaced_stepping] { 
+    unsupported "displaced stepping"
+    return -1
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile}] } {
+    return -1
+}
+
+gdb_test_no_output "set target-async on"
+
+# Required for remote targets due to:
+# Cannot execute this command while the target is running.
+# Required also for local mode as GDB does not insert breakpoints otherwise.
+gdb_test_no_output "set non-stop on"
+
+if ![runto_main] then {
+   return 0
+}
+
+delete_breakpoints
+
+gdb_breakpoint "main"
+
+gdb_test "continue &" {Continuing\.}
+
+# FAIL was: warning: Error removing breakpoint 1
+gdb_test_no_output {delete $bpnum}
+
+# FAIL was: Cannot access memory at address 0x40057c
+gdb_breakpoint "func"
+
+set test "set variable stopme=1"
+gdb_test_multiple $test $test {
+    -re "$gdb_prompt " {
+	exp_continue
+    }
+    -re "Breakpoint \[0-9\]+, func \\(\\) at " {
+	pass $test
+    }
+}
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index ee66e48..557e86d 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -47,6 +47,9 @@
 proc gdb_target_cmd { targetname serialport } {
     global gdb_prompt
 
+    # Do not use ending anchor for "$gdb_prompt $" as in async mode we may get
+    # also notification of the stopped target.
+
     set serialport_re [string_to_regexp $serialport]
     for {set i 1} {$i <= 3} {incr i} {
 	send_gdb "target $targetname $serialport\n"
@@ -58,26 +61,26 @@ proc gdb_target_cmd { targetname serialport } {
 	    -re "unknown host.*$gdb_prompt" {
 	        verbose "Couldn't look up $serialport"
 	    }
-	    -re "Couldn't establish connection to remote.*$gdb_prompt $" {
+	    -re "Couldn't establish connection to remote.*$gdb_prompt " {
 		verbose "Connection failed"
 	    }
 	    -re "Remote MIPS debugging.*$gdb_prompt" {
 		verbose "Set target to $targetname"
 		return 0
 	    }
-	    -re "Remote debugging using .*$serialport_re.*$gdb_prompt $" {
+	    -re "Remote debugging using .*$serialport_re.*$gdb_prompt " {
 		verbose "Set target to $targetname"
 		return 0
 	    }
-	    -re "Remote debugging using stdio.*$gdb_prompt $" {
+	    -re "Remote debugging using stdio.*$gdb_prompt " {
 		verbose "Set target to $targetname"
 		return 0
 	    }
-	    -re "Remote target $targetname connected to.*$gdb_prompt $" {
+	    -re "Remote target $targetname connected to.*$gdb_prompt " {
 		verbose "Set target to $targetname"
 		return 0
 	    }
-	    -re "Connected to.*$gdb_prompt $" { 
+	    -re "Connected to.*$gdb_prompt " { 
 		verbose "Set target to $targetname"
 		return 0
 	    }

Re: [PATCH] breakpoint remove fail handle bug fix

[Doug Evans]

On Wed, Apr 11, 2012 at 1:35 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Wed, 11 Apr 2012 11:07:08 +0200, Hui Zhu wrote:
>> (gdb) d
>> Delete all breakpoints? (y or n) y
>> warning: Error removing breakpoint 2
>
> I would propose the attached patch instead.
>
> It needs a testcase, would you write one?
>
> Not sure if gdbserver also needs a fix or not.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
>
>
> Thanks,
> Jan
>
>
> gdb/
> 2012-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        * linux-nat.c (linux_proc_xfer_partial): Do not check for LEN size and
>        support also WRITEBUF.
>        (linux_xfer_partial): Move here the LEN check from
>        linux_proc_xfer_partial but also call linux_proc_xfer_partial as a last
>        resort if super_xfer_partial fails.

fwiw,
This comment:

+  /* PTRACE_* of super_xfer_partial may not work if the inferior is running.
+     linux_proc_xfer_partial still may work in such case.  */

is not sufficient, to me anyway, to tell me why the code is the way it is.
[E.g., *why* is it important that the write succeed?]
The reader of the code a year from now will have no idea of connection
between this code and breakpoint handling issues.

[Setting aside the fact that recording an operation as having
succeeded when it did not (and even knowing it did not) is just asking
for recurring trouble, and any patch that doesn't fix that underlying
problem is just papering over the real problem.]

Re: [PATCH] breakpoint remove fail handle bug fix

[Doug Evans]

On Wed, Apr 11, 2012 at 2:07 AM, Hui Zhu <hui_zhu@mentor.com> wrote:
> (gdb) d
> Delete all breakpoints? (y or n) y
> warning: Error removing breakpoint 2

At this point:

(gdb) i b
(gdb) # breakpoint is gone as far as gdb is concerned

Eewwww. :-(

Re: [PATCH] breakpoint remove fail handle bug fix

[Hui Zhu]

On 04/12/12 12:17, Doug Evans wrote:
> On Wed, Apr 11, 2012 at 2:07 AM, Hui Zhu<hui_zhu@mentor.com>  wrote:
>> (gdb) d
>> Delete all breakpoints? (y or n) y
>> warning: Error removing breakpoint 2
>
> At this point:
>
> (gdb) i b
> (gdb) # breakpoint is gone as far as gdb is concerned
>
> Eewwww. :-(

Yes.  I handle this issue through is deleted them from the breakpoint 
list and put them to a special list.  When the GDB try to remove the 
breakpoint, it will try again with this list.

I suggest that even if we can fix the issue that make the target-async 
get that breakpoint error, I need a way to handle the error in 
breakpoint part.

What do you think about it?

Thanks,
Hui