Re: [patch 1/2] Fix watchpoints across fork

[Pedro Alves <alves dot ped at gmail dot com>]

On 01/02/2012 04:45 PM, Jan Kratochvil wrote:

> There is an issue that formerly linux-nat.c arch backends had ptid_t as their
> LWP parameter.  It has been changed by Pedro to `struct lwp_info *' recently
> but there does not exist any struct lwp_info for the LWPs being detached.  The
> patch creates struct lwp_info temporarily for the detach while keeping the
> current inferior intact (that lwp_info has then PID non-matching current
> inferior).
>
> I do not think it is related to the gdbserver protocol in any way, with
> "set detach-on-fork on" the "set follow-fork-mode" setting is solely target
> specific.

At some point, the remote protocol will need to learn about
following forks and maybe the follow-fork modes, but we don't have
to worry about it for this patch.

> -/* Callback for iterate_over_lwps.  Update the debug registers of
> -   LWP.  */
> +/* Callback for linux_nat_iterate_watchpoint_lwps.  Update the debug registers
> +   of LWP.  */
>
>   static int
>   update_debug_registers_callback (struct lwp_info *lwp, void *arg)
> @@ -364,9 +364,7 @@ update_debug_registers_callback (struct lwp_info *lwp, void *arg)
>   static void
>   amd64_linux_dr_set_control (unsigned long control)
>   {
> -  ptid_t pid_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
> -
> -  iterate_over_lwps (pid_ptid, update_debug_registers_callback, NULL);
> +  linux_nat_iterate_watchpoint_lwps (update_debug_registers_callback, NULL);

This already iterated over threads of the current inferior only.
linux_nat_iterate_watchpoint_lwps does not do that, but leaves it to the
next patch instead.  It'd be better to move that hunk into this patch
already.

>   }
>
>   /* Set address REGNUM (zero based) to ADDR in all LWPs of the current
> @@ -379,7 +377,7 @@ amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr)
>
>     gdb_assert (regnum>= 0&&  regnum<= DR_LASTADDR - DR_FIRSTADDR);
>
> -  iterate_over_lwps (pid_ptid, update_debug_registers_callback, NULL);
> +  linux_nat_iterate_watchpoint_lwps (update_debug_registers_callback, NULL);

I guess this leaves a stale `pid_ptid' local behind.


>   	}
>         else
>   	{
> @@ -671,6 +720,9 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	  save_current_program_space ();
>
>   	  inferior_ptid = ptid_build (child_pid, child_pid, 0);
> +	  reinit_frame_cache ();
> +	  registers_changed ();

Only registers_changed is necessary.  That always invalidates the
frame cache of inferior_ptid.

> +
>   	  add_thread (inferior_ptid);
>   	  child_lp = add_lwp (inferior_ptid);
>   	  child_lp->stopped = 1;
> @@ -862,6 +914,9 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	 informing the solib layer about this new process.  */
>
>         inferior_ptid = ptid_build (child_pid, child_pid, 0);
> +      reinit_frame_cache ();
> +      registers_changed ();

Likewise.

But, I'm not sure I understand why are these calls are necessary.  We
keep a  register cache list, indexed by ptid nowadays.  Could this
be a latent bug elsewhere?

> +
>         add_thread (inferior_ptid);
>         child_lp = add_lwp (inferior_ptid);
>         child_lp->stopped = 1;
> @@ -1112,21 +1167,6 @@ purge_lwp_list (int pid)
>       }
>   }


> +test parent FOLLOW_PARENT
> +
> +# Only GNU/Linux is known to support `set follow-fork-mode child'.

A [target_info exists gdb,no_hardware_watchpoints] check appears
missing, either to skip to test, or call "set can-use-hw-watchpoints
0".  Similarly, [skip_hw_breakpoint_tests] for the hardware breakpoints
bits.

Otherwise looks okay.

Took me a bit to convince myself i386_inferior_data_get was okay.
I wish it was cleaner (could be if we had a separate struct 
process_info, which will end up with, if/when we merge
gdb+gdbserver), but this will do for now.

--
Pedro Alves