Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping)

[Christian Biesinger <cbiesinger@google.com>]

On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sun, 19 Apr 2020 18:25:55 +0100
> Subject: [PATCH] Stop considering hw and sw breakpoints duplicates
>
> ---
>  gdb/breakpoint.c                                   | 269 ++++++++++++++-------
>  gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |   2 +-
>  2 files changed, 183 insertions(+), 88 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index e49025461ba..27799e89807 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *,
>  static int watchpoint_locations_match (struct bp_location *loc1,
>                                        struct bp_location *loc2);
>
> +static int breakpoint_locations_match (struct bp_location *loc1,
> +                                      struct bp_location *loc2,
> +                                      bool sw_hw_bps_match = false);
> +

Should this return a bool?

>  static int breakpoint_location_address_match (struct bp_location *bl,
>                                               const struct address_space *aspace,
>                                               CORE_ADDR addr);

And this, I guess.

> @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl)
>      return;
>
>    /* Do a first pass to check for locations with no assigned
> -     conditions or conditions that fail to parse to a valid agent expression
> -     bytecode.  If any of these happen, then it's no use to send conditions
> -     to the target since this location will always trigger and generate a
> -     response back to GDB.  */
> +     conditions or conditions that fail to parse to a valid agent
> +     expression bytecode.  If any of these happen, then it's no use to
> +     send conditions to the target since this location will always
> +     trigger and generate a response back to GDB.  Note we consider
> +     all locations at the same address irrespective of type, i.e.,
> +     even if the locations aren't considered duplicates (e.g.,
> +     software breakpoint and hardware breakpoint at the same
> +     address).  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl)
>         }
>      }
>
> -  /* No NULL conditions or failed bytecode generation.  Build a condition list
> -     for this location's address.  */
> +  /* No NULL conditions or failed bytecode generation.  Build a
> +     condition list for this location's address.  If we have software
> +     and hardware locations at the same address, they aren't
> +     considered duplicates, but we still marge all the conditions
> +     anyway, as it's simpler, and doesn't really make a practical
> +     difference.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl)
>    if (dprintf_style != dprintf_style_agent)
>      return;
>
> -  /* For now, if we have any duplicate location that isn't a dprintf,
> -     don't install the target-side commands, as that would make the
> -     breakpoint not be reported to the core, and we'd lose
> +  /* For now, if we have any location at the same address that isn't a
> +     dprintf, don't install the target-side commands, as that would
> +     make the breakpoint not be reported to the core, and we'd lose
>       control.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
> @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl)
>         }
>      }
>
> -  /* No NULL commands or failed bytecode generation.  Build a command list
> -     for this location's address.  */
> +  /* No NULL commands or failed bytecode generation.  Build a command
> +     list for all duplicate locations at this location's address.
> +     Note that here we must care for whether the breakpoint location
> +     types are considered duplicates, otherwise, say, if we have a
> +     software and hardware location at the same address, the target
> +     could end up running the commands twice.  For the moment, we only
> +     support targets-side commands with dprintf, but it doesn't hurt
> +     to be pedantically correct in case that changes.  */
>    ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
>      {
>        loc = (*loc2p);
> -      if (loc->owner->extra_string
> +      if (breakpoint_locations_match (bl, loc)
> +         && loc->owner->extra_string
>           && is_breakpoint (loc->owner)
>           && loc->pspace->num == bl->pspace->num
>           && loc->owner->enable_state == bp_enabled
> @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl,
>    if (bl->loc_type == bp_loc_software_breakpoint
>        || bl->loc_type == bp_loc_hardware_breakpoint)
>      {
> -      if (bl->owner->type != bp_hardware_breakpoint)
> -       {
> -         /* If the explicitly specified breakpoint type
> -            is not hardware breakpoint, check the memory map to see
> -            if the breakpoint address is in read only memory or not.
> -
> -            Two important cases are:
> -            - location type is not hardware breakpoint, memory
> -            is readonly.  We change the type of the location to
> -            hardware breakpoint.
> -            - location type is hardware breakpoint, memory is
> -            read-write.  This means we've previously made the
> -            location hardware one, but then the memory map changed,
> -            so we undo.
> -
> -            When breakpoints are removed, remove_breakpoints will use
> -            location types we've just set here, the only possible
> -            problem is that memory map has changed during running
> -            program, but it's not going to work anyway with current
> -            gdb.  */
> -         struct mem_region *mr
> -           = lookup_mem_region (bl->target_info.reqstd_address);
> -
> -         if (mr)
> -           {
> -             if (automatic_hardware_breakpoints)
> -               {
> -                 enum bp_loc_type new_type;
> -
> -                 if (mr->attrib.mode != MEM_RW)
> -                   new_type = bp_loc_hardware_breakpoint;
> -                 else
> -                   new_type = bp_loc_software_breakpoint;
> -
> -                 if (new_type != bl->loc_type)
> -                   {
> -                     static int said = 0;
> -
> -                     bl->loc_type = new_type;
> -                     if (!said)
> -                       {
> -                         fprintf_filtered (gdb_stdout,
> -                                           _("Note: automatically using "
> -                                             "hardware breakpoints for "
> -                                             "read-only addresses.\n"));
> -                         said = 1;
> -                       }
> -                   }
> -               }
> -             else if (bl->loc_type == bp_loc_software_breakpoint
> -                      && mr->attrib.mode != MEM_RW)
> -               {
> -                 fprintf_unfiltered (tmp_error_stream,
> -                                     _("Cannot insert breakpoint %d.\n"
> -                                       "Cannot set software breakpoint "
> -                                       "at read-only address %s\n"),
> -                                     bl->owner->number,
> -                                     paddress (bl->gdbarch, bl->address));
> -                 return 1;
> -               }
> -           }
> -       }
> -
>        /* First check to see if we have to handle an overlay.  */
>        if (overlay_debugging == ovly_off
>           || bl->section == NULL
> @@ -2830,7 +2786,11 @@ insert_breakpoints (void)
>
>    /* Updating watchpoints creates new locations, so update the global
>       location list.  Explicitly tell ugll to insert locations and
> -     ignore breakpoints_always_inserted_mode.  */
> +     ignore breakpoints_always_inserted_mode.  Also,
> +     update_global_location_list tries to "upgrade" software
> +     breakpoints to hardware breakpoints to handle "set breakpoint
> +     auto-hw", so we need to call it even if we don't have new
> +     locations.  */
>    update_global_location_list (UGLL_INSERT);
>  }
>
> @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1,
>
>  /* Assuming LOC1 and LOC2's types' have meaningful target addresses
>     (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
> -   the same location.  */
> +   the same location.  If SW_HW_BPS_MATCH is true, then software
> +   breakpoint locations and hardware breakpoint locations match,
> +   otherwise they don't.  */
>
>  static int
> -breakpoint_locations_match (struct bp_location *loc1,
> -                           struct bp_location *loc2)
> +breakpoint_locations_match (struct bp_location *loc1,
> +                           struct bp_location *loc2,
> +                           bool sw_hw_bps_match)
>  {
>    int hw_point1, hw_point2;
>
> @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1,
>    else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
>      return tracepoint_locations_match (loc1, loc2);
>    else
> -    /* We compare bp_location.length in order to cover ranged breakpoints.  */
> +    /* We compare bp_location.length in order to cover ranged
> +       breakpoints.  Keep this in sync with
> +       bp_location_is_less_than.  */
>      return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
>                                      loc2->pspace->aspace, loc2->address)
> +           && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
>             && loc1->length == loc2->length);
>  }
>
> @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b)
>  }
>
>
> +/* Return the location number of LOC.  */
> +
> +static int
> +bp_location_number (bp_location *loc)
> +{
> +  int n = 1;
> +  for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next)
> +    {
> +      if (l == loc)
> +       return n;
> +      ++n;
> +    }
> +
> +  gdb_assert_not_reached ("breakpoint location not found in owner breakpoint");
> +}
> +
>  static bool bp_loc_is_permanent (struct bp_location *loc);
>
> +/* Handle "set breakpoint auto-hw".
> +
> +   If the explicitly specified breakpoint type is not hardware
> +   breakpoint, check the memory map to see whether the breakpoint
> +   address is in read-only memory.
> +
> +   And then, if "set breakpoint auto-hw" is enabled:
> +
> +   - location type is not hardware breakpoint, memory is read-only.
> +   We change the type of the location to hardware breakpoint.
> +
> +   - location type is hardware breakpoint, memory is read-write.  This
> +   means we've previously made the location hardware one, but then the
> +   memory map changed, so we undo.
> +
> +   However, if "set breakpoint auto-hw" is disabled, and memory is
> +   read-only, we warn and disable the breakpoint location.  */
> +
> +static void
> +handle_automatic_hardware_breakpoints (bp_location *bl)
> +{
> +  if (bl->loc_type == bp_loc_software_breakpoint
> +      || bl->loc_type == bp_loc_hardware_breakpoint)
> +    {
> +      if (bl->owner->type != bp_hardware_breakpoint)
> +       {
> +         /* When breakpoints are removed, remove_breakpoints will use
> +            location types we've just set here, the only possible
> +            problem is that memory map has changed during running
> +            program, but it's not going to work anyway with current
> +            gdb.  */
> +         mem_region *mr = lookup_mem_region (bl->address);
> +
> +         if (mr != nullptr)
> +           {
> +             if (automatic_hardware_breakpoints)
> +               {
> +                 enum bp_loc_type new_type;
> +
> +                 if (mr->attrib.mode != MEM_RW)
> +                   new_type = bp_loc_hardware_breakpoint;
> +                 else
> +                   new_type = bp_loc_software_breakpoint;
> +
> +                 if (new_type != bl->loc_type)
> +                   {
> +                     static bool said = false;
> +
> +                     bl->loc_type = new_type;
> +                     if (!said)
> +                       {
> +                         fprintf_filtered (gdb_stdout,
> +                                           _("Note: automatically using "
> +                                             "hardware breakpoints for "
> +                                             "read-only addresses.\n"));
> +                         said = true;
> +                       }
> +                   }
> +               }
> +             else if (bl->loc_type == bp_loc_software_breakpoint
> +                      && mr->attrib.mode != MEM_RW)
> +               {
> +                 bl->enabled = false;
> +
> +                 warning (_("Breakpoint %d.%d disabled.\n"
> +                           "Cannot set software breakpoint "
> +                           "at read-only address %s\n"),
> +                          bl->owner->number, bp_location_number (bl),
> +                          paddress (bl->gdbarch, bl->address));
> +               }
> +           }
> +       }
> +    }
> +}
> +
>  static struct bp_location *
>  add_location_to_breakpoint (struct breakpoint *b,
>                             const struct symtab_and_line *sal)
> @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b)
>    if (a->permanent != b->permanent)
>      return a->permanent > b->permanent;
>
> +  /* Sort by type in order to make duplicate determination easier.
> +     See update_global_location_list.  This is kept in sync with
> +     breakpoint_locations_match.  */
> +  if (a->loc_type < b->loc_type)
> +    return true;
> +
> +  /* Likewise, for range-breakpoints, sort by length.  */
> +  if (a->loc_type == bp_loc_hardware_breakpoint
> +      && b->loc_type == bp_loc_hardware_breakpoint
> +      && a->length < b->length)
> +    return true;
> +
>    /* Make the internal GDB representation stable across GDB runs
>       where A and B memory inside GDB can differ.  Breakpoint locations of
>       the same type at the same address can be sorted in arbitrary order.  */
> @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl)
>        loc->cond_bytecode.reset ();
>      }
>  }
> +
>  /* Called whether new breakpoints are created, or existing breakpoints
>     deleted, to update the global location list and recompute which
>     locations are duplicate of which.
> @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>    ALL_BREAKPOINTS (b)
>      for (loc = b->loc; loc; loc = loc->next)
>        *locp++ = loc;
> +
> +  /* See if we need to "upgrade" a software breakpoint to a hardware
> +     breakpoint.  Do this before deciding whether locations are
> +     duplicates.  Also do this before sorting because sorting order
> +     depends on location type.  */
> +  for (locp = bp_locations;
> +       locp < bp_locations + bp_locations_count;
> +       locp++)
> +    {
> +      loc = *locp;
> +      if (!loc->inserted)
> +       handle_automatic_hardware_breakpoints (loc);
> +    }
> +
>    std::sort (bp_locations, bp_locations + bp_locations_count,
>              bp_location_is_less_than);
>
> @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>                     {
>                       struct bp_location *loc2 = *loc2p;
>
> +                     if (loc2 == old_loc)
> +                       continue;
> +
>                       if (breakpoint_locations_match (loc2, old_loc))
>                         {
>                           /* Read watchpoint locations are switched to
> @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>                           /* loc2 is a duplicated location. We need to check
>                              if it should be inserted in case it will be
>                              unduplicated.  */
> -                         if (loc2 != old_loc
> -                             && unduplicated_should_be_inserted (loc2))
> +                         if (unduplicated_should_be_inserted (loc2))
>                             {
>                               swap_insertion (old_loc, loc2);
>                               keep_in_target = 1;
> @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b,
>             if (have_ambiguous_names)
>               {
>                 for (; l; l = l->next)
> -                 if (breakpoint_locations_match (e, l))
> -                   {
> -                     l->enabled = 0;
> -                     break;
> -                   }
> +                 {
> +                   /* Ignore software vs hardware location type at
> +                      this point, because with "set breakpoint
> +                      auto-hw", after a re-set, locations that were
> +                      hardware can end up as software, or vice versa.
> +                      As mentioned above, this is an heuristic and in
> +                      practice should give the correct answer often
> +                      enough.  */
> +                   if (breakpoint_locations_match (e, l, true))
> +                     {
> +                       l->enabled = 0;
> +                       break;
> +                     }
> +                 }
>               }
>             else
>               {
> diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> index 65f7baae9f5..b950e433ed8 100644
> --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
> @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \
>  # Ensure that inserting a software breakpoint in a known-read-only
>  # region fails.
>  gdb_test "break *$main_lo" \
> -    "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \
> +    "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \
>      "inserting software breakpoint in read-only memory fails"
>
>  delete_breakpoints
>
> base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079
> --
> 2.14.5
>