Message ID | be46fe0bc0e6658400ff50d36290d5c5ccc7e2dd.1675869497.git.aburgess@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 209B5385800A for <patchwork@sourceware.org>; Wed, 8 Feb 2023 15:24:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 209B5385800A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1675869849; bh=9sovpJpV8zayyv1/beWmmmWwJZvGY3ZJt2MJDVVZ0Kw=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=cy0Fm0kfBGw1CfTudYKNVQ+0nAeW3FkmBqSEEBif/vFNJniEwgZstTkoTxM760JHR uFHcNikCVbAiPEG0pLOEtZeWsNFQD/WfzohJz3EyEFYlWC+r3w+hGKf4bcpwW47mBz S8xLSBZh4tjrEjY/bbHI+5QSRABcULyEFPQls6F8= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 1E12B385840D for <gdb-patches@sourceware.org>; Wed, 8 Feb 2023 15:23:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1E12B385840D Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-639-p37Fh17YPMyhBOq_MXJvuA-1; Wed, 08 Feb 2023 10:23:40 -0500 X-MC-Unique: p37Fh17YPMyhBOq_MXJvuA-1 Received: by mail-qt1-f198.google.com with SMTP id i5-20020ac813c5000000b003b86b748aadso10934097qtj.14 for <gdb-patches@sourceware.org>; Wed, 08 Feb 2023 07:23:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9sovpJpV8zayyv1/beWmmmWwJZvGY3ZJt2MJDVVZ0Kw=; b=eodJ78sqFaqwFFDTzQQCSmRgz/V7ANmrYGTouWyVl4e6fk4FVNkO0KHrTAiPlv1jdL 8bCltGlPH5R8MrcJAoq6RDV7enWd6vnkWju7sRmPY0MC5mOMyGno6jzaFmAUBq0oPgub 7IlCa45O9JuqG1kN9t+FqCUSHT7FYBKpjCjZoA2gJQ5A6v0WznlghqTPNkGcaxutFjKV kYpE2/2Us9JRgHiB6WIWVvSxAtY8+PKbcpFWnKQJVHwfhtEX0fncpjgZKiXtpNc0oAhg YZ5QGoUSAy3t8mQz76klMx1qTHJnwCz5QNG3+E8Mde57bCDRpcTgB54JbwTiq5Jw68X1 3IoA== X-Gm-Message-State: AO0yUKWsbp5yxccK54BAxH/BvP8FOUaPYs4hQVnjaUg3NaR2d4ObEA5p ZatZEo4xTjApwUWm6Lzs5LV2HsfjX/8JRmxgwVN/lcsgQhyUT5VHA4drpRJBM16QQx9TJ8pU8rS 1trKbVil6tbGXjmGMLkMshjHt0txOZaFClLItTAIsl1Qvs3OMZjhvnvmFD/l/w/Yyv4g7Crf78Z SDejk= X-Received: by 2002:ac8:7f01:0:b0:3b8:5057:377b with SMTP id f1-20020ac87f01000000b003b85057377bmr13238849qtk.65.1675869819979; Wed, 08 Feb 2023 07:23:39 -0800 (PST) X-Google-Smtp-Source: AK7set8qwCuqgDiZIdi3NU27Y1H85+gIfGdjRfAf9dvqUsiQu57/xlX9wVPEuuW6zTKkyWoEgityfA== X-Received: by 2002:ac8:7f01:0:b0:3b8:5057:377b with SMTP id f1-20020ac87f01000000b003b85057377bmr13238808qtk.65.1675869819663; Wed, 08 Feb 2023 07:23:39 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id f3-20020ac84703000000b003b2d890752dsm11543384qtp.88.2023.02.08.07.23.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Feb 2023 07:23:39 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess <aburgess@redhat.com> Subject: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file Date: Wed, 8 Feb 2023 15:23:30 +0000 Message-Id: <be46fe0bc0e6658400ff50d36290d5c5ccc7e2dd.1675869497.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: <cover.1675869497.git.aburgess@redhat.com> References: <cover.1675869497.git.aburgess@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Andrew Burgess <aburgess@redhat.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Avoid printing global thread-id in CLI command output
|
|
Commit Message
Andrew Burgess
Feb. 8, 2023, 3:23 p.m. UTC
I noticed that breakpoint::print_recreate_thread was printing the global thread-id. This function is used to implement the 'save breakpoints' command, and should be writing out suitable CLI commands for recreating the current breakpoints. The CLI does not use global thread-ids, but instead uses the inferior specific thread-ids, e.g. "2.1". This commit updates breakpoint::print_recreate_thread to use the print_thread_id function, and adds a test for this change. --- gdb/breakpoint.c | 5 +- .../gdb.multi/bp-thread-specific.exp | 46 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-)
Comments
On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote: > breakpoint::print_recreate_thread (struct ui_file *fp) const > { > if (thread != -1) > - gdb_printf (fp, " thread %d", thread); > + { > + struct thread_info *thr = find_thread_global_id (thread); > + gdb_printf (fp, " thread %s", print_thread_id (thr)); print_thread_id only prints the inferior-qualified thread id if there are multiple inferiors. I am wondering whether the save breakpoints file should _always_ end up with inferior-qualified thread ids, so that reloading the saved file works the same if you meanwhile add another inferior. Otherwise, Approved-By: Pedro Alves <pedro@palves.net>
Pedro Alves <pedro@palves.net> writes: > On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote: > >> breakpoint::print_recreate_thread (struct ui_file *fp) const >> { >> if (thread != -1) >> - gdb_printf (fp, " thread %d", thread); >> + { >> + struct thread_info *thr = find_thread_global_id (thread); >> + gdb_printf (fp, " thread %s", print_thread_id (thr)); > > print_thread_id only prints the inferior-qualified thread id if > there are multiple inferiors. I am wondering whether the save breakpoints > file should _always_ end up with inferior-qualified thread ids, so that > reloading the saved file works the same if you meanwhile add another > inferior. As a counter argument; if the user has a single inferior and places breakpoints on a particular thread, we'll have a save like: break foo thread 2 Then if the user sets up two inferiors, they can select which inferior the breakpoints should apply to - source the saves from inferior 2, and the b/p will apply to inferior 2 threads, source from inferior 1, and the b/p will apply to inferior 1 threads. If the user has changed the inferior setup when sourcing the breakpoint save file, I think they have to take some responsibility for knowing what they want ... maybe? If you feel strongly then it's easy enough to print the qualified thread-id, just let me know and I'll get it done. Thanks, Andrew > > Otherwise, > > Approved-By: Pedro Alves <pedro@palves.net>
Hi! On 2023-02-10 7:22 p.m., Andrew Burgess wrote: > Pedro Alves <pedro@palves.net> writes: > >> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote: >> >>> breakpoint::print_recreate_thread (struct ui_file *fp) const >>> { >>> if (thread != -1) >>> - gdb_printf (fp, " thread %d", thread); >>> + { >>> + struct thread_info *thr = find_thread_global_id (thread); >>> + gdb_printf (fp, " thread %s", print_thread_id (thr)); >> >> print_thread_id only prints the inferior-qualified thread id if >> there are multiple inferiors. I am wondering whether the save breakpoints >> file should _always_ end up with inferior-qualified thread ids, so that >> reloading the saved file works the same if you meanwhile add another >> inferior. > > As a counter argument; if the user has a single inferior and places > breakpoints on a particular thread, we'll have a save like: > > break foo thread 2 > > Then if the user sets up two inferiors, they can select which inferior > the breakpoints should apply to - source the saves from inferior 2, and > the b/p will apply to inferior 2 threads, source from inferior 1, and > the b/p will apply to inferior 1 threads. > > If the user has changed the inferior setup when sourcing the breakpoint > save file, I think they have to take some responsibility for knowing > what they want ... maybe? > > If you feel strongly then it's easy enough to print the qualified > thread-id, just let me know and I'll get it done. > My thinking is that internally, the thread is really inferior-qualified. It is just a presentation detail in the CLI that we don't print the inferior when there's only one inferior, for backwards compatibility. That may even change in the future. An MI frontend / GUI may be presenting the qualified ID, for instance. It seems to be that there are two valid approaches: #1 - we consider what the user typed when the breakpoint was created as canonical, and thus we save the breakpoint using the same breakpoint spec string that user typed originally, meaning, if the user typed: "break foo thread 1" then that's what we'd save, even if the user added a second inferior after creating the breakpoint. Of course, it follows then that if the breakpoint is created with "break foo thread 1.1" then that's what we save. So the user would have the option. I'm really not sure whether this is an option that we should be giving users, though. What if the breakpoint was created via Python, or via the MI with --thread ? Then the concept of original "thread" may not even exists, even though we save such a breakpoint too. #2 - we consider that the thread that the breakpoint ended up bound to is what is canonical and thus we always print the qualified id to the file. The approach in your patch is neither of the above -- it prints the qualified or non-qualified thread id depending on a CLI presentation detail, which seems brittle to me. Option #2 seems the simplest to explain, document, and implement, to me, but I could be convinced to go with #1 too. Pedro Alves > Thanks, > Andrew > >> >> Otherwise, >> >> Approved-By: Pedro Alves <pedro@palves.net> >
Pedro Alves <pedro@palves.net> writes: > Hi! > > On 2023-02-10 7:22 p.m., Andrew Burgess wrote: >> Pedro Alves <pedro@palves.net> writes: >> >>> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote: >>> >>>> breakpoint::print_recreate_thread (struct ui_file *fp) const >>>> { >>>> if (thread != -1) >>>> - gdb_printf (fp, " thread %d", thread); >>>> + { >>>> + struct thread_info *thr = find_thread_global_id (thread); >>>> + gdb_printf (fp, " thread %s", print_thread_id (thr)); >>> >>> print_thread_id only prints the inferior-qualified thread id if >>> there are multiple inferiors. I am wondering whether the save breakpoints >>> file should _always_ end up with inferior-qualified thread ids, so that >>> reloading the saved file works the same if you meanwhile add another >>> inferior. >> >> As a counter argument; if the user has a single inferior and places >> breakpoints on a particular thread, we'll have a save like: >> >> break foo thread 2 >> >> Then if the user sets up two inferiors, they can select which inferior >> the breakpoints should apply to - source the saves from inferior 2, and >> the b/p will apply to inferior 2 threads, source from inferior 1, and >> the b/p will apply to inferior 1 threads. >> >> If the user has changed the inferior setup when sourcing the breakpoint >> save file, I think they have to take some responsibility for knowing >> what they want ... maybe? >> >> If you feel strongly then it's easy enough to print the qualified >> thread-id, just let me know and I'll get it done. >> > > My thinking is that internally, the thread is really inferior-qualified. > It is just a presentation detail in the CLI that we don't print the > inferior when there's only one inferior, for backwards compatibility. > That may even change in the future. An MI frontend / GUI may be presenting > the qualified ID, for instance. > > It seems to be that there are two valid approaches: > > #1 - we consider what the user typed when the breakpoint was created as canonical, > and thus we save the breakpoint using the same breakpoint spec string that > user typed originally, meaning, if the user typed: > > "break foo thread 1" > > then that's what we'd save, even if the user added a second > inferior after creating the breakpoint. > > Of course, it follows then that if the breakpoint is created with > > "break foo thread 1.1" > > then that's what we save. So the user would have the option. > > I'm really not sure whether this is an option that we should be giving > users, though. What if the breakpoint was created via Python, or via the > MI with --thread ? Then the concept of original "thread" may not even exists, > even though we save such a breakpoint too. > > #2 - we consider that the thread that the breakpoint ended up bound to is what > is canonical and thus we always print the qualified id to the file. > > The approach in your patch is neither of the above -- it prints the qualified > or non-qualified thread id depending on a CLI presentation detail, which seems > brittle to me. > > Option #2 seems the simplest to explain, document, and implement, to me, > but I could be convinced to go with #1 too. Thanks for the explanation. I've implemented #2 in the patch below, what are your thoughts? Thanks, Andrew --- commit 868a074345bb6d20d9a64470936d699c8a123894 Author: Andrew Burgess <aburgess@redhat.com> Date: Wed Feb 8 13:56:22 2023 +0000 gdb: don't use the global thread-id in the saved breakpoints file I noticed that breakpoint::print_recreate_thread was printing the global thread-id. This function is used to implement the 'save breakpoints' command, and should be writing out suitable CLI commands for recreating the current breakpoints. The CLI does not use global thread-ids, but instead uses the inferior specific thread-ids, e.g. "2.1". After some discussion on the mailing list it was suggested that the most consistent solution would be for the saved breakpoints file to always contain the inferior-qualified thread-id, so the file would include "thread 1.1" instead of just "thread 1", even when there is only a single inferior. So, this commit adds print_full_thread_id, which is just like the existing print_thread_id, only it always prints the inferior-qualified thread-id. I then update the existing print_thread_id to make use of this new function, and finally, I update breakpoint::print_recreate_thread to also use this new function. There's a multi-inferior test that confirms the saved breakpoints file correctly includes the fully-qualified thread-id, and I've also updated the single inferior test gdb.base/save-bp.exp to have it validate that the saved breakpoints file includes the inferior-qualified thread-id, even for this single inferior case. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 0db3adaf916..6b616be547a 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -14141,7 +14141,10 @@ void breakpoint::print_recreate_thread (struct ui_file *fp) const { if (thread != -1) - gdb_printf (fp, " thread %d", thread); + { + struct thread_info *thr = find_thread_global_id (thread); + gdb_printf (fp, " thread %s", print_full_thread_id (thr)); + } if (task != -1) gdb_printf (fp, " task %d", task); diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index c0f27a8a66e..848daa94410 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -661,6 +661,10 @@ extern int show_inferior_qualified_tids (void); circular static buffer, NUMCELLS deep. */ const char *print_thread_id (struct thread_info *thr); +/* Like print_thread_id, but always prints the inferior-qualified form, + even when there is only a single inferior. */ +const char *print_full_thread_id (struct thread_info *thr); + /* Boolean test for an already-known ptid. */ extern bool in_thread_list (process_stratum_target *targ, ptid_t ptid); diff --git a/gdb/testsuite/gdb.base/save-bp.exp b/gdb/testsuite/gdb.base/save-bp.exp index 41d71837fb6..68933d36427 100644 --- a/gdb/testsuite/gdb.base/save-bp.exp +++ b/gdb/testsuite/gdb.base/save-bp.exp @@ -89,3 +89,19 @@ gdb_test_sequence "info break" "info break" [list \ "\[\r\n\]+\[ \t\]+printf" \ "\[\r\n\]+$disabled_row_start main at \[^\r\n\]*$srcfile:$loc_bp8" \ ] + +# Copy the saved breakpoints file to the local machine (if necessary), +# and then check its contents. +if {[is_remote host]} { + set bps [remote_upload host bps [standard_output_file bps]] +} +set fh [open $bps] +set lines [split [read $fh] "\n"] +close $fh + +with_test_prefix "in bps file" { + gdb_assert {[lsearch -regexp $lines "break ${srcfile}:${loc_bp2}$"] != -1} \ + "check for general breakpoint" + gdb_assert {[lsearch -regexp $lines "break ${srcfile}:${loc_bp3} thread 1\\.1"] != -1} \ + "check for thread-specific breakpoint" +} diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp index 777fcf85ab0..85c08f44a2c 100644 --- a/gdb/testsuite/gdb.multi/bp-thread-specific.exp +++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp @@ -15,6 +15,9 @@ # Check that GDB uses the correct thread-id when describing multiple # thread specific breakpoints at the same location. +# +# Also check that the correct thread-ids are used in the saved +# breakpoints file. # The plain remote target can't do multiple inferiors. require !use_gdb_stub @@ -59,3 +62,46 @@ gdb_test "break foo thread 1.1" \ "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \ "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \ "Breakpoint $decimal at $hex: foo\\. \\(2 locations\\)"] + +# Save the breakpoints into a file. +if {[is_remote host]} { + set bps bps +} else { + set bps [standard_output_file bps] +} + +remote_file host delete "$bps" +gdb_test "save breakpoints $bps" "" "save breakpoint to bps" + +if {[is_remote host]} { + set bps [remote_upload host bps [standard_output_file bps]] +} + +# Now dig through the saved breakpoints file and check that the +# thread-ids were written out correctly. First open the saved +# breakpoints and read them into a list. +set fh [open $bps] +set lines [split [read $fh] "\n"] +close $fh + +# Except the list created from the saved breakpoints file will have a +# blank line entry at the end, so remove it now. +gdb_assert {[string equal [lindex $lines end] ""]} \ + "check last item was an empty line" +set lines [lrange $lines 0 end-1] + +# These are the lines we expect in the saved breakpoints file, in the +# order that we expect them. These are strings, not regexps. +set expected_results \ + [list \ + "break -qualified main" \ + "break foo thread 2.1" \ + "break foo thread 1.1"] + +# Now check that the files contents (in LINES) matches the +# EXPECTED_RESULTS. +gdb_assert {[llength $lines] == [llength $expected_results]} \ + "correct number of lines in saved breakpoints file" +foreach a $lines b $expected_results { + gdb_assert {[string equal $a $b]} "line '$b'" +} diff --git a/gdb/thread.c b/gdb/thread.c index 1a852f70206..9ba383d9bee 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1431,12 +1431,22 @@ show_inferior_qualified_tids (void) const char * print_thread_id (struct thread_info *thr) { + if (show_inferior_qualified_tids ()) + return print_full_thread_id (thr); + char *s = get_print_cell (); + xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num); + return s; +} - if (show_inferior_qualified_tids ()) - xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num); - else - xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num); +/* See gdbthread.h. */ + +const char * +print_full_thread_id (struct thread_info *thr) +{ + char *s = get_print_cell (); + + xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num); return s; }
Andrew Burgess <aburgess@redhat.com> writes: > Pedro Alves <pedro@palves.net> writes: > >> Hi! >> >> On 2023-02-10 7:22 p.m., Andrew Burgess wrote: >>> Pedro Alves <pedro@palves.net> writes: >>> >>>> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote: >>>> >>>>> breakpoint::print_recreate_thread (struct ui_file *fp) const >>>>> { >>>>> if (thread != -1) >>>>> - gdb_printf (fp, " thread %d", thread); >>>>> + { >>>>> + struct thread_info *thr = find_thread_global_id (thread); >>>>> + gdb_printf (fp, " thread %s", print_thread_id (thr)); >>>> >>>> print_thread_id only prints the inferior-qualified thread id if >>>> there are multiple inferiors. I am wondering whether the save breakpoints >>>> file should _always_ end up with inferior-qualified thread ids, so that >>>> reloading the saved file works the same if you meanwhile add another >>>> inferior. >>> >>> As a counter argument; if the user has a single inferior and places >>> breakpoints on a particular thread, we'll have a save like: >>> >>> break foo thread 2 >>> >>> Then if the user sets up two inferiors, they can select which inferior >>> the breakpoints should apply to - source the saves from inferior 2, and >>> the b/p will apply to inferior 2 threads, source from inferior 1, and >>> the b/p will apply to inferior 1 threads. >>> >>> If the user has changed the inferior setup when sourcing the breakpoint >>> save file, I think they have to take some responsibility for knowing >>> what they want ... maybe? >>> >>> If you feel strongly then it's easy enough to print the qualified >>> thread-id, just let me know and I'll get it done. >>> >> >> My thinking is that internally, the thread is really inferior-qualified. >> It is just a presentation detail in the CLI that we don't print the >> inferior when there's only one inferior, for backwards compatibility. >> That may even change in the future. An MI frontend / GUI may be presenting >> the qualified ID, for instance. >> >> It seems to be that there are two valid approaches: >> >> #1 - we consider what the user typed when the breakpoint was created as canonical, >> and thus we save the breakpoint using the same breakpoint spec string that >> user typed originally, meaning, if the user typed: >> >> "break foo thread 1" >> >> then that's what we'd save, even if the user added a second >> inferior after creating the breakpoint. >> >> Of course, it follows then that if the breakpoint is created with >> >> "break foo thread 1.1" >> >> then that's what we save. So the user would have the option. >> >> I'm really not sure whether this is an option that we should be giving >> users, though. What if the breakpoint was created via Python, or via the >> MI with --thread ? Then the concept of original "thread" may not even exists, >> even though we save such a breakpoint too. >> >> #2 - we consider that the thread that the breakpoint ended up bound to is what >> is canonical and thus we always print the qualified id to the file. >> >> The approach in your patch is neither of the above -- it prints the qualified >> or non-qualified thread id depending on a CLI presentation detail, which seems >> brittle to me. >> >> Option #2 seems the simplest to explain, document, and implement, to me, >> but I could be convinced to go with #1 too. > > Thanks for the explanation. I've implemented #2 in the patch below, > what are your thoughts? > > Thanks, > Andrew > > --- > > commit 868a074345bb6d20d9a64470936d699c8a123894 > Author: Andrew Burgess <aburgess@redhat.com> > Date: Wed Feb 8 13:56:22 2023 +0000 > > gdb: don't use the global thread-id in the saved breakpoints file > > I noticed that breakpoint::print_recreate_thread was printing the > global thread-id. This function is used to implement the 'save > breakpoints' command, and should be writing out suitable CLI commands > for recreating the current breakpoints. The CLI does not use global > thread-ids, but instead uses the inferior specific thread-ids, > e.g. "2.1". > > After some discussion on the mailing list it was suggested that the > most consistent solution would be for the saved breakpoints file to > always contain the inferior-qualified thread-id, so the file would > include "thread 1.1" instead of just "thread 1", even when there is > only a single inferior. > > So, this commit adds print_full_thread_id, which is just like the > existing print_thread_id, only it always prints the inferior-qualified > thread-id. > > I then update the existing print_thread_id to make use of this new > function, and finally, I update breakpoint::print_recreate_thread to > also use this new function. > > There's a multi-inferior test that confirms the saved breakpoints file > correctly includes the fully-qualified thread-id, and I've also > updated the single inferior test gdb.base/save-bp.exp to have it > validate that the saved breakpoints file includes the > inferior-qualified thread-id, even for this single inferior case. I'm planning to push this patch some time next week unless someone objects. Thanks, Andrew > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 0db3adaf916..6b616be547a 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -14141,7 +14141,10 @@ void > breakpoint::print_recreate_thread (struct ui_file *fp) const > { > if (thread != -1) > - gdb_printf (fp, " thread %d", thread); > + { > + struct thread_info *thr = find_thread_global_id (thread); > + gdb_printf (fp, " thread %s", print_full_thread_id (thr)); > + } > > if (task != -1) > gdb_printf (fp, " task %d", task); > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index c0f27a8a66e..848daa94410 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -661,6 +661,10 @@ extern int show_inferior_qualified_tids (void); > circular static buffer, NUMCELLS deep. */ > const char *print_thread_id (struct thread_info *thr); > > +/* Like print_thread_id, but always prints the inferior-qualified form, > + even when there is only a single inferior. */ > +const char *print_full_thread_id (struct thread_info *thr); > + > /* Boolean test for an already-known ptid. */ > extern bool in_thread_list (process_stratum_target *targ, ptid_t ptid); > > diff --git a/gdb/testsuite/gdb.base/save-bp.exp b/gdb/testsuite/gdb.base/save-bp.exp > index 41d71837fb6..68933d36427 100644 > --- a/gdb/testsuite/gdb.base/save-bp.exp > +++ b/gdb/testsuite/gdb.base/save-bp.exp > @@ -89,3 +89,19 @@ gdb_test_sequence "info break" "info break" [list \ > "\[\r\n\]+\[ \t\]+printf" \ > "\[\r\n\]+$disabled_row_start main at \[^\r\n\]*$srcfile:$loc_bp8" \ > ] > + > +# Copy the saved breakpoints file to the local machine (if necessary), > +# and then check its contents. > +if {[is_remote host]} { > + set bps [remote_upload host bps [standard_output_file bps]] > +} > +set fh [open $bps] > +set lines [split [read $fh] "\n"] > +close $fh > + > +with_test_prefix "in bps file" { > + gdb_assert {[lsearch -regexp $lines "break ${srcfile}:${loc_bp2}$"] != -1} \ > + "check for general breakpoint" > + gdb_assert {[lsearch -regexp $lines "break ${srcfile}:${loc_bp3} thread 1\\.1"] != -1} \ > + "check for thread-specific breakpoint" > +} > diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp > index 777fcf85ab0..85c08f44a2c 100644 > --- a/gdb/testsuite/gdb.multi/bp-thread-specific.exp > +++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp > @@ -15,6 +15,9 @@ > > # Check that GDB uses the correct thread-id when describing multiple > # thread specific breakpoints at the same location. > +# > +# Also check that the correct thread-ids are used in the saved > +# breakpoints file. > > # The plain remote target can't do multiple inferiors. > require !use_gdb_stub > @@ -59,3 +62,46 @@ gdb_test "break foo thread 1.1" \ > "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \ > "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \ > "Breakpoint $decimal at $hex: foo\\. \\(2 locations\\)"] > + > +# Save the breakpoints into a file. > +if {[is_remote host]} { > + set bps bps > +} else { > + set bps [standard_output_file bps] > +} > + > +remote_file host delete "$bps" > +gdb_test "save breakpoints $bps" "" "save breakpoint to bps" > + > +if {[is_remote host]} { > + set bps [remote_upload host bps [standard_output_file bps]] > +} > + > +# Now dig through the saved breakpoints file and check that the > +# thread-ids were written out correctly. First open the saved > +# breakpoints and read them into a list. > +set fh [open $bps] > +set lines [split [read $fh] "\n"] > +close $fh > + > +# Except the list created from the saved breakpoints file will have a > +# blank line entry at the end, so remove it now. > +gdb_assert {[string equal [lindex $lines end] ""]} \ > + "check last item was an empty line" > +set lines [lrange $lines 0 end-1] > + > +# These are the lines we expect in the saved breakpoints file, in the > +# order that we expect them. These are strings, not regexps. > +set expected_results \ > + [list \ > + "break -qualified main" \ > + "break foo thread 2.1" \ > + "break foo thread 1.1"] > + > +# Now check that the files contents (in LINES) matches the > +# EXPECTED_RESULTS. > +gdb_assert {[llength $lines] == [llength $expected_results]} \ > + "correct number of lines in saved breakpoints file" > +foreach a $lines b $expected_results { > + gdb_assert {[string equal $a $b]} "line '$b'" > +} > diff --git a/gdb/thread.c b/gdb/thread.c > index 1a852f70206..9ba383d9bee 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -1431,12 +1431,22 @@ show_inferior_qualified_tids (void) > const char * > print_thread_id (struct thread_info *thr) > { > + if (show_inferior_qualified_tids ()) > + return print_full_thread_id (thr); > + > char *s = get_print_cell (); > + xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num); > + return s; > +} > > - if (show_inferior_qualified_tids ()) > - xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num); > - else > - xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num); > +/* See gdbthread.h. */ > + > +const char * > +print_full_thread_id (struct thread_info *thr) > +{ > + char *s = get_print_cell (); > + > + xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num); > return s; > } >
On 2023-03-16 5:06 p.m., Andrew Burgess wrote: > Andrew Burgess <aburgess@redhat.com> writes: > >> Pedro Alves <pedro@palves.net> writes: >>> >>> On 2023-02-10 7:22 p.m., Andrew Burgess wrote: >>>> Pedro Alves <pedro@palves.net> writes: >>> My thinking is that internally, the thread is really inferior-qualified. >>> It is just a presentation detail in the CLI that we don't print the >>> inferior when there's only one inferior, for backwards compatibility. >>> That may even change in the future. An MI frontend / GUI may be presenting >>> the qualified ID, for instance. >>> >>> It seems to be that there are two valid approaches: >>> >>> #1 - we consider what the user typed when the breakpoint was created as canonical, >>> and thus we save the breakpoint using the same breakpoint spec string that >>> user typed originally, meaning, if the user typed: >>> >>> "break foo thread 1" >>> >>> then that's what we'd save, even if the user added a second >>> inferior after creating the breakpoint. >>> >>> Of course, it follows then that if the breakpoint is created with >>> >>> "break foo thread 1.1" >>> >>> then that's what we save. So the user would have the option. >>> >>> I'm really not sure whether this is an option that we should be giving >>> users, though. What if the breakpoint was created via Python, or via the >>> MI with --thread ? Then the concept of original "thread" may not even exists, >>> even though we save such a breakpoint too. >>> >>> #2 - we consider that the thread that the breakpoint ended up bound to is what >>> is canonical and thus we always print the qualified id to the file. >>> >>> The approach in your patch is neither of the above -- it prints the qualified >>> or non-qualified thread id depending on a CLI presentation detail, which seems >>> brittle to me. >>> >>> Option #2 seems the simplest to explain, document, and implement, to me, >>> but I could be convinced to go with #1 too. >> >> Thanks for the explanation. I've implemented #2 in the patch below, >> what are your thoughts? Sorry for the delay. (I simply forgot to reply.) It looks good to me. > > I'm planning to push this patch some time next week unless someone > objects. >
Pedro Alves <pedro@palves.net> writes: > On 2023-03-16 5:06 p.m., Andrew Burgess wrote: >> Andrew Burgess <aburgess@redhat.com> writes: >> >>> Pedro Alves <pedro@palves.net> writes: >>>> >>>> On 2023-02-10 7:22 p.m., Andrew Burgess wrote: >>>>> Pedro Alves <pedro@palves.net> writes: > >>>> My thinking is that internally, the thread is really inferior-qualified. >>>> It is just a presentation detail in the CLI that we don't print the >>>> inferior when there's only one inferior, for backwards compatibility. >>>> That may even change in the future. An MI frontend / GUI may be presenting >>>> the qualified ID, for instance. >>>> >>>> It seems to be that there are two valid approaches: >>>> >>>> #1 - we consider what the user typed when the breakpoint was created as canonical, >>>> and thus we save the breakpoint using the same breakpoint spec string that >>>> user typed originally, meaning, if the user typed: >>>> >>>> "break foo thread 1" >>>> >>>> then that's what we'd save, even if the user added a second >>>> inferior after creating the breakpoint. >>>> >>>> Of course, it follows then that if the breakpoint is created with >>>> >>>> "break foo thread 1.1" >>>> >>>> then that's what we save. So the user would have the option. >>>> >>>> I'm really not sure whether this is an option that we should be giving >>>> users, though. What if the breakpoint was created via Python, or via the >>>> MI with --thread ? Then the concept of original "thread" may not even exists, >>>> even though we save such a breakpoint too. >>>> >>>> #2 - we consider that the thread that the breakpoint ended up bound to is what >>>> is canonical and thus we always print the qualified id to the file. >>>> >>>> The approach in your patch is neither of the above -- it prints the qualified >>>> or non-qualified thread id depending on a CLI presentation detail, which seems >>>> brittle to me. >>>> >>>> Option #2 seems the simplest to explain, document, and implement, to me, >>>> but I could be convinced to go with #1 too. >>> >>> Thanks for the explanation. I've implemented #2 in the patch below, >>> what are your thoughts? > > Sorry for the delay. (I simply forgot to reply.) It looks good to me. > Not a problem. Now pushed. Thanks, Andrew
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 6b576859592..6e3c76305e7 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -14120,7 +14120,10 @@ void breakpoint::print_recreate_thread (struct ui_file *fp) const { if (thread != -1) - gdb_printf (fp, " thread %d", thread); + { + struct thread_info *thr = find_thread_global_id (thread); + gdb_printf (fp, " thread %s", print_thread_id (thr)); + } if (task != 0) gdb_printf (fp, " task %d", task); diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp index 777fcf85ab0..6a7cd044af4 100644 --- a/gdb/testsuite/gdb.multi/bp-thread-specific.exp +++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp @@ -15,6 +15,9 @@ # Check that GDB uses the correct thread-id when describing multiple # thread specific breakpoints at the same location. +# +# Also check that the correct thread-ids are used in the saved +# breakpoints file. # The plain remote target can't do multiple inferiors. require !use_gdb_stub @@ -59,3 +62,46 @@ gdb_test "break foo thread 1.1" \ "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \ "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \ "Breakpoint $decimal at $hex: foo\\. \\(2 locations\\)"] + +# Save the breakpoints into a file. +if {[is_remote host]} { + set bps bps +} else { + set bps [standard_output_file bps] +} + +remote_file host delete "$bps" +gdb_test "save breakpoint $bps" "" "save breakpoint to bps" + +if {[is_remote host]} { + set bps [remote_upload host bps [standard_output_file bps]] +} + +# Now dig through the saved breakpoints file and check that the +# thread-ids were written out correctly. First open the saved +# breakpoints and read them into a list. +set fh [open $bps] +set lines [split [read $fh] "\n"] +close $fh + +# Except the list created from the saved breakpoints file will have a +# blank line entry at the end, so remove it now. +gdb_assert {[string equal [lindex $lines end] ""]} \ + "check last item was an empty line" +set lines [lrange $lines 0 end-1] + +# These are the lines we expect in the saved breakpoints file, in the +# order that we expect them. These are strings, not regexps. +set expected_results \ + [list \ + "break -qualified main" \ + "break foo thread 2.1" \ + "break foo thread 1.1"] + +# Now check that the files contents (in LINES) matches the +# EXPECTED_RESULTS. +gdb_assert {[llength $lines] == [llength $expected_results]} \ + "correct number of lines in saved breakpoints file" +foreach a $lines b $expected_results { + gdb_assert {[string equal $a $b]} "line '$b'" +}