On 02/11/2013 09:09 PM, Pedro Alves wrote: > On 02/07/2013 04:59 PM, Jan Kratochvil wrote: >> On Thu, 07 Feb 2013 17:33:39 +0100, Pedro Alves wrote: >>> 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 >> set detach-on-fork on >>> 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 tried that in the past and I agree it was not worth it. > > I've now gone the 'target side "struct process_info", thus decoupling from > struct inferior' way. > >> >> >>> --- 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; >>> + } >> >> It is already handled by this code which seems to be skipped by this patch. > > Whoops, completely forgot this is here. > >> >> if (detached_inf_pid != ptid_get_pid (inferior_ptid)) >> { >> /* Reinitialize the local cache if INFERIOR_PTID is >> different from the LWP last detached. >> >> Linux kernel before 2.6.33 commit >> 72f674d203cd230426437cdcf7dd6f681dad8b0d >> will inherit hardware debug registers from parent >> on fork/vfork/clone. Newer Linux kernels create such tasks with >> zeroed debug registers. >> >> GDB will remove all breakpoints (and watchpoints) from the forked >> off process. We also need to reset the debug registers in that >> process to be compatible with the older Linux kernels. >> >> Copy the debug registers mirrors into the new process so that all >> breakpoints and watchpoints can be removed together. The debug >> registers mirror will become zeroed in the end before detaching >> the forked off process. */ >> >> detached_inf_pid = ptid_get_pid (inferior_ptid); >> detached_inf_data_local = *inf_data; >> } >> >> Also it seems incorrect to me to use 'ptid_get_pid (inferior_ptid)' there when >> the detached LWP may not come from the current inferior, it is expected to be >> the PID of the process remaining under control of GDB. > > Right. But prepare_to_resume is called in a lot more cases where we're > not detaching, and we don't switch the current inferior in those either. > I think that way is becoming too fragile. > >> I did not try it but what about temporarily switching current inferior? > > I think I'd rather try a different approach. > > In a nutshell, we decouple the watchpoints code from inferiors, making > it track target processes instead. This way, we can freely keep track > of the watchpoint mirrors for these processes behind the core's > back. Checkpoints also play dirty tricks with swapping the process behind > the inferior, so they get special treatment too in the patch (which just > amounts to calling a new hook). Instead of the old > hack in i386_inferior_data_get, where we returned a copy of the current > inferior's debug registers mirror, as soon as we detect a fork in the target, > we copy the debug register mirror from the parent to the child process. > > I don't have an old kernel handy to test, but I stepped through gdb doing > the watchpoint removal in the fork child in the watchpoint-fork test > seeing that the debug registers end up cleared in the child. > > I didn't find the need for linux_nat_iterate_watchpoint_lwps. If > we use plain iterate_over_lwps instead, what happens is that > when removing watchpoints, that iterate_over_lwps doesn't actually > iterate over anything, since the fork child is not added to the > lwp list until later, at detach time, in linux_child_follow_fork. > And if we don't iterate over that lwp, we don't mark its debug > registers as needing update. But linux_child_follow_fork takes > care of doing that explicitly: > > child_lp = add_lwp (inferior_ptid); > child_lp->stopped = 1; > child_lp->last_resume_kind = resume_stop; > make_cleanup (delete_lwp_cleanup, child_lp); > > /* CHILD_LP has new PID, therefore linux_nat_new_thread is not called for it. > See i386_inferior_data_get for the Linux kernel specifics. > Ensure linux_nat_prepare_to_resume will reset the hardware debug > registers. It is done by the linux_nat_new_thread call, which is > being skipped in add_lwp above for the first lwp of a pid. */ > gdb_assert (num_lwps (GET_PID (child_lp->ptid)) == 1); > if (linux_nat_new_thread != NULL) > linux_nat_new_thread (child_lp); > > if (linux_nat_prepare_to_resume != NULL) > linux_nat_prepare_to_resume (child_lp); > ptrace (PTRACE_DETACH, child_pid, 0, 0); > > so unless I'm missing something (quite possible) it ends up all > the same. But, the !detach-on-fork, and the "follow-fork child" paths > should also call linux_nat_new_thread, and they don't presently. It > seems to me in those cases we're not clearing debug regs correctly > when that's needed? Instead of copying that bit that works around > add_lwp bypassing the linux_nat_new_thread call, I thought it'd > be better to add an add_initial_lwp call to be used in the case we > really need to bypass linux_nat_new_thread, and make > add_lwp always call linux_nat_new_thread. > > WDYT? > > Re-tested on Fedora 17. Here's a slithly updated patch that leverages i386_cleanup_dregs to hopefully take care of the other i386 ports that support watchpoints. I've also pushed this to https://github.com/palves/gdb/commits/debug_regs_state (git://github.com/palves/gdb.git debug_regs_state) for convenience. -- Pedro Alves