Re: [PATCH v3] gdb: Add source-tracking breakpoints feature

[Andrew Burgess <aburgess@redhat.com>]

Alexandra Hájková <ahajkova@redhat.com> writes:

> The breakpoint_source structure stores captured source code lines
> around a breakpoint location, along with a reference to the BFD
> that was current when the source was captured.
>
> When source tracking is enabled (via 'set breakpoint source-tracking
> enabled on'), GDB captures 3 lines of source context
> (BREAKPOINT_SRC_CTX_LINES) along with the current BFD when a
> breakpoint is first set. On executable reload (detected by comparing
> BFDs), it searches within a 12-line window
> (BREAKPOINT_SRC_CTX_LINES * BREAKPOINT_SRC_SEARCH_MULTIPLIER) for
> the best match and adjusts the breakpoint location if needed.
>
> Tests added:
>   gdb.base/adjust_breakpoint.exp
>   gdb.base/adjust_breakpoint-missing-source.exp
>   gdb.base/source-tracking-inline.exp
>
> adjust_breakpoint.exp covers four scenarios:
>   - adjust the breakpoint when lines are deleted
>   - adjust the breakpoint when lines are inserted
>   - the tracked line disappears entirely
>   - verify the tracking can be disabled
>
> adjust_breakpoint-missing-source.exp covers the edge case where source
> files are unavailable, verifying GDB falls back to non-tracking breakpoints.
>
> source-tracking-inline.exp covers source tracking with inline functions.
>
> Add maintenance command to print tracked source code
> Add documentation for the new source-tracking breakpoints feature.
>
> Limitations of the current implementation:
>
> Source tracking is not enabled for pending breakpoints that become
> non-pending.  When a breakpoint is created pending (e.g. with 'set
> breakpoint pending on'), source context is not captured at creation
> time since no symtab is available yet.  When the breakpoint later
> resolves to a location, re_set_default() only updates existing tracked
> breakpoints and does not initiate tracking for newly resolved ones.
> This could be fixed in the future by initiating source tracking in
> re_set_default() when a breakpoint transitions from pending to
> non-pending.
>
> Source tracking for ranged breakpoints is not currently supported.
> Ranged breakpoints have a start and end location spec, and tracking
> both independently raises questions about whether to preserve the
> range length or track each end separately.  For now, ranged
> breakpoints will never be source-tracked.



> @@ -13201,6 +13418,157 @@ code_breakpoint::location_spec_to_sals (location_spec *locspec,
>    return sals;
>  }
>  
> +/* Match BREAKPOINT_SRC_CTX_LINES lines of the initially stored source in a
> +   BREAKPOINT_SRC_CTX_LINES * BREAKPOINT_SRC_SEARCH_MULTIPLIER lines current
> +   source window.
> +   Returns new breakpoint line on success or -1 on failure.  */
> +static int
> +sliding_window_match (breakpoint_source *bp_source,
> +		      breakpoint_source *tmp_source)
> +{
> +  int bp_stored = bp_source->bp_line_stored;
> +  int tmp_size = (int) tmp_source->source_lines.size ();
> +  int bp_size = (int) bp_source->source_lines.size ();
> +
> +  for (int i = 0; i < tmp_size; i++)
> +    {
> +      /* Look for the initial breakpoint line in a stored window.  */
> +      if (bp_source->source_lines[bp_stored] == tmp_source->source_lines[i])
> +	{
> +	  /* Check if the stored lines before and after the breakpoint line
> +	     also match, to reduce false positives.  */
> +	  if (i > 0 && bp_stored > 0
> +	      && (i + 1) < tmp_size
> +	      && (bp_stored + 1) < bp_size)
> +	    {
> +	      if ((bp_source->source_lines[bp_stored - 1] == tmp_source->source_lines[i - 1])
> +		  && (bp_source->source_lines[bp_stored + 1] == tmp_source->source_lines[i + 1]))
> +		{
> +		  return tmp_source->bp_line + i - tmp_source->bp_line_stored;
> +		}
> +	    }
> +	  else if (i == 0 && (i + 1) < tmp_size
> +		   && (bp_stored + 1) < bp_size)
> +	    {
> +	      if (bp_source->source_lines[bp_stored + 1] == tmp_source->source_lines[i + 1])
> +		{
> +		  return tmp_source->bp_line + i - tmp_source->bp_line_stored;
> +		}
> +	    }
> +	  else if ((i + 1) == tmp_size && i > 0
> +		   && bp_stored > 0)
> +	    {
> +	      if (bp_source->source_lines[bp_stored - 1] == tmp_source->source_lines[i - 1])
> +		{
> +		  return tmp_source->bp_line + i - tmp_source->bp_line_stored;
> +		}
> +	    }

I think there might be a case which isn't handled correctly here.  If
bp_stored == 0 indicating that the breakpoint line _was_ the first line
in the file, but i > 0 indicating that the line is no longer the first
line, then I don't think any of these cases will hit.

Also, the 'return' statements don't need the { ... }.

> +	}
> +    }
> +
> +  return -1;
> +}
> +
> +/* See breakpoint.h.  */
> +
> +void
> +code_breakpoint::adjust_bp_for_source_tracking
> +  (program_space *filter_pspace,
> +   std::vector<symtab_and_line> &expanded)
> +{
> +  if (expanded.empty () || expanded[0].symtab == nullptr
> +      || !breakpoint_source_is_tracked (bp_source.get ()))
> +    return;

We don't check `source_tracking_breakpoints` here, so if this feature is
turned on, then a b/p is created with tracking, then the feature is
turned off, the breakpoint will still adjust.

I wonder if when the source-tracking feature is turned off, maybe we
should convert all breakpoints to none source tracked by deleting the
tracking information.  If we did this then we wouldn't need to check the
flag here as we would only see tracked breakpoints if the feature is
currently on.

> +
> +  struct compunit_symtab *cust = expanded[0].symtab->compunit ();
> +  if (cust == nullptr || cust->objfile () == nullptr)
> +    return;
> +
> +  bfd *current_bfd = cust->objfile ()->obfd.get ();
> +  if (bp_source->source_bfd.get () == current_bfd)
> +    return;
> +
> +  /* BFD changed — executable was reloaded.  */
> +  if (expanded.size () != 1)
> +    {
> +      warning (_("Breakpoint %d now has multiple locations after reload, "
> +		 "disabling source tracking."), number);
> +      bp_source.reset ();
> +      return;
> +    }
> +
> +  /* If this fails then the location spec has changed since the
> +     breakpoint's source tracking was initially setup.  */
> +  gdb_assert (breakpoint_locspec_suitable_for_tracking (locspec.get ()));
> +
> +  int bp_stored = bp_source->bp_line_stored;
> +  std::string line;
> +  auto restore_styling = make_scoped_restore (&source_styling, false);
> +  if (!g_source_cache.get_source_lines (expanded[0].symtab,
> +					expanded[0].line,
> +					expanded[0].line, &line))
> +    {
> +      /* Source is unreadable after reload — drop tracking.  */
> +      bp_source.reset ();
> +      return;
> +    }
> +
> +  if (line == bp_source->source_lines[bp_stored])
> +    {
> +      /* Line unchanged — just refresh the capture with the new BFD.  */
> +      bp_source = std::make_unique<breakpoint_source>
> +	(breakpoint_source_capture (expanded, BREAKPOINT_SRC_CTX_LINES));
> +      return;
> +    }
> +
> +  breakpoint_source tmp_source
> +    = breakpoint_source_capture (expanded,
> +				 BREAKPOINT_SRC_CTX_LINES
> +				 * BREAKPOINT_SRC_SEARCH_MULTIPLIER);
> +  int new_bp_line = sliding_window_match (bp_source.get (), &tmp_source);
> +  if (new_bp_line == -1)
> +    {
> +      warning (_("Breakpoint %d source code not found "
> +		 "after reload, keeping original location."), number);
> +      bp_source.reset ();
> +      return;
> +    }
> +
> +  location_spec *spec = locspec.get ();
> +  std::string bp_string (spec->to_string ());
> +  auto pos = bp_string.rfind (':');
> +  if (pos == std::string::npos)
> +    {
> +      warning (_("unable to update location spec for breakpoint %d, "
> +		 "disabling source tracking."), number);
> +      bp_source.reset ();
> +      return;
> +    }
> +
> +  bp_string = bp_string.substr (0, pos + 1);
> +  bp_string.append (std::to_string (new_bp_line));
> +
> +  /* set_string only updates the cached display string, not the parsed
> +     spec_string that location_spec_to_sals actually uses.  Replace the
> +     location spec entirely by re-parsing the updated string so the new
> +     line number takes effect.  */
> +  const char *new_spec_str = bp_string.c_str ();
> +  locspec = string_to_location_spec (&new_spec_str, current_language);
> +  spec = locspec.get ();
> +
> +  int found;
> +  expanded = location_spec_to_sals (spec, filter_pspace, &found);
> +  if (found && new_bp_line != bp_source->bp_line)
> +    {
> +      gdb_printf (_("Breakpoint %d adjusted from line %d to line %d.\n"),
> +		  number, bp_source->bp_line, new_bp_line);
> +      notify_breakpoint_modified (this);
> +    }
> +
> +  bp_source = std::make_unique<breakpoint_source>
> +    (breakpoint_source_capture (expanded, BREAKPOINT_SRC_CTX_LINES));
> +}
> +
>  /* The default re_set method, for typical hardware or software
>     breakpoints.  Reevaluate the breakpoint and recreate its
>     locations.  */
> @@ -13231,6 +13599,9 @@ code_breakpoint::re_set_default (struct program_space *filter_pspace)
>  
>        if (locspec_range_end != nullptr)
>  	{
> +	  /* Ranged breakpoints are not currently tracked.  */
> +	  gdb_assert (!breakpoint_source_is_tracked (bp_source.get ()));
> +
>  	  std::vector<symtab_and_line> sals_end
>  	    = location_spec_to_sals (locspec_range_end.get (),
>  				     filter_pspace, &found);
> @@ -13239,6 +13610,8 @@ code_breakpoint::re_set_default (struct program_space *filter_pspace)
>  	}
>      }
>  
> +  adjust_bp_for_source_tracking (filter_pspace, expanded);

This can probably move in the preceding `if` block, if we don't pass
through the `if` block then `expanded` will be empty.  This isn't really
a problem as adjust_bp_for_source_tracking has an early return if
`expanded` is empty, but I think it makes more sense to move this call
into the `if` block.

> +
>    /* Update the locations for this breakpoint.  For thread-specific
>       breakpoints this will remove any old locations that are for the wrong
>       program space -- this can happen if the user changes the thread of a


> @@ -82,6 +84,17 @@ enum remove_bp_reason
>     architecture.  */
>  
>  #define	BREAKPOINT_MAX	16
> +
> +/* Number of source lines to capture around a breakpoint for source tracking.
> +   This context is used to match and relocate breakpoints when the executable
> +   is reloaded.  The window is centered on the breakpoint line, capturing
> +   lines both before and after it.  */
> +#define BREAKPOINT_SRC_CTX_LINES 3
> +
> +/* Multiplier for the search window when looking for relocated breakpoints.
> +   We search in (BREAKPOINT_SRC_CTX_LINES * BREAKPOINT_SRC_SEARCH_MULTIPLIER)
> +   lines to find code that may have moved.  */
> +#define BREAKPOINT_SRC_SEARCH_MULTIPLIER 4

These could probably move into breakpoint.c.

Thanks,
Andrew