Re: [patch 4/4] hw watchpoints made multi-inferior

[Yao Qi <yao@codesourcery.com>]

On 12/06/2010 07:14 PM, Jan Kratochvil wrote:

Here are some my cents, although I can't approve or reject this patch. :)

> gdb/
> 2010-12-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix watchpoints for multi-inferior.
> 	* breakpoint.c
> 	(update_watchpoint) <b->pspace != current_program_space>: New.

This changelog entry looks strange.  You can describe what is this
change here, like,

	* breakpoint.c (update_watchpoint): Return directly if
breakpoint's pspace is not current_program_space.

> 	* linux-nat.c (linux_nat_iterate_watchpoint_lwps): Fix
> 	iterate_over_lwps FILTER.

What is FILTER?  Why capitalized?

> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1302,6 +1302,9 @@ update_watchpoint (struct breakpoint *b, int reparse)
>    if (!watchpoint_in_thread_scope (b))
>      return;
>  
> +  if (b->pspace != current_program_space)
> +    return;
> +
>    /* We don't free locations.  They are stored in bp_location array and
>       update_global_locations will eventually delete them and remove
>       breakpoints if needed.  */
> @@ -1879,6 +1882,7 @@ insert_breakpoint_locations (void)
>    int val = 0;
>    int disabled_breaks = 0;
>    int hw_breakpoint_error = 0;
> +  struct program_space *saved_current_program_space = current_program_space;
>  
>    struct ui_file *tmp_error_stream = mem_fileopen ();
>    struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
> @@ -1908,7 +1912,8 @@ insert_breakpoint_locations (void)
>  	 if we aren't attached to any process yet, we should still
>  	 insert breakpoints.  */
>        if (!gdbarch_has_global_breakpoints (target_gdbarch)
> -	  && ptid_equal (inferior_ptid, null_ptid))
> +	  && (ptid_equal (inferior_ptid, null_ptid)
> +	      || b->pspace != saved_current_program_space))
>  	continue;

Please add some comments here for this change.  It is not that obvious
to understand without multi-inferior context.

>        val = insert_bp_location (b, tmp_error_stream,
> @@ -8265,6 +8270,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
>    b = set_raw_breakpoint_without_location (NULL, bp_type);
>    set_breakpoint_number (internal, b);
>    b->thread = thread;
> +  b->pspace = current_program_space;
>    b->disposition = disp_donttouch;
>    b->exp = exp;
>    b->exp_valid_block = exp_valid_block;
> @@ -9404,6 +9410,9 @@ update_global_location_list (int should_insert)
>        int keep_in_target = 0;
>        int removed = 0;
>  
> +      if (old_loc->pspace != current_program_space)
> +	continue;

code indent problem?  Put extra three spaces in front of "continue".

> +static void
> +i386_inferior_data_cleanup (struct inferior *inf, void *arg)
> +{
> +  struct i386_inferior_data *inf_data = arg;
> +
> +  xfree (inf_data);
> +}
> +

Can't we 'xfree (arg);' directly?

> @@ -1287,7 +1287,7 @@ linux_nat_iterate_watchpoint_lwps
>    if (inf->pid == inferior_pid)
>      {
>        /* Standard mode.  */
> -      iterate_over_lwps (minus_one_ptid,
> +      iterate_over_lwps (pid_to_ptid (inferior_pid),
>  			 iterate_watchpoint_lwps_callback, &data);

Please add some comments to explain this change "minus_one_ptid ->
pid_to_ptid (inferior_pid)".  This change is mentioned in changelog as
"Fix iterate_over_lwps FILTER", but it is still no enough, IMO.

> +
> +gdb_test "add-inferior" "Added inferior 2"
> +gdb_test "inferior 2" "witching to inferior 2 .*"
> +gdb_load $binfile
> +
> +if ![runto_main] {

Please add `perror "Couldn't run ${testfile}"'

> +    return
> +}

-- 
Yao (齐尧)