[PATCH] [native x86 GNU/Linux] Access debug register mirror from the corresponding inferior.

[Pedro Alves <palves at redhat dot com>]

While reviewing the native AArch64 patch, I noticed a problem:

On 02/06/2013 08:46 PM, Pedro Alves wrote:
>
>> > +static void
>> > +aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
>> > +{
>> > +  struct arch_lwp_info *info = lwp->arch_private;
>> > +
>> > +  /* NULL means this is the main thread still going through the shell,
>> > +     or, no watchpoint has been set yet.  In that case, there's
>> > +     nothing to do.  */
>> > +  if (info == NULL)
>> > +    return;
>> > +
>> > +  if (DR_HAS_CHANGED (info->dr_changed_bp)
>> > +      || DR_HAS_CHANGED (info->dr_changed_wp))
>> > +    {
>> > +      int tid = GET_LWP (lwp->ptid);
>> > +      struct aarch64_debug_reg_state *state = aarch64_get_debug_reg_state ();
> Hmm.  This is always fetching the debug_reg_state of
> the current inferior, but may not be the inferior of lwp.
> I see the same bug on x86.  Sorry about that.  I'll fix it.

The fix is to make i386_debug_reg_state take an inferior pointer and
work with it instead of always using the current inferior, and then
update all callers to pass in the right inferior.

There's one wrinkle though, and one which we already handle somewhat.
When detaching the fork child that we're not interested in debugging
(set detach-on-fork off / follow-fork parent), we don't even create an
inferior for that fork child, so there's no place to get the struct
i386_debug_reg_state from, as that's stored in the inferior.

I thought of more than one way to fix this, and this seemed the
simplest - special case the null inferior case.

Other options involved creating a about_to_detach/about_to_fork_detach
hook;

Create a target side "struct process_info", thus decoupling from
struct inferior (mildly complicated, lots of mechanical changes across
all native targets that do x86 watchpoints, or

Always creating an inferior (that has lots of complications).

I don't think now's the right time to do lots of changes in this area.

Tested on Fedora 17 x86_64 -m64/-m32.

I plan to check this in a bit later to today, unless of course there
are objections.

GDBserver already fetches the i386_debug_reg_state from the right
process, and, it doesn't handle forks at all, so no fix is needed over
there.

gdb/
2013-02-07  Pedro Alves  <palves@redhat.com>

	* amd64-linux-nat.c (amd64_linux_prepare_to_resume): Handle the
	case of there being no matching inferior for the resumed lwp.
	* i386-linux-nat.c (i386_linux_prepare_to_resume): Ditto.

	* i386-nat.c (i386_inferior_data_cleanup, i386_debug_reg_state):
	New parameter 'inf'.  Use it instead of the current inferior.
	(i386_cleanup_dregs, i386_update_inferior_debug_regs)
	(i386_insert_watchpoint, i386_remove_watchpoint)
	(i386_region_ok_for_watchpoint, i386_stopped_data_address)
	(i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): Adjust to
	pass the current inferior explicitly.
	* i386-nat.h (struct inferior): Forward declare.
	(i386_debug_reg_state): New parameter 'inf'.
---
 gdb/amd64-linux-nat.c |   15 ++++++++++++++-
 gdb/i386-linux-nat.c  |   15 ++++++++++++++-
 gdb/i386-nat.c        |   31 +++++++++++++++++++------------
 gdb/i386-nat.h        |    8 +++++---
 4 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index e3e7f05..17cd2fd 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -394,9 +394,22 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
 
   if (lwp->arch_private->debug_registers_changed)
     {
-      struct i386_debug_reg_state *state = i386_debug_reg_state ();
+      int pid = ptid_get_pid (lwp->ptid);
+      struct inferior *inf = find_inferior_pid (pid);
+      struct i386_debug_reg_state *state;
       int i;
 
+      if (inf == NULL)
+	{
+	  /* NULL means this is a fork child we're not interested in
+	     debugging being detached.  We want to leave it with its
+	     debug registers cleared.  */
+	  amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+	  return;
+	}
+
+      state = i386_debug_reg_state (inf);
+
       /* On Linux kernel before 2.6.33 commit
 	 72f674d203cd230426437cdcf7dd6f681dad8b0d
 	 if you enable a breakpoint by the DR_CONTROL bits you need to have
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index 20cc032..1bfd73d 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -767,9 +767,22 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
 
   if (lwp->arch_private->debug_registers_changed)
     {
-      struct i386_debug_reg_state *state = i386_debug_reg_state ();
+      int pid = ptid_get_pid (lwp->ptid);
+      struct inferior *inf = find_inferior_pid (pid);
+      struct i386_debug_reg_state *state;
       int i;
 
+      if (inf == NULL)
+	{
+	  /* NULL means this is a fork child we're not interested in
+	     debugging being detached.  We want to leave it with its
+	     debug registers cleared.  */
+	  i386_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+	  return;
+	}
+
+      state = i386_debug_reg_state (inf);
+
       /* See amd64_linux_prepare_to_resume for Linux kernel note on
 	 i386_linux_dr_set calls ordering.  */
 
diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
index c87e753..2f0a5f4 100644
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -194,9 +194,8 @@ i386_inferior_data_cleanup (struct inferior *inf, void *arg)
    for processes being detached.  */
 
 static struct i386_inferior_data *
-i386_inferior_data_get (void)
+i386_inferior_data_get (struct inferior *inf)
 {
-  struct inferior *inf = current_inferior ();
   struct i386_inferior_data *inf_data;
 
   inf_data = inferior_data (inf, i386_inferior_data);
@@ -248,9 +247,9 @@ i386_inferior_data_get (void)
    i386_inferior_data_get.  */
 
 struct i386_debug_reg_state *
-i386_debug_reg_state (void)
+i386_debug_reg_state (struct inferior *inf)
 {
-  return &i386_inferior_data_get ()->state;
+  return &i386_inferior_data_get (inf)->state;
 }
 
 /* Whether or not to print the mirrored debug registers.  */
@@ -303,7 +302,8 @@ static int i386_handle_nonaligned_watchpoint (struct i386_debug_reg_state *state
 void
 i386_cleanup_dregs (void)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
 
   i386_init_dregs (state);
 }
@@ -569,7 +569,8 @@ Invalid value %d of operation in i386_handle_nonaligned_watchpoint.\n"),
 static void
 i386_update_inferior_debug_regs (struct i386_debug_reg_state *new_state)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   int i;
 
   ALL_DEBUG_REGISTERS (i)
@@ -594,7 +595,8 @@ static int
 i386_insert_watchpoint (CORE_ADDR addr, int len, int type,
 			struct expression *cond)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   int retval;
   /* Work on a local copy of the debug registers, and on success,
      commit the change back to the inferior.  */
@@ -631,7 +633,8 @@ static int
 i386_remove_watchpoint (CORE_ADDR addr, int len, int type,
 			struct expression *cond)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   int retval;
   /* Work on a local copy of the debug registers, and on success,
      commit the change back to the inferior.  */
@@ -664,7 +667,8 @@ i386_remove_watchpoint (CORE_ADDR addr, int len, int type,
 static int
 i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   int nregs;
 
   /* Compute how many aligned watchpoints we would need to cover this
@@ -681,7 +685,8 @@ i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
 static int
 i386_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   CORE_ADDR addr = 0;
   int i;
   int rc = 0;
@@ -766,7 +771,8 @@ static int
 i386_insert_hw_breakpoint (struct gdbarch *gdbarch,
 			   struct bp_target_info *bp_tgt)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
   CORE_ADDR addr = bp_tgt->placed_address;
   /* Work on a local copy of the debug registers, and on success,
@@ -791,7 +797,8 @@ static int
 i386_remove_hw_breakpoint (struct gdbarch *gdbarch,
 			   struct bp_target_info *bp_tgt)
 {
-  struct i386_debug_reg_state *state = i386_debug_reg_state ();
+  struct i386_debug_reg_state *state
+    = i386_debug_reg_state (current_inferior ());
   unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
   CORE_ADDR addr = bp_tgt->placed_address;
   /* Work on a local copy of the debug registers, and on success,
diff --git a/gdb/i386-nat.h b/gdb/i386-nat.h
index 87e313d..322d7cf 100644
--- a/gdb/i386-nat.h
+++ b/gdb/i386-nat.h
@@ -25,10 +25,12 @@
 
 /* Hardware-assisted breakpoints and watchpoints.  */
 
+struct inferior;
+struct target_ops;
+
 /* Add watchpoint methods to the provided target_ops.  
    Targets using i386 family debug registers for watchpoints should call
    this.  */
-struct target_ops;
 extern void i386_use_watchpoints (struct target_ops *);
 
 /* Support for hardware watchpoints and breakpoints using the i386
@@ -110,9 +112,9 @@ extern void i386_set_debug_register_length (int len);
 
 extern void i386_cleanup_dregs (void);
 
-/* Return a pointer to the the local mirror of the inferior's debug
+/* Return a pointer to the the local mirror of inferior INF's debug
    registers.  */
 
-extern struct i386_debug_reg_state *i386_debug_reg_state (void);
+extern struct i386_debug_reg_state *i386_debug_reg_state (struct inferior *inf);
 
 #endif /* I386_NAT_H */