Re: [PATCH] [native x86 GNU/Linux] Access debug register mirror from the corresponding inferior.

Jan Kratochvil
2013-02-12T16:50:49Z

On Tue, 12 Feb 2013 13:35:59 +0100, Pedro Alves wrote:
> On 02/11/2013 09:09 PM, Pedro Alves wrote:
> > In a nutshell, we decouple the watchpoints code from inferiors, making
> > it track target processes instead.

It looks really cleaner than waht it was.

I haven't found there any real bug.

[...]
> --- a/gdb/i386-nat.c
> +++ b/gdb/i386-nat.c
> @@ -153,104 +153,102 @@ struct i386_dr_low_type i386_dr_low;
> /* A macro to loop over all debug registers. */
> #define ALL_DEBUG_REGISTERS(i) for (i = 0; i < DR_NADDR; i++)
>
> -/* Clear the reference counts and forget everything we knew about the
> - debug registers. */
> +/* Per-process data. We don't bind this to a per-inferior registry
> + because of targets like x86 GNU/Linux that need to keep track of
> + processes that aren't bound to any inferior (e.g., fork children,
> + checkpoints). */
>
> -static void
> -i386_init_dregs (struct i386_debug_reg_state *state)
> +struct i386_process_info
> {
> - int i;
> -
> - ALL_DEBUG_REGISTERS (i)
> - {
> - state->dr_mirror[i] = 0;
> - state->dr_ref_count[i] = 0;
> - }
> - state->dr_control_mirror = 0;
> - state->dr_status_mirror = 0;
> -}
> + /* Linked list. */
> + struct i386_process_info *next;
>
> -/* Per-inferior data key. */
> -static const struct inferior_data *i386_inferior_data;
> + /* The process identifier. */
> + int pid;

style: Here and on many other places should be pid_t.

>
> -/* Per-inferior data. */
> -struct i386_inferior_data
> -{
> - /* Copy of i386 hardware debug registers for performance reasons. */
> + /* Copy of i386 hardware debug registers. */
> struct i386_debug_reg_state state;
> };
>
> -/* Per-inferior hook for register_inferior_data_with_cleanup. */
> +struct i386_process_info *i386_process_list = NULL;

Missing static.

>
> -static void
> -i386_inferior_data_cleanup (struct inferior *inf, void *arg)
> +/* Find process data for process PID. */
> +
> +static struct i386_process_info *
> +i386_find_process_pid (int pid)
> {
> - struct i386_inferior_data *inf_data = arg;
> + struct i386_process_info *inf;

style: The 'inf' name is confusing when it is not inferior.

> +
> + for (inf = i386_process_list; inf; inf = inf->next)
> + if (inf->pid == pid)
> + return inf;
>
> - xfree (inf_data);
> + return NULL;
> }
>
[...]
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -184,6 +184,13 @@ static struct target_ops linux_ops_saved;
> /* The method to call, if any, when a new thread is attached. */
> static void (*linux_nat_new_thread) (struct lwp_info *);
>
> +/* The method to call, if any, when a new fork is attached. */
> +static void (*linux_nat_new_fork) (struct lwp_info *, int);

style: The parameters could be named.

> +
> +/* The method to call, if any, when a process is no longer
> + attached. */
> +static void (*linux_nat_forget_process_hook) (int);

style: The parameter could be named.

> +
> /* Hook to call prior to resuming a thread. */
> static void (*linux_nat_prepare_to_resume) (struct lwp_info *);
>
[...]
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
[...]
> @@ -176,6 +170,18 @@ void linux_nat_add_target (struct target_ops *);
> /* Register a method to call whenever a new thread is attached. */
> void linux_nat_set_new_thread (struct target_ops *, void (*) (struct lwp_info *));
>
> +/* Register a method to call whenever a new fork is attached. */
> +void linux_nat_set_new_fork (struct target_ops *,
> + void (*) (struct lwp_info *, int));
> +
> +/* Register a method to call whenever a process is killed or
> + detached. */
> +void linux_nat_set_forget_process (struct target_ops *, void (*) (int));

style: Parameters could be named, I do not understand why to omit their names.

> +
> +/* Call the method registered with the function above. PID is the
> + process to forget about. */
> +void linux_nat_forget_process (int pid);
> +
> /* Register a method that converts a siginfo object between the layout
> that ptrace returns, and the layout in the architecture of the
> inferior. */
[...]

Thanks,
Jan