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

[Kevin Buettner <kevinb@redhat.com>]

Hi Sasha,

See my review, below.  I've trimmed away much of the patch, leaving
ample context for my remarks.

On Sat, 13 Jun 2026 21:57:12 +0200
Alexandra Hajkova <ahajkova@redhat.com> wrote:

[...]

> +/* Capture source lines around a breakpoint location for source tracking.
> +   Returns a breakpoint_source structure with the captured lines, or an
> +   empty structure if capture fails.  Does not print the lines.  */
> +
> +static breakpoint_source
> +breakpoint_source_capture (gdb::array_view<const symtab_and_line> sals,
> +			   int num_of_lines)
> +{
> +  /* Check if we have a valid symtab - if not, we can't capture source lines.
> +     The symtab can be missing if the executable wasn't compiled with
> +     debugging symbols.  */
> +  if (sals.empty () || sals[0].symtab == nullptr || sals[0].line <= 0)
> +    return {};
> +
> +  breakpoint_source result;
> +  result.bp_line = sals[0].line;
> +
> +  /* Calculate the starting line, centering around the breakpoint line.
> +     Avoid going before line 1.  */
> +  int lines_to_capture = num_of_lines;
> +  int start_line = sals[0].line - (lines_to_capture / 2);
> +  if (start_line < 1)
> +    start_line = 1;
> +
> +  /* Get line offsets to check file bounds.  */
> +  const std::vector<off_t> *offsets;
> +  if (!g_source_cache.get_line_charpos (sals[0].symtab, &offsets))
> +    return {};
> +
> +  /* Adjust number of lines if we'd run past the end of the file.  */
> +  if (start_line + lines_to_capture > (int) offsets->size ())
> +    lines_to_capture = (int) offsets->size () - start_line;

I think you might have a fencepost (off-by-one) error here.  I think
that lines_to_capture should actually be:

       lines_to_capture = (int) offsets->size () - start_line + 1;

The example that my AI helper came up with is:

    Breakpoint is at line 3 of a 3-line file.

    num_of_lines is 3, so start_line = 3 - (3/2) = 2.

    start_line + lines_to_capture = 2 + 3 = 5, which is greater than
    offsets->size() (which is 3).

    So lines_to_capture is clipped to 3 - 2 = 1.

    The loop runs only for j = 0. It captures line start_line + 0 = 2.

    The check if (start_line + j == sals[0].line) becomes if (2 == 3),
    which is false.

    Thus, result.bp_line_stored is never set and remains 0.

Before fixing this, you might write a test case which demonstrates
this failure.  I.e. it should FAIL with the current code, but then
PASS once the off-by-one problems is addressed.

> +
> +  /* Get the BFD from the symtab.  */
> +  if (sals[0].symtab->compunit ()->objfile ())
> +    result.source_bfd = sals[0].symtab->compunit ()->objfile ()->obfd;

Here's the first case of API drift that my AI helper found.
symtab::compunit() now returns a 'compunit_symtab &', a reference,
instead of a pointer.  You should use the dot operator '.' instead
of the arrow operator '->' when calling objfile() on it.  I.e.:

  if (sals[0].symtab->compunit ().objfile ())
    result.source_bfd = sals[0].symtab->compunit ().objfile ()->obfd;

[...]

> +/* 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;

There's been some API drift since you posted this patch: 
symtab::compunit() now returns a 'compunit_symtab &' (reference)
instead of a pointer.  Because it returns a reference, taking its
address and checking for 'nullptr' is dead code and will likely
trigger compiler warnings (e.g., -Waddress). 

You can adjust it like this to use a reference instead:

  struct compunit_symtab &cust = expanded[0].symtab->compunit ();
  if (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 ()));
> +
> +  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_source->bp_line_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;
> +    }
> +
> +  auto *explicit_loc = as_explicit_location_spec (locspec.get ());
> +  location_spec_up new_locspec = explicit_loc->clone ();
> +  auto *new_explicit = as_explicit_location_spec (new_locspec.get ());
> +  new_explicit->line_offset.offset = new_bp_line;
> +  new_explicit->line_offset.sign = LINE_OFFSET_NONE;
> +  new_explicit->set_string ("");   // invalidate the cached display string

Nit: I think that we still generally prefer C-style comments.  That
said, after looking at the GDB C/C++ coding standard, I don't see a
prohibition against C++ style comments.  But, a search of the GDB
sources show very few uses of //, so it seems to me that /* */ is
still preferred.

> +  locspec = std::move (new_locspec);

You are setting locspec here (above)...

> +
> +  location_spec *spec = locspec.get ();
> +
> +  int found;
> +  expanded = location_spec_to_sals (spec, filter_pspace, &found);
> +  if (!found)
> +    {

...but then, here, if the spec isn't found, the locspec member variable
retains the new_locspec setting.  I recommend moving that assignment
to locspec after the end of this block.  (Some other slight adjustments
are needed too.)

> +      warning (_("Breakpoint %d adjusted to line %d but location could not "
> +                 "be resolved; keeping original location."), number, new_bp_line);
> +      bp_source.reset ();
> +      return;
> +    }
> +  if (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));
> +}

[...]

> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 7149211a055..4a7952b042a 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -27,6 +27,7 @@
>  #include "probe.h"
>  #include "location.h"
>  #include <vector>
> +#include <memory>
>  #include "gdbsupport/array-view.h"
>  #include "gdbsupport/filtered-iterator.h"
>  #include "gdbsupport/iterator-range.h"
> @@ -34,6 +35,7 @@
>  #include "gdbsupport/safe-iterator.h"
>  #include "cli/cli-script.h"
>  #include "target/waitstatus.h"
> +#include "gdb_bfd.h"
>  
>  struct block;
>  struct gdbpy_breakpoint_object;
> @@ -615,6 +617,30 @@ using bp_location_list = intrusive_list<bp_location>;
>  using bp_location_iterator = bp_location_list::iterator;
>  using bp_location_range = iterator_range<bp_location_iterator>;
>  
> +/* Captured source code around a breakpoint location, used for
> +   source-tracking breakpoints.  When source tracking is enabled,
> +   this structure stores the original source lines around a breakpoint
> +   so the breakpoint can be automatically adjusted if the source code
> +   changes when the executable is reloaded.  */
> +
> +struct breakpoint_source
> +{
> +  /* The captured source lines as strings.  The number of captured lines
> +     is 'source_lines.size ()'.  */
> +  std::vector<std::string> source_lines;
> +
> +  /* The original line number where the breakpoint was set
> +     in the source file.  */
> +  int bp_line = 0;
> +
> +  /* Index into source_lines vector indicating which line
> +     contains the breakpoint (0-based).  */
> +  size_t bp_line_stored = 0;
> +
> +  /* BFD when source was captured.  */
> +  gdb_bfd_ref_ptr source_bfd;
> +};
> +

Consider whether this struct might be moved to gdb/breakpoint.c.  If
so, you'd replace it with a forward declaration:

  struct breakpoint_source;

With that done, you'd no longer need the #include of gdb_bfd.h, earlier.
That #include would then be moved to breakpoint.c.

>  /* Note that the ->silent field is not currently used by any commands
>     (though the code is in there if it was to be, and set_raw_breakpoint
>     does set it to 0).  I implemented it because I thought it would be
> @@ -863,6 +889,11 @@ struct breakpoint : public intrusive_list_node<breakpoint>
>       find the end of the range.  */
>    location_spec_up locspec_range_end;
>  
> +  /* Captured source code around the breakpoint location, used to
> +     track source around the breakpoint to automatically adjust the breakpoint
> +     when source code changes between recompilations.  */
> +  std::unique_ptr<breakpoint_source> bp_source;
> +
>    /* Architecture we used to set the breakpoint.  */
>    struct gdbarch *gdbarch;
>    /* Language we used to set the breakpoint.  */
> @@ -983,6 +1014,13 @@ struct code_breakpoint : public breakpoint
>    /* Helper method that does the basic work of re_set.  */
>    void re_set_default (program_space *pspace);
>  
> +  /* Helper method for re_set_default.  Checks if the executable was
> +     reloaded and if so, attempts to adjust the breakpoint location
> +     using source tracking.  EXPANDED may be updated if the location
> +     is adjusted.  */
> +  void adjust_bp_for_source_tracking (program_space *filter_pspace,
> +				      std::vector<symtab_and_line> &expanded);
> +
>    /* Find the SaL locations corresponding to the given LOCATION.
>       On return, FOUND will be 1 if any SaL was found, zero otherwise.  */