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 3/4] hw watchpoints made multi-inferior


note: reordered - this former patch 4/4 is now patch 3/4.

On Tue, 07 Dec 2010 06:01:55 +0100, Yao Qi wrote:
> On 12/06/2010 07:14 PM, Jan Kratochvil wrote:
> Here are some my cents, although I can't approve or reject this patch. :)

The review is sure welcome.


> > 	(update_watchpoint) <b->pspace != current_program_space>: New.
> 
> This changelog entry looks strange.  You can describe what is this
> change here, like,
> 
> 	* breakpoint.c (update_watchpoint): Return directly if
> breakpoint's pspace is not current_program_space.

Done.


> > 	* linux-nat.c (linux_nat_iterate_watchpoint_lwps): Fix
> > 	iterate_over_lwps FILTER.
> 
> What is FILTER?

`filter' is name of the first parameter of iterate_over_lwps.

> Why capitalized?

I follow:

http://sourceware.org/ml/gdb-patches/2008-09/msg00374.html
On Wed, 17 Sep 2008 20:03:37 +0200, Daniel Jacobowitz wrote:
# You capitalize FOO when you mean "the value of a variable named foo", but the
# name of the variable is still "foo".

But I haven't found such rule in GNU Coding Standards (GCS) now.  But some
keywords are capitalized in GCS.


> > -	  && ptid_equal (inferior_ptid, null_ptid))
> > +	  && (ptid_equal (inferior_ptid, null_ptid)
> > +	      || b->pspace != saved_current_program_space))
> >  	continue;
> 
> Please add some comments here for this change.  It is not that obvious
> to understand without multi-inferior context.

Done.


> > +      if (old_loc->pspace != current_program_space)
> > +	continue;
> 
> code indent problem?  Put extra three spaces in front of "continue".

There is <tab> before `continue;'.  The tab should be there for any 8 spaces
according to GNU indent options present in GCS.

It just gets displayed wrong in the patch file due to the first column from
diff.  I do not know an easy solution for it.  Rather well formatted code
using >=4 blocks indentation looks wrong to me (as it may not be using tabs).


> > +static void
> > +i386_inferior_data_cleanup (struct inferior *inf, void *arg)
> > +{
> > +  struct i386_inferior_data *inf_data = arg;
> > +
> > +  xfree (inf_data);
> > +}
> 
> Can't we 'xfree (arg);' directly?

This was discussed here before at least in:
	http://sourceware.org/ml/gdb-patches/2010-05/msg00162.html

The problem is primarily GDB still has not switched to C++ with templates
where ARG could have the right type instead of `void *'.

With the `void *' cast I find it both more readable to see what the parameter
in fact is.  And I also find it safer is someone needs to access some members
of `struct i386_inferior_data' in the future.  Having to re-cast `void *' to
the right type during such code changes can be IMO more fragile than to just
write it down once when one implements both the caller and the callback.

And it is just the simpler stupid writing of code when one does not have to
think about it much and it just works.

Unless decided otherwise so far I consider this style is agreed upon for GDB.


> >        /* Standard mode.  */
> > -      iterate_over_lwps (minus_one_ptid,
> > +      iterate_over_lwps (pid_to_ptid (inferior_pid),
> >  			 iterate_watchpoint_lwps_callback, &data);
> 
> Please add some comments to explain this change "minus_one_ptid ->
> pid_to_ptid (inferior_pid)".  This change is mentioned in changelog as
> "Fix iterate_over_lwps FILTER", but it is still no enough, IMO.

done.


> > +if ![runto_main] {
> 
> Please add `perror "Couldn't run ${testfile}"'
> 
> > +    return
> > +}

In any case runto_main returns 0 some kind of error message has been already
printed.  Or do I miss some case?


Thanks,
Jan


gdb/
2010-12-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix watchpoints for multi-inferior.
	* amd64-linux-nat.c (amd64_linux_dr): Remove.
	(amd64_linux_dr_set_control, amd64_linux_dr_set_addr): Remove setting
	AMD64_LINUX_DR.
	(amd64_linux_new_thread): New variable dr_mirror, use it instead of
	amd64_linux_dr.  New assert.
	* breakpoint.c (update_watchpoint): Return directly if breakpoint's
	pspace is not current_program_space.
	(insert_breakpoint_locations): New variable
	saved_current_program_space.  Exclude breakpoints not matching it.
	(watch_command_1): Initialize b->PSPACE.
	(update_global_location_list): Exclude non-matching old_loc->PSPACE.
	* i386-linux-nat.c (i386_linux_dr): Remove.
	(i386_linux_dr_set_control, i386_linux_dr_set_addr): Remove setting
	I386_LINUX_DR.
	(i386_linux_new_thread): New variable dr_mirror, use it instead of
	i386_linux_dr.  New assert.
	* i386-nat.c (DR_NADDR, struct i386_dr_mirror): Move to i386-nat.h.
	(i386_inferior_data_cleanup): New.
	(i386_inferior_data_get): Remove variable inf_data_local.  Initialize
	inf_data from an inferior_data call.
	(i386_dr_mirror_get): Make it global.
	(i386_use_watchpoints): Initialize I386_INFERIOR_DATA.
	* i386-nat.h (DR_NADDR, struct i386_dr_mirror): Moved here from
	i386-nat.c.
	(i386_dr_mirror_get): New declaration.
	* linux-nat.c (linux_nat_iterate_watchpoint_lwps): Fix
	iterate_over_lwps FILTER for multi-inferior.  Extend the comment.

gdb/testsuite/
2010-12-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix watchpoints for multi-inferior.
	* gdb.multi/watchpoint-multi.c: New file.
	* gdb.multi/watchpoint-multi.exp: New file.

--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -265,8 +265,6 @@ amd64_linux_store_inferior_registers (struct target_ops *ops,
 
 /* Support for debug registers.  */
 
-static unsigned long amd64_linux_dr[DR_CONTROL + 1];
-
 static unsigned long
 amd64_linux_dr_get (int tid, int regnum)
 {
@@ -317,8 +315,6 @@ amd64_linux_dr_set_control_callback (int tid, void *control_voidp)
 static void
 amd64_linux_dr_set_control (unsigned long control)
 {
-  amd64_linux_dr[DR_CONTROL] = control;
-
   linux_nat_iterate_watchpoint_lwps (amd64_linux_dr_set_control_callback,
 				     &control);
 }
@@ -349,8 +345,6 @@ amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr)
 
   gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
 
-  amd64_linux_dr[DR_FIRSTADDR + regnum] = addr;
-
   data.regnum = regnum;
   data.addr = addr;
   linux_nat_iterate_watchpoint_lwps (amd64_linux_dr_set_addr_callback, &data);
@@ -404,16 +398,20 @@ amd64_linux_dr_unset_status (unsigned long mask)
 static void
 amd64_linux_new_thread (ptid_t ptid)
 {
+  struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get ();
   int i, tid;
 
+  /* Verify DR_MIRROR is valid.  */
+  gdb_assert (PIDGET (ptid) == PIDGET (inferior_ptid));
+
   tid = TIDGET (ptid);
   if (tid == 0)
     tid = PIDGET (ptid);
 
-  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
-    amd64_linux_dr_set (tid, i, amd64_linux_dr[i]);
+  for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++)
+    amd64_linux_dr_set (tid, DR_FIRSTADDR + i, dr_mirror->addr[i]);
 
-  amd64_linux_dr_set (tid, DR_CONTROL, amd64_linux_dr[DR_CONTROL]);
+  amd64_linux_dr_set (tid, DR_CONTROL, dr_mirror->control);
 }
 
 
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1302,6 +1302,9 @@ update_watchpoint (struct breakpoint *b, int reparse)
   if (!watchpoint_in_thread_scope (b))
     return;
 
+  if (b->pspace != current_program_space)
+    return;
+
   /* We don't free locations.  They are stored in bp_location array and
      update_global_locations will eventually delete them and remove
      breakpoints if needed.  */
@@ -1879,6 +1882,7 @@ insert_breakpoint_locations (void)
   int val = 0;
   int disabled_breaks = 0;
   int hw_breakpoint_error = 0;
+  struct program_space *saved_current_program_space = current_program_space;
 
   struct ui_file *tmp_error_stream = mem_fileopen ();
   struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
@@ -1906,9 +1910,13 @@ insert_breakpoint_locations (void)
       /* For targets that support global breakpoints, there's no need
 	 to select an inferior to insert breakpoint to.  In fact, even
 	 if we aren't attached to any process yet, we should still
-	 insert breakpoints.  */
+	 insert breakpoints.
+
+	 Also inserting breakpoints into inappropriate inferior must be
+	 prevented.  */
       if (!gdbarch_has_global_breakpoints (target_gdbarch)
-	  && ptid_equal (inferior_ptid, null_ptid))
+	  && (ptid_equal (inferior_ptid, null_ptid)
+	      || b->pspace != saved_current_program_space))
 	continue;
 
       val = insert_bp_location (b, tmp_error_stream,
@@ -8328,6 +8336,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   b = set_raw_breakpoint_without_location (NULL, bp_type);
   set_breakpoint_number (internal, b);
   b->thread = thread;
+  b->pspace = current_program_space;
   b->disposition = disp_donttouch;
   b->exp = exp;
   b->exp_valid_block = exp_valid_block;
@@ -9478,6 +9487,9 @@ update_global_location_list (int should_insert)
       int keep_in_target = 0;
       int removed = 0;
 
+      if (old_loc->pspace != current_program_space)
+	continue;
+
       /* Skip LOCP entries which will definitely never be needed.  Stop either
 	 at or being the one matching OLD_LOC.  */
       while (locp < bp_location + bp_location_count
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -633,10 +633,6 @@ i386_linux_store_inferior_registers (struct target_ops *ops,
 }
 
 
-/* Support for debug registers.  */
-
-static unsigned long i386_linux_dr[DR_CONTROL + 1];
-
 /* Get debug register REGNUM value from only the one LWP of PTID.  */
 
 static unsigned long
@@ -689,8 +685,6 @@ i386_linux_dr_set_control_callback (int tid, void *control_voidp)
 static void
 i386_linux_dr_set_control (unsigned long control)
 {
-  i386_linux_dr[DR_CONTROL] = control;
-
   linux_nat_iterate_watchpoint_lwps (i386_linux_dr_set_control_callback,
 				     &control);
 }
@@ -721,8 +715,6 @@ i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
 
   gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
 
-  i386_linux_dr[DR_FIRSTADDR + regnum] = addr;
-
   data.regnum = regnum;
   data.addr = addr;
   linux_nat_iterate_watchpoint_lwps (i386_linux_dr_set_addr_callback, &data);
@@ -776,16 +768,20 @@ i386_linux_dr_unset_status (unsigned long mask)
 static void
 i386_linux_new_thread (ptid_t ptid)
 {
+  struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get ();
   int i, tid;
 
+  /* Verify DR_MIRROR is valid.  */
+  gdb_assert (PIDGET (ptid) == PIDGET (inferior_ptid));
+
   tid = TIDGET (ptid);
   if (tid == 0)
     tid = PIDGET (ptid);
 
-  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
-    i386_linux_dr_set (tid, i, i386_linux_dr[i]);
+  for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++)
+    i386_linux_dr_set (tid, DR_FIRSTADDR + i, dr_mirror->addr[i]);
 
-  i386_linux_dr_set (tid, DR_CONTROL, i386_linux_dr[DR_CONTROL]);
+  i386_linux_dr_set (tid, DR_CONTROL, dr_mirror->control);
 }
 
 
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -45,7 +45,6 @@ struct i386_dr_low_type i386_dr_low;
 #define TARGET_HAS_DR_LEN_8 (i386_dr_low.debug_register_length == 8)
 
 /* Debug registers' indices.  */
-#define DR_NADDR	4	/* The number of debug address registers.  */
 #define DR_STATUS	6	/* Index of debug status register (DR6).  */
 #define DR_CONTROL	7	/* Index of debug control register (DR7). */
 
@@ -105,19 +104,6 @@ struct i386_dr_low_type i386_dr_low;
    FIXME: My Intel manual says we should use 0xF800, not 0xFC00.  */
 #define DR_CONTROL_RESERVED	(0xFC00)
 
-/* Copy of hardware debug registers for performance reasons.  */
-
-struct i386_dr_mirror
-  {
-    /* Mirror the inferior's DRi registers.  We keep the status and
-       control registers separated because they don't hold addresses.  */
-    CORE_ADDR addr[DR_NADDR];
-    unsigned long status, control;
-
-    /* Reference counts for each debug register.  */
-    int ref_count[DR_NADDR];
-  };
-
 /* Auxiliary helper macros.  */
 
 /* A value that masks all fields in DR7 that are reserved by Intel.  */
@@ -229,13 +215,26 @@ struct i386_inferior_data
     struct i386_dr_mirror dr_mirror;
   };
 
+static void
+i386_inferior_data_cleanup (struct inferior *inf, void *arg)
+{
+  struct i386_inferior_data *inf_data = arg;
+
+  xfree (inf_data);
+}
+
 static struct i386_inferior_data *
 i386_inferior_data_get (void)
 {
-  /* Intermediate patch stub.  */
-  static struct i386_inferior_data inf_data_local;
   struct inferior *inf = current_inferior ();
-  struct i386_inferior_data *inf_data = &inf_data_local;
+  struct i386_inferior_data *inf_data;
+
+  inf_data = inferior_data (inf, i386_inferior_data);
+  if (inf_data == NULL)
+    {
+      inf_data = xzalloc (sizeof (*inf_data));
+      set_inferior_data (current_inferior (), i386_inferior_data, inf_data);
+    }
 
   if (inf->pid != ptid_get_pid (inferior_ptid))
     {
@@ -260,7 +259,7 @@ i386_inferior_data_get (void)
 /* Clear the reference counts and forget everything we knew about the
    debug registers.  */
 
-static struct i386_dr_mirror *
+struct i386_dr_mirror *
 i386_dr_mirror_get (void)
 {
   return &i386_inferior_data_get ()->dr_mirror;
@@ -761,6 +760,10 @@ i386_use_watchpoints (struct target_ops *t)
   t->to_remove_watchpoint = i386_remove_watchpoint;
   t->to_insert_hw_breakpoint = i386_insert_hw_breakpoint;
   t->to_remove_hw_breakpoint = i386_remove_hw_breakpoint;
+
+  if (i386_inferior_data == NULL)
+    i386_inferior_data
+      = register_inferior_data_with_cleanup (i386_inferior_data_cleanup);
 }
 
 void
--- a/gdb/i386-nat.h
+++ b/gdb/i386-nat.h
@@ -78,6 +78,25 @@ struct i386_dr_low_type
 
 extern struct i386_dr_low_type i386_dr_low;
 
+/* The number of debug address registers.  */
+#define DR_NADDR	4
+
+/* Copy of hardware debug registers for performance reasons.  */
+
+struct i386_dr_mirror
+  {
+    /* Mirror the inferior's DRi registers.  We keep the status and
+       control registers separated because they don't hold addresses.  */
+    CORE_ADDR addr[DR_NADDR];
+
+    /* Reference counts for each debug register.  */
+    int ref_count[DR_NADDR];
+
+    unsigned long status, control;
+  };
+
+extern struct i386_dr_mirror *i386_dr_mirror_get (void);
+
 /* Use this function to set i386_dr_low debug_register_length field
    rather than setting it directly to check that the length is only
    set once.  It also enables the 'maint set/show show-debug-regs' 
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1286,13 +1286,17 @@ linux_nat_iterate_watchpoint_lwps
 
   if (inf->pid == inferior_pid)
     {
-      /* Standard mode.  */
-      iterate_over_lwps (minus_one_ptid,
+      /* Standard mode.  Iterate all the threads of the current inferior.
+	 Without specifying INFERIOR_PID it would iterate all the threads of
+	 all the inferiors, which is inappropriate for watchpoints.  */
+
+      iterate_over_lwps (pid_to_ptid (inferior_pid),
 			 iterate_watchpoint_lwps_callback, &data);
     }
   else
     {
       /* Detaching a new child PID temporarily present in INFERIOR_PID.  */
+
       callback (inferior_pid, callback_data);
     }
 }
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.c
@@ -0,0 +1,46 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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 int a, b, c;
+
+static void
+marker_exit1 (void)
+{
+  a = 1;
+}
+
+/* Workaround PR breakpoints/12272 by two different breakpoint locations.  */
+static void
+marker_exit2 (void)
+{
+  a = 1;
+}
+
+int
+main (void)
+{
+  a = 1;
+  a = 1;
+  b = 2;
+  b = 2;
+  c = 3;
+  c = 3;
+
+  marker_exit1 ();
+  marker_exit2 ();
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi.exp
@@ -0,0 +1,84 @@
+# Copyright 2010 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/>.
+
+if { [is_remote target] || ![isnative] } then {
+    continue
+}
+
+set testfile "watchpoint-multi"
+
+set executable ${testfile}
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [prepare_for_testing ${testfile}.exp ${executable} ${srcfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return
+}
+# Never keep/use any non-hw breakpoints to workaround a multi-inferior bug.
+delete_breakpoints
+
+gdb_test "add-inferior" "Added inferior 2"
+gdb_test "inferior 2" "witching to inferior 2 .*"
+gdb_load $binfile
+
+if ![runto_main] {
+    return
+}
+delete_breakpoints
+
+# Simulate non-stop+target-async which also uses breakpoint always-inserted.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# Debugging of this testcase:
+#gdb_test_no_output "maintenance set show-debug-regs on"
+#gdb_test_no_output "set debug infrun 1"
+
+gdb_test "watch c" "Hardware watchpoint \[0-9\]+: c"
+# Never keep/use any non-hw breakpoints to workaround a multi-inferior bug.
+# Use `*' to workaround a multi-inferior bug.
+set test "hbreak *marker_exit2"
+gdb_test_multiple $test $test {
+    -re "Hardware assisted breakpoint \[0-9\]+ at .*\r\n$gdb_prompt $" {
+	pass $test
+    }
+    -re "(No hardware breakpoint support in the target\\.|Hardware breakpoints used exceeds limit\\.)\r\n$gdb_prompt $" {
+	pass $test
+	untested ${testfile}.exp
+	return
+    }
+}
+
+gdb_test "inferior 1" "witching to inferior 1 .*"
+
+gdb_test "watch b" "Hardware watchpoint \[0-9\]+: b"
+gdb_test "hbreak *marker_exit1" {Hardware assisted breakpoint [0-9]+ at .*}
+
+gdb_test "inferior 2" "witching to inferior 2 .*"
+
+# FAIL would be a hit on watchpoint for `b' - that one is for the other
+# inferior.
+gdb_test "continue" "Hardware watchpoint \[0-9\]+: c\r\n\r\nOld value = 0\r\nNew value = 3\r\n.*" "catch c"
+
+gdb_test "continue" {Breakpoint [0-9]+, marker_exit2 .*} "catch marker_exit2"
+
+gdb_test "inferior 1" "witching to inferior 1 .*"
+
+gdb_test "continue" "Hardware watchpoint \[0-9\]+: b\r\n\r\nOld value = 0\r\nNew value = 2\r\n.*" "catch b"
+
+gdb_test "continue" {Breakpoint [0-9]+, marker_exit1 .*} "catch marker_exit2"


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