Re: [patch 3/4] Fix hw watchpoints #2: reordered / simultaneously hit

[Joel Brobecker <brobecker at adacore dot com>]

Jan,

> If you hit two watchpoints at the same time in the default all-stop mode GDB
> does not cope well with the pending watchpoint hit in non-current LWP.
> Moreover when you change the watchpoints setup during such stop.

Can you be more specific as to what happens in this case?

> gdb/
> 2009-11-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix reordered watchpoints triggered in other threads during all-stop.
> 	* i386-nat.c (i386_stopped_by_watchpoint): Call
> 	i386_stopped_data_address through the target vector.
> 	* linux-nat.c (save_sigtrap, linux_nat_stopped_data_address): New.
> 	(stop_wait_callback, linux_nat_filter_event): Call save_sigtrap.
> 	(linux_nat_add_target): Install linux_nat_stopped_data_address.
> 	* linux-nat.h (struct lwp_info): New fields watchpoint_hit_set and
> 	watchpoint_hit.

Well, I agree now that this stopped_by_watchpoint/stopped_data_address
can get you confused... It took me a LONG time to understand how the
patch is working...

Some comments below.

> gdb/testsuite/
> 2009-11-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         * gdb.base/watchthreads-reorder.exp, gdb.base/watchthreads-reorder.c:
>         New.

Pre-approved, with just a few minor comments.


> +  /* WATCHPOINT_HIT_SET is non-zero if this LWP stopped with a trap and a data
> +     watchpoint has been found as triggered.  In such case WATCHPOINT_HIT
> +     contains data address of the triggered data watchpoint.  */
> +  unsigned watchpoint_hit_set : 1;

I think we should avoid bitfields unless we can show that they make
a difference in terms of memory usage. I don't think that this structure
is critical.  Do you think that a name like "watchpoint_hit" or
"watchpoint_hit_p" might be simpler?

    int watchpoint_hit;

for instance?

> +  CORE_ADDR watchpoint_hit;

I'm being a little bit on the picky side (again! Sorry...), but I think
it's better to use names that are consistent with the associated target
methods.  We may want to change these names later if we decide to go
ahead with your proposed interface cleanup (merging the two watchpoint
target routines into one), but for now, I would personally prefer:

     CORE_ADDR stopped_data_address;

> +  /* linux_nat_stopped_data_address is not even installed in this case.  */
> +  if (linux_ops->to_stopped_data_address == NULL)
> +    return;

I think the comment is a little obscure and that it should be placed
after the if. Or, if you want to place it before the if, I'd make the
commend conditional.

Suggestions:

  if (linux_ops->to_stopped_data_address == NULL)
    /* This platform does not seem to support watchpoints.  */
    return;

Or:

  /* Nothing to do if this platform does not seem to support watchpoints.  */
  if (linux_ops->to_stopped_data_address == NULL)
    return;

> +/* Wrap target_stopped_data_address where the GNU/Linux native target may be
> +   directed by the watchpoint/debug register number.  Base the reported value
> +   on the triggered data address instead.  During inferior stop the assignment
> +   of watchpoint/debug registers may change making the register number specific
> +   trigger info stale.
> +
> +   See the comment at update_watchpoint for the trigger lifecycle.  */

Part of this comment, I think, would be easier to understand if it was
placed in the description of save_sigtrap(). (the part that explains
that the data might change, etc, and thus you have to save it).
For this function, I'd just stick to a general description, explaining
that you base the results on the info cached in the lwp struct, and
refer them to save_sigtrap.

> +  /* Just prevent compiler `warning: ‘unusedX_rwatch’ defined but not used'.  */

I see some weird codes in that comment...

> +# two watchpoint being hit at the same time, without reordering them during the
         ^^^^^^^^^^ watchpoints

> +    if ![runto_main] {
> +	gdb_suppress_tests
> +    }

Please avoid the use of gdb_suppress_tests.  If you can't run to main,
then there is no point in continuing. Just abort via return.

> +    # Use "rwatch" as "watch" would report the watchpoint changed just based on its
> +    # read memory value during a stop by unrelated event.  We are interested to not
> +    # to lose the hardware watchpoint trigger.

"we are interested in not losing"

-- 
Joel