From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id m3qNAEYrAGqe1isAWB0awg (envelope-from ) for ; Sun, 10 May 2026 02:52:54 -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=PzJRviGX; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id E38BC1E093; Sun, 10 May 2026 02:52:53 -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.4 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_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED 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 E12EA1E093 for ; Sun, 10 May 2026 02:52:52 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F1A344BA23D0 for ; Sun, 10 May 2026 06:52:51 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F1A344BA23D0 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=PzJRviGX Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 4B9CA4BA2E15 for ; Sun, 10 May 2026 06:52:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4B9CA4BA2E15 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 4B9CA4BA2E15 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1778395945; cv=none; b=tk5MdYwJNe31zBSEHeVy7bsEgPuxAT0S8caZDr0n3dxCpiK1bVByET2lWheuYp9NgFdTdJqvNmBAnY5wfNZFVfZm3SYHdfwZibFL6BAG8zf5INaRUUIQZebwnycTUSZfXp+CjhO4RVa6DjXdfJy7Hdw4/36wemhC9Qz+wYpJiuE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1778395945; c=relaxed/simple; bh=fSOoEmvT+e1hxFHsFKXcgOSDCh2zUtj0d6NEt98qbP0=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=Vk29u4LsGwKfdjROy0HaNkTjcjjuZG1/vCbce6vBGTMr/Bhw5Vl4Z4WH4HFvzrEhQ9z6nh6khTQDXNQv+gjZNGImJcU9yEHBx7V2WHpx+eaeOmbkEJAVv9Q1CU8/eNI2xjPN4XZEs3CHg4e5T/5GLWYJlM1bYh4VWK+IpOlQvoQ= 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=PzJRviGX DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 4B9CA4BA2E15 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778395944; 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=XNKLTg5/Gf71+q3Y24USmIOK5bby6du9KY3NQzEeEkE=; b=PzJRviGX2J30+sI+xjrv/WUGpUczaLT/pcMtOwb1qtMAgJNjt0mEosvMyywMg2CNXjpkS4 PXrgiDsH5mILEZVprAk6pQxXHctCzPnQMCkRKLSaovSxL5D3WTm2QaqOPYzl3JC64CteVl Zad5+fcdaPWshBCO9ePbMAfH1EM4MuE= Received: from mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (ec2-35-165-154-97.us-west-2.compute.amazonaws.com [35.165.154.97]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-691-lfHfHG9FOWqLm54qEK5Cmw-1; Sun, 10 May 2026 02:52:23 -0400 X-MC-Unique: lfHfHG9FOWqLm54qEK5Cmw-1 X-Mimecast-MFC-AGG-ID: lfHfHG9FOWqLm54qEK5Cmw_1778395942 Received: from mx-prod-int-06.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-06.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.93]) (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-06.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 36A4F18005B2 for ; Sun, 10 May 2026 06:52:22 +0000 (UTC) Received: from f42-zbm-amd (unknown [10.22.80.31]) by mx-prod-int-06.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 1D1CC18004A3; Sun, 10 May 2026 06:52:20 +0000 (UTC) Date: Sat, 9 May 2026 23:52:17 -0700 From: Kevin Buettner To: Alexandra =?ISO-8859-1?Q?H=E1jkov=E1?= Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v5] gdb: Add source-tracking breakpoints feature Message-ID: <20260509235217.4b9143c9@f42-zbm-amd> In-Reply-To: <20260505082943.346275-1-ahajkova@redhat.com> References: <20260505082943.346275-1-ahajkova@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.93 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: l6W_Q9xye4tSpSTSJkiwNkLgW8qgMJVvuwsdQU0Y6fU_1778395942 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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, On Tue, 5 May 2026 10:29:03 +0200 Alexandra H=C3=A1jkov=C3=A1 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, > +=09=09 breakpoint_source *tmp_source) > +{ > + /* The index into BP_SOURCE's lines where the breakpoint was placed. > */ > + int bp_stored =3D bp_source->bp_line_stored; > + size_t bp_size =3D 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 =3D > + (bp_stored =3D=3D 0 > + ? empty_string : bp_source->source_lines[bp_stored - 1]); > + const std::string &bp_next =3D > + ((bp_stored + 1) =3D=3D 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 &expanded) > +{ > + if (expanded.empty () || expanded[0].symtab =3D=3D nullptr > + || !breakpoint_source_is_tracked (bp_source.get ())) > + return; > + > + struct compunit_symtab *cust =3D expanded[0].symtab->compunit (); > + if (cust =3D=3D nullptr || cust->objfile () =3D=3D nullptr) > + return; > + > + bfd *current_bfd =3D cust->objfile ()->obfd.get (); > + if (bp_source->source_bfd.get () =3D=3D current_bfd) > + return; > + > + /* BFD changed ___ executable was reloaded. */ > + if (expanded.size () !=3D 1) > + { > + warning (_("Breakpoint %d now has multiple locations after reload, > " > +=09=09 "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 =3D bp_source->bp_line_stored; > + std::string line; > + auto restore_styling =3D make_scoped_restore (&source_styling, false); > + if (!g_source_cache.get_source_lines (expanded[0].symtab, > +=09=09=09=09=09expanded[0].line, > +=09=09=09=09=09expanded[0].line, &line)) > + { > + /* Source is unreadable after reload ___ drop tracking. */ > + bp_source.reset (); > + return; > + } > + > + if (line =3D=3D bp_source->source_lines[bp_stored]) > + { > + /* Line unchanged ___ just refresh the capture with the new BFD. > */ > + bp_source =3D std::make_unique > +=09(breakpoint_source_capture (expanded, BREAKPOINT_SRC_CTX_LINES)); > + return; > + } > + > + breakpoint_source tmp_source > + =3D breakpoint_source_capture (expanded, > +=09=09=09=09 BREAKPOINT_SRC_CTX_LINES > +=09=09=09=09 * BREAKPOINT_SRC_SEARCH_MULTIPLIER); > + int new_bp_line =3D sliding_window_match (bp_source.get (), &tmp_sourc= e); > + if (new_bp_line =3D=3D -1) > + { > + warning (_("Breakpoint %d source code not found " > +=09=09 "after reload, keeping original location."), number); > + bp_source.reset (); > + return; > + } > + > + location_spec *spec =3D locspec.get (); > + std::string bp_string (spec->to_string ()); > + auto pos =3D bp_string.rfind (':'); > + if (pos =3D=3D std::string::npos) > + { > + warning (_("unable to update location spec for breakpoint %d, " > +=09=09 "disabling source tracking."), number); > + bp_source.reset (); > + return; > + } > + > + bp_string =3D 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 =3D bp_string.c_str (); > + locspec =3D string_to_location_spec (&new_spec_str, current_language); > + spec =3D 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 =3D location_spec_to_sals (spec, filter_pspace, &found); > + if (found && new_bp_line !=3D bp_source->bp_line) > + { > + gdb_printf (_("Breakpoint %d adjusted from line %d to line %d.\n")= , > +=09=09 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 =3D std::make_unique > + (breakpoint_source_capture (expanded, BREAKPOINT_SRC_CTX_LINES)); > +} > + [...] Kevin