Re: Problems with hardware watchpoint on ia32.

[Michael Snyder <msnyder at cygnus dot com>]

Eli Zaretskii wrote:
> 
> Okay, to put my money where my mouth is, I'm posting patches to
> correct two problems related to hardware breakpoints and watchpoints.
> These were all found in the DJGPP (a.k.a. go32) version of GDB, but I
> firmly believe that they can be seen on any x86 platform, at least in
> native debugging, and possibly on other platforms as well.
> 
> Each one of the two messages, this and the next, handles a specific
> problem.
> 
> Problem no.1: GDB cannot watch bit fields with hardware watchpoints.

OK, I am checking this one in.  Thanks, good fix, 
nice example etc.  I will be reviewing your next 
HW watchpoint patch next.

				Michael Snyder

> Example:
> 
> $ cat bf.c
> 
> int main (int argc, char *argv[])
> {
>   struct foo {
>     int iv;
>     double dv;
>     unsigned flag1:1;
>     unsigned flag2:2;
>     int jv;
>   } foo_var, *foo_ptr;
> 
>   foo_var.flag1 = 1;
>   foo_var.flag2 = foo_var.flag1 + 1;
>   return 0;
> }
> 
> $ gcc -g -o bf bf.c
> $ gdb bf
> (gdb) b main
> Breakpoint 1 at 0x1573: file bf.c, line 11.
> (gdb) r
> Starting program: g:/gdbsnap/gdb-0222/gdb/bf
> 
> Breakpoint 1, main (argc=1, argv=0x8c6fc) at bf.c:11
> 11      foo_var.flag1 = 1;
> (gdb) watch foo_var.flag1
> Watchpoint 2: foo_var.flag1
> 
> Here are the patches, after which you can set hardware watchpoints on
> bit fields.  You will notice that these patches also augment the
> lazy-value trick suggested by Jim Blandy as a means to watch struct
> members; this is because that trick doesn't work for bit fields, and
> because I regard it generally fragile as far as watchpoints are
> considered (we depend on the assumption that GDB doesn't evaluate
> parent structures, but nothing in GDB's code suggests that this
> assumption will hold).
> 
> 2000-03-08  Eli Zaretskii  <eliz@is.elta.co.il>
> 
>         * breakpoint.c (insert_breakpoints, remove_breakpoint)
>         (bpstat_stop_status, can_use_hardware_watchpoint): Don't insert,
>         remove, or check status of hardware watchpoints for entire structs
>         and arrays unless the user explicitly asked to watch that struct
>         or array.
>         (insert_breakpoints): Try to insert watchpoints for all the values
>         on the value chain, even if some of them fail to insert.
> 
>         * values.c (value_primitive_field): Set the offset in struct value
>         we return when the field is a packed bitfield.
> 
> --- gdb/breakpoint.c~1  Wed Mar  8 18:16:00 2000
> +++ gdb/breakpoint.c    Wed Mar  8 19:20:28 2000
> @@ -974,24 +974,39 @@ insert_breakpoints ()
>                 if (VALUE_LVAL (v) == lval_memory
>                     && ! VALUE_LAZY (v))
>                   {
> -                   CORE_ADDR addr;
> -                   int len, type;
> +                   struct type *vtype = check_typedef (VALUE_TYPE (v));
> 
> -                   addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> -                   len = TYPE_LENGTH (VALUE_TYPE (v));
> -                   type   = hw_write;
> -                   if (b->type == bp_read_watchpoint)
> -                     type = hw_read;
> -                   else if (b->type == bp_access_watchpoint)
> -                     type = hw_access;
> -
> -                   val = target_insert_watchpoint (addr, len, type);
> -                   if (val == -1)
> +                   /* We only watch structs and arrays if user asked
> +                      for it explicitly, never if they just happen to
> +                      appear in the middle of some value chain.  */
> +                   if (v == b->val_chain
> +                       || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
> +                           && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
>                       {
> -                       b->inserted = 0;
> -                       break;
> +                       CORE_ADDR addr;
> +                       int len, type;
> +
> +                       addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> +                       len = TYPE_LENGTH (VALUE_TYPE (v));
> +                       type   = hw_write;
> +                       if (b->type == bp_read_watchpoint)
> +                         type = hw_read;
> +                       else if (b->type == bp_access_watchpoint)
> +                         type = hw_access;
> +
> +                       val = target_insert_watchpoint (addr, len, type);
> +                       if (val == -1)
> +                         {
> +                           /* Don't exit the loop, try to insert
> +                              every value on the value chain.  That's
> +                              because we will be removing all the
> +                              watches below, and removing a
> +                              watchpoint we didn't insert could have
> +                              adverse effects.  */
> +                           b->inserted = 0;
> +                         }
> +                       val = 0;
>                       }
> -                   val = 0;
>                   }
>               }
>             /* Failure to insert a watchpoint on any memory value in the
> @@ -1327,21 +1342,28 @@ remove_breakpoint (b, is)
>           if (VALUE_LVAL (v) == lval_memory
>               && ! VALUE_LAZY (v))
>             {
> -             CORE_ADDR addr;
> -             int len, type;
> +             struct type *vtype = check_typedef (VALUE_TYPE (v));
> 
> -             addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> -             len = TYPE_LENGTH (VALUE_TYPE (v));
> -             type   = hw_write;
> -             if (b->type == bp_read_watchpoint)
> -               type = hw_read;
> -             else if (b->type == bp_access_watchpoint)
> -               type = hw_access;
> -
> -             val = target_remove_watchpoint (addr, len, type);
> -             if (val == -1)
> -               b->inserted = 1;
> -             val = 0;
> +             if (v == b->val_chain
> +                 || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
> +                     && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
> +               {
> +                 CORE_ADDR addr;
> +                 int len, type;
> +
> +                 addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> +                 len = TYPE_LENGTH (VALUE_TYPE (v));
> +                 type   = hw_write;
> +                 if (b->type == bp_read_watchpoint)
> +                   type = hw_read;
> +                 else if (b->type == bp_access_watchpoint)
> +                   type = hw_access;
> +
> +                 val = target_remove_watchpoint (addr, len, type);
> +                 if (val == -1)
> +                   b->inserted = 1;
> +                 val = 0;
> +               }
>             }
>         }
>        /* Failure to remove any of the hardware watchpoints comes here.  */
> @@ -2571,14 +2593,21 @@ bpstat_stop_status (pc, not_a_breakpoint
>             if (VALUE_LVAL (v) == lval_memory
>                 && ! VALUE_LAZY (v))
>               {
> -               CORE_ADDR vaddr;
> +               struct type *vtype = check_typedef (VALUE_TYPE (v));
> 
> -               vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> -               /* Exact match not required.  Within range is sufficient.
> -                */
> -               if (addr >= vaddr &&
> -                   addr < vaddr + TYPE_LENGTH (VALUE_TYPE (v)))
> -                 found = 1;
> +               if (v == b->val_chain
> +                   || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
> +                       && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
> +                 {
> +                   CORE_ADDR vaddr;
> +
> +                   vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> +                   /* Exact match not required.  Within range is
> +                       sufficient.  */
> +                   if (addr >= vaddr &&
> +                       addr < vaddr + TYPE_LENGTH (VALUE_TYPE (v)))
> +                     found = 1;
> +                 }
>               }
>           }
>         if (found)
> @@ -5515,6 +5544,7 @@ can_use_hardware_watchpoint (v)
>       struct value *v;
>  {
>    int found_memory_cnt = 0;
> +  struct value *head = v;
> 
>    /* Did the user specifically forbid us to use hardware watchpoints? */
>    if (!can_use_hw_watchpoints)
> @@ -5552,13 +5582,23 @@ can_use_hardware_watchpoint (v)
>             {
>               /* Ahh, memory we actually used!  Check if we can cover
>                   it with hardware watchpoints.  */
> -             CORE_ADDR vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> -             int       len   = TYPE_LENGTH (VALUE_TYPE (v));
> +             struct type *vtype = check_typedef (VALUE_TYPE (v));
> 
> -             if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len))
> -               return 0;
> -             else
> -               found_memory_cnt++;
> +             /* We only watch structs and arrays if user asked for it
> +                explicitly, never if they just happen to appear in a
> +                middle of some value chain.  */
> +             if (v == head
> +                 || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
> +                     && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
> +               {
> +                 CORE_ADDR vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> +                 int       len   = TYPE_LENGTH (VALUE_TYPE (v));
> +
> +                 if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len))
> +                   return 0;
> +                 else
> +                   found_memory_cnt++;
> +               }
>             }
>         }
>        else if (v->lval != not_lval && v->modifiable == 0)
> --- gdb/values.c~0      Thu Sep  9 02:20:18 1999
> +++ gdb/values.c        Wed Mar  8 18:53:48 2000
> @@ -802,6 +802,8 @@ value_primitive_field (arg1, offset, fie
>                                                     fieldno));
>        VALUE_BITPOS (v) = TYPE_FIELD_BITPOS (arg_type, fieldno) % 8;
>        VALUE_BITSIZE (v) = TYPE_FIELD_BITSIZE (arg_type, fieldno);
> +      VALUE_OFFSET (v) = VALUE_OFFSET (arg1) + offset
> +       + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
>      }
>    else if (fieldno < TYPE_N_BASECLASSES (arg_type))
>      {