From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 717763858D35 for ; Thu, 16 Mar 2023 17:06:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 717763858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1678986385; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SVL0i8N7bEDMidaKic9E4cjte2R3o3CkX7Nl4nWmlak=; b=YnQHYIsW7+f7p1tJgSFgl41+k8tVHLwB3brKWFAHx7roXrcY+cGU7P2z4aOrVUe8ku/P83 UxISqgmTcqT1wuxefgNLxnmbIkSDInriLh3ycyKjjkpQpWWj7deIxICfnS1pHlmsMTKKZc RB7LF5rGjbk7qpRNu0x6by+n3RDqdnU= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-357-0bj_Rr88Nm6eSMYnewef8w-1; Thu, 16 Mar 2023 13:06:23 -0400 X-MC-Unique: 0bj_Rr88Nm6eSMYnewef8w-1 Received: by mail-ed1-f69.google.com with SMTP id t14-20020a056402240e00b004fb36e6d670so3956458eda.5 for ; Thu, 16 Mar 2023 10:06:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678986381; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=SVL0i8N7bEDMidaKic9E4cjte2R3o3CkX7Nl4nWmlak=; b=nBYkTbF/7H6vjFjsadJn0Pc6tfnsrgXD2hIiCTD47/9dPCv/Uytosjw5f8OXWybKpX 0H9c96foUqsBhVk71C1uWJ900RIwaWMuNtej+iTRjsJTHs+XPnwADrDaNybtMzROkUZq ehxmv5OU+4wLt5uKyLvgTnGAiyzDOTl7VfULZRRFOWibRc2F5lHliTOQXg/2SEXrYvyq NgEDjDdTmhW7ktk4jE1b7V22wFwWlX0J5Qu4CNBEV9GszxBoJtMbC1tVJHQNrE8RoTAX b8DAHsp3o+KsFEvDyQBFhFQvQ5CijHYZaM3FDfFsnDC1OkIMSJCj77OUc6GqRhtE2RRg FJew== X-Gm-Message-State: AO0yUKWmiThozZVUFNNKRLZrjkjsSOTqH3byT8klbRKyVbgJbvX+HqE+ xKgwi//9RUZlLPQOHZKqVmZq4y5DNqFA9HJUN6rGJHmwBRqYbAz2ZraSNPcdJB/bVdton1KhPEV 6e8FY3caBt0w7NhSbB7EW5RAI4iOiVQ== X-Received: by 2002:aa7:c956:0:b0:4fe:9160:6a7b with SMTP id h22-20020aa7c956000000b004fe91606a7bmr310321edt.9.1678986381276; Thu, 16 Mar 2023 10:06:21 -0700 (PDT) X-Google-Smtp-Source: AK7set/hMMPTeSpv+mgId8r2Tlid9sgVkYS5Qc7KLnfOuO8PJnD65W+RuteX+vBi/9WdIAn4DN6g0Q== X-Received: by 2002:aa7:c956:0:b0:4fe:9160:6a7b with SMTP id h22-20020aa7c956000000b004fe91606a7bmr310299edt.9.1678986380874; Thu, 16 Mar 2023 10:06:20 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id z23-20020a50cd17000000b004bd1fe2cc02sm19316edi.16.2023.03.16.10.06.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Mar 2023 10:06:20 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file In-Reply-To: <87o7pej3ir.fsf@redhat.com> References: <22df7afc-cf88-d260-516d-7b9a45e2ad78@palves.net> <87pmahuxzu.fsf@redhat.com> <00607ab6-b94c-869c-5d1a-7528cf4dd85f@palves.net> <87o7pej3ir.fsf@redhat.com> Date: Thu, 16 Mar 2023 17:06:19 +0000 Message-ID: <87ttykeid0.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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 List-Id: Andrew Burgess writes: > Pedro Alves writes: > >> Hi! >> >> On 2023-02-10 7:22 p.m., Andrew Burgess wrote: >>> Pedro Alves 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 > 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; > } >