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

[Kevin Buettner <kevinb@redhat.com>]

Hi Sasha,

On Tue,  5 May 2026 10:29:03 +0200
Alexandra Hájková <ahajkova@redhat.com> wrote:

> The breakpoint_source structure stores captured source code lines

Instead of jumping right into describing the implementation, I'd
like to see a high level description of the problem that this new
feature is intended to solve.

> around a breakpoint location, along with a reference to the BFD
> that was current when the source was captured.

[...]

> +/* 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)
> +{
> +  /* The index into BP_SOURCE's lines where the breakpoint was placed.
> */
> +  int bp_stored = bp_source->bp_line_stored;
> +  size_t bp_size = bp_source->source_lines.size ();
> +
> +  /* An empty string, used if the breakpoint line is at the start or end
> of
> +     the context window.  */
> +  static std::string empty_string ("");
> +
> +  /* The lines immediately before and after the breakpoint in BP_SOURCE.
> +     If the breakpoint is the first or last line in BP_SOURCE then use
> +     EMPTY_STRING as a stand in.  */
> +  const std::string &bp_prev =
> +    (bp_stored == 0
> +     ? empty_string : bp_source->source_lines[bp_stored - 1]);
> +  const std::string &bp_next =
> +    ((bp_stored + 1) == bp_size

This is a comparison between a signed and unsigned value.  Maybe change
the type of bp_stored to size_t?  (That assignment above will likely
require a cast.)

[...]

> +  /* The updated breakpoint location has not bee found in TMP_SOURCE.  */

Typo: s/bee/been/

> +  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;
> +
> +  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 ();

Instead of using rfind and this rather circuitous approach to making a
new locspec, why not clone the existing locspec and then update its
offset?  (Or you might be able to change it directly, but I'm not
entirely sure that's safe.)

Also, if you do end up keeping that code above, you should probably
use the language specified by the existing breakpoint instead of
the global 'current_language'.

> +
> +  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);
> +    }

What happens here when !found is true?  I expected to see some code
here to handle this case.  If it turns out that !found can't happen,
perhaps use an assert to indicate this.  If it's safe for !found to
fall through (which I don't think it is - expanded would be empty and
update_breakpoint_locations would silently remove all breakpoint
locations), then add a comment here indicating that fact.

> +
> +  bp_source = std::make_unique<breakpoint_source>
> +    (breakpoint_source_capture (expanded, BREAKPOINT_SRC_CTX_LINES));
> +}
> +
[...]

Kevin