From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19409 invoked by alias); 7 Feb 2013 16:33:50 -0000 Received: (qmail 19367 invoked by uid 22791); 7 Feb 2013 16:33:48 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Feb 2013 16:33:41 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r17GXfHr022001 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 7 Feb 2013 11:33:41 -0500 Received: from brno.lan (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r17GXdkP023191 for ; Thu, 7 Feb 2013 11:33:40 -0500 Subject: [PATCH] [native x86 GNU/Linux] Access debug register mirror from the corresponding inferior. To: gdb-patches@sourceware.org From: Pedro Alves Date: Thu, 07 Feb 2013 16:33:00 -0000 Message-ID: <20130207163339.19427.73350.stgit@brno.lan> User-Agent: StGit/0.16 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-02/txt/msg00181.txt.bz2 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 * 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 */