From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15792 invoked by alias); 11 Dec 2010 05:16:23 -0000 Received: (qmail 15783 invoked by uid 22791); 11 Dec 2010 05:16:21 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,T_RP_MATCHES_RCVD 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; Sat, 11 Dec 2010 05:16:15 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oBB5GDjn016001 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 11 Dec 2010 00:16:13 -0500 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id oBB5G73J008292 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Sat, 11 Dec 2010 00:16:12 -0500 Received: from host0.dyn.jankratochvil.net (host0.dyn.jankratochvil.net [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id oBB5G7OS013771; Sat, 11 Dec 2010 06:16:07 +0100 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id oBB5G6mf013769; Sat, 11 Dec 2010 06:16:06 +0100 Date: Sat, 11 Dec 2010 05:16:00 -0000 From: Jan Kratochvil To: Mark Kettenis Cc: gdb-patches@sourceware.org Subject: Re: [patch 4/4] hw watchpoints kernel workaround Message-ID: <20101211051606.GD10298@host0.dyn.jankratochvil.net> References: <20101206111405.GD27176@host0.dyn.jankratochvil.net> <201012061345.oB6DjepW005461@glazunov.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201012061345.oB6DjepW005461@glazunov.sibelius.xs4all.nl> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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: 2010-12/txt/msg00156.txt.bz2 note: reordered - this former patch 3/4 is now patch 4/4. On Mon, 06 Dec 2010 14:45:40 +0100, Mark Kettenis wrote: > That's too bad. Please do at least mention some kernel version > numbers, such that 10 years from now we have an idea when the > behaviour changed. OK, asked Oleg Nesterov for some hash/version, included them. > Not sure I understand the issue correctly. You mention two changes. > So is there a period in between those changes where things are broken, > and are things fixed now? Or is the EINVAL behaviour there to stay? According to Oleg (I did not know) the two changes went in (probably) by a single upstream commit. The EINVAL problem is still present in any upstream 2.6.33+ kernel so far. > Also, do you know if the problematic behaviour ever was in a kernel > officially released by Linus? Oleg says so and I have verified it on a Fedora kernel with disabled utrace, therefore the ptrace behavior should match the upstream one. kernel-2.6.35.6-48.noutrace.fc13 http://koji.fedoraproject.org/scratch/jkratoch/task_2644776/ (it will get discarded in a week) > If not, I'm inclined to suggest that the GDB should only have to deal with > the old behaviour and the final new behaviour, and that RedHat should ^^^^^^ = Red Hat > distribute a kernel update to make their kernels conform to either the old > or the final new behaviour. I do not post GDB patches dealing with Fedora / Red Hat specific variants for FSF GDB. > > pid != inferior_pid>: New. > > The <...> bit in the ChangeLog is odd. I don't think that's an > acceptable way to describe changes according to the coding standards. OK, changed. > > --- a/gdb/i386-nat.c > > +++ b/gdb/i386-nat.c > > @@ -453,6 +453,24 @@ i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits) > > { > > struct dr_mirror *dr_mirror = dr_mirror_get (); > > int i, retval = -1; > > + int inferior_pid = ptid_get_pid (inferior_ptid); > > + struct inferior *inf = current_inferior (); > > + > > + /* Are we detaching breakpoints from a fork-ed child? > > + See linux_nat_iterate_watchpoint_lwps. */ > > It's not abvious to me what this "See > linux_nat_iterate_watchpoint_lwps" is referring to. linux_nat_iterate_watchpoint_lwps function comment and the two code paths inside also contain comments about the variables state during detaching breakpoints from a forked child. As GDB contained bugs so far and I also do not find this issue so obvious I wanted to introduce the reader to this problem more than just by wods "fork-ed child". I did not want to copy the comments as they will get out of sync and obsolete in the future otherwise, as discussed in other mails. Specific suggestion on improvements of these comments is welcome. > But I think you'd better rewrite that comment completely since it > would really help if you could explain what old kernels did and what > new kernels do and that you need to reset the breakpoints in the > fork-ed child to deal with the new behaviour. Done. > Also, if the EINVAL behaviour is there to stay, don't say "Work > around" but simly say "Deal with the fact that the Linux kernel > behaviour changed", or something like that. I + Oleg believe the EINVAL is a clear regression and bug which needs to be fixed upstream. Just it is present in Linus Torvalds released kernels for long enough it is IMO worth to be work arounded by FSF GDB. > It doesn't seem inconceivable that other (non-Linux) kernels also need > this bit of code, so i386-nat.c is an appropriate place for it. I agree, fixed. Thanks, Jan gdb/ 2010-12-06 Jan Kratochvil Workaround Linux kernel Bug - Red Hat Bugzilla #660204. * amd64-linux-nat.c (amd64_linux_dr_set_addr): New declaration. (amd64_linux_dr_set_control): New variables inferior_pid and inf. Pre-set address registers if we are detaching breakpoints now. * i386-linux-nat.c (i386_linux_dr_set_addr): New declaration. (i386_linux_dr_set_control): New variables inferior_pid and inf. Pre-set address registers if we are detaching breakpoints now. * i386-nat.c (i386_inferior_data_get): Reset ADDR_PRESET for fork-ed child. (i386_cleanup_dregs): Reset ADDR_PRESET. * i386-nat.h (struct i386_dr_mirror) pid != inferior_pid) + { + struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get (); + int i; + + /* There were two changes in Linux kernel 2.6.33 by the commit: + 72f674d203cd230426437cdcf7dd6f681dad8b0d + + (1) After fork/vfork/clone the new task no longer inherits the debug + registers. It has them zeroed instead. Either case is OK for GDB as + GDB already registers a fix up by linux_nat_set_new_thread. + + (2) If you enable a breakpoint by the CONTROL bits you have already + written its ADDRESS. Otherwise Linux kernel will report EINVAL. + For this case the workaround here ensures that during resetting + (detaching) watchpoints for a fork-ed child we can set CONTROL + arbitrarily as the addresses get pre-set here just to be sure. + + The second issue is hopefully going to be fixed in Linux kernel: + https://bugzilla.redhat.com/show_bug.cgi?id=660204 */ + + if (!dr_mirror->addr_preset) + { + dr_mirror->addr_preset = 1; + + for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++) + amd64_linux_dr_set_addr (i, dr_mirror->addr[i]); + } + } + linux_nat_iterate_watchpoint_lwps (amd64_linux_dr_set_control_callback, &control); } --- a/gdb/i386-linux-nat.c +++ b/gdb/i386-linux-nat.c @@ -680,11 +680,31 @@ i386_linux_dr_set_control_callback (int tid, void *control_voidp) i386_linux_dr_set (tid, DR_CONTROL, control); } +static void i386_linux_dr_set_addr (int regnum, CORE_ADDR addr); + /* Set DR_CONTROL to ADDR in all LWPs of CURRENT_INFERIOR. */ static void i386_linux_dr_set_control (unsigned long control) { + int inferior_pid = ptid_get_pid (inferior_ptid); + struct inferior *inf = current_inferior (); + + /* The amd64 counterpart and description is amd64_linux_dr_set_control. */ + if (inf->pid != inferior_pid) + { + struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get (); + int i; + + if (!dr_mirror->addr_preset) + { + dr_mirror->addr_preset = 1; + + for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++) + i386_linux_dr_set_addr (i, dr_mirror->addr[i]); + } + } + linux_nat_iterate_watchpoint_lwps (i386_linux_dr_set_control_callback, &control); } --- a/gdb/i386-nat.c +++ b/gdb/i386-nat.c @@ -248,6 +248,7 @@ i386_inferior_data_get (void) /* Forked processes get a copy of the debug registers. */ memcpy (&detached_inf_data_local, inf_data, sizeof (detached_inf_data_local)); + detached_inf_data_local.dr_mirror.addr_preset = 0; } return &detached_inf_data_local; @@ -276,6 +277,7 @@ i386_cleanup_dregs (void) dr_mirror->addr[i] = 0; dr_mirror->ref_count[i] = 0; } + dr_mirror->addr_preset = 0; dr_mirror->control = 0; dr_mirror->status = 0; } --- a/gdb/i386-nat.h +++ b/gdb/i386-nat.h @@ -89,6 +89,9 @@ struct i386_dr_mirror control registers separated because they don't hold addresses. */ CORE_ADDR addr[DR_NADDR]; + /* All the ADDR hardware registers have been written at least once. */ + unsigned addr_preset : 1; + /* Reference counts for each debug register. */ int ref_count[DR_NADDR];