This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 4/4] hw watchpoints kernel workaround
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.
> > <inf->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 <jan.kratochvil@redhat.com>
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) <addr_preset: New.
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -310,11 +310,49 @@ amd64_linux_dr_set_control_callback (int tid, void *control_voidp)
amd64_linux_dr_set (tid, DR_CONTROL, control);
}
+static void amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr);
+
/* Set DR_CONTROL to ADDR in all LWPs of CURRENT_INFERIOR. */
static void
amd64_linux_dr_set_control (unsigned long control)
{
+ 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 for the fork-ed child description.
+ The i386 counterpart is i386_linux_dr_set_control. */
+ if (inf->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];