From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Uza+M7PISWp/PCgAWB0awg (envelope-from ) for ; Sat, 04 Jul 2026 23:00:03 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LS/BiXAB; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id B826E1E070; Sat, 04 Jul 2026 23:00:03 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_SBL_CSS autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 7DB431E070 for ; Sat, 04 Jul 2026 23:00:01 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5B5F24BAD167 for ; Sun, 5 Jul 2026 03:00:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5B5F24BAD167 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LS/BiXAB Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 74EF14BAD148 for ; Sun, 5 Jul 2026 02:59:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 74EF14BAD148 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 74EF14BAD148 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1783220372; cv=none; b=af9opMj3v+0F+TdHTUPPhEbUPVDwFYJeXIgv4FTvynr6cGQJeRsb7oO1EWbxvu1OmqW01mflZC5yCgE1w1p9tYnWyEfGuLHe6DJLArDoKguGrlAG4WLzDQ3Zw3eEiSBjxBc7A0tZJBtv27vHqkeEJ4hY+GBlRQXiRmq1trmyBG8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1783220372; c=relaxed/simple; bh=Ahg9rawTAthOcXN2qCPSx7iXT++KKKtsYFSMIViSxNo=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=tTrb0XOsAoIBsOEsbhMW+Sr0tND1/HTHcV8MX3Jp9IsVobnJeLbWPS5uM6RzsZZPcHZTB7a1P5hu+PT1pVIOgnGU4KZ7VZ8JmpO+6IZ1Z4oh/AyUWYgdoczGgslxmx9NURf8yb3XAQ7v1VGuWAMyJqJ1PTtx0EnWOHkPakZ47GE= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LS/BiXAB DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 74EF14BAD148 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1783220372; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pcpFYfV++D30SqIIdOnI0C4idzoiH6fwd8sA/vx1rVE=; b=LS/BiXABEjI/6SYeZXvnVaTspVVEt6VwXtcXVeN+03Z5M1cDQLwnGyeQRhzaXbsTMyUUfv FZpUkXS7pOnYD2Q9XI3yfJQhHZ7VxL2YDtUpwOhqR3yKo3lAcSlBuWbESKhX3DArYDJcUQ xTA8MfvbytDH95NBoMf8uwZdCMpMThw= Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-134-rlA9oki1NyqDFMYOASBM5w-1; Sat, 04 Jul 2026 22:59:28 -0400 X-MC-Unique: rlA9oki1NyqDFMYOASBM5w-1 X-Mimecast-MFC-AGG-ID: rlA9oki1NyqDFMYOASBM5w_1783220368 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 7873F195605A for ; Sun, 5 Jul 2026 02:59:27 +0000 (UTC) Received: from f44-mesa-1 (unknown [10.22.88.58]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id BA7A830002DD; Sun, 5 Jul 2026 02:59:26 +0000 (UTC) Date: Sat, 4 Jul 2026 19:59:24 -0700 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Alexandra =?ISO-8859-1?Q?H=E1jkov=E1?= Subject: Re: [PATCH v7] gdb: Add source-tracking breakpoints feature Message-ID: <20260704195924.7e97257d@f44-mesa-1> In-Reply-To: <20260613201437.18964-1-ahajkova@redhat.com> References: <20260613201437.18964-1-ahajkova@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: YYR6ScUhkHIoVvRvVneCEf0GutJDSwA1_XdrS0Imdw0_1783220368 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org 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 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 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 *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 &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_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_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 > +#include > #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; > using bp_location_iterator = bp_location_list::iterator; > using bp_location_range = iterator_range; > > +/* 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 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 > 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 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 &expanded); > + > /* Find the SaL locations corresponding to the given LOCATION. > On return, FOUND will be 1 if any SaL was found, zero otherwise. */