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.133.124]) by sourceware.org (Postfix) with ESMTPS id B3DFC3858C3A for ; Mon, 6 Feb 2023 11:06:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B3DFC3858C3A 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=1675681586; 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=6/xS1DgPyHR05pnAz7dBZaa3gPD7RQSJ/Km4XOyaKt8=; b=PdETjQS1dWLG/n01WbOx5g6y380Orv5qIlka0KNTeFhwgfp7QzfVbDxfS5qMoNhf8v0lNA aVtg6ZeFWz14Ndh2/v7dY5YMrwHm2usn/53j538DStjjYU188aRmdDGkLyVqVNgh4QY54x KMo+MpE2nvtA9RVHDbWUHQO41chKlec= Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-372-rtIbU_7pMPSQwaAdcJCRWQ-1; Mon, 06 Feb 2023 06:06:25 -0500 X-MC-Unique: rtIbU_7pMPSQwaAdcJCRWQ-1 Received: by mail-qk1-f200.google.com with SMTP id x12-20020a05620a258c00b007051ae500a2so7548029qko.15 for ; Mon, 06 Feb 2023 03:06:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=6/xS1DgPyHR05pnAz7dBZaa3gPD7RQSJ/Km4XOyaKt8=; b=tagbiysqvd4km2K6tms365pfJGDMoqXTrV6dSrExk1oyty3+BLgbLPXzmP+FCZw2dp /tyff/mrpUrBEY+e375gIcYhiTLzg9bXhJw0nvVWy/sBMPPCBQbA0WyzLjBi9uk7RkP5 fW5zSVPVKgMVX574sKky6QDIHwIDxXoWUhcbqFFZYUoXo0p1wNmbCrUDBrGvVkCbJ3Wf 9DrzNXqQLt25skM1UnlNIrLwJCjGaUhJHM4rCYui/rKjbXgUzEEOUOm+V8M6nnOfJdcM X0GTgDQiTr396Vokfg9OAScsqbyVKSLK6T1LaDZ/ZV0gvoQh4goDOY3W7PGBYMOfLENq ix0Q== X-Gm-Message-State: AO0yUKXUrkT+SNBptMTACLwqSsF0NdEPH37/QgBBCb2S/arG/5XfoJei 1geRqLiTrkIWBs1/ljv5t0Tfv0cj7QuPt86zqUloO83AZ6tY6ThFGMB+9zKfUIMTsS/32xqGKnT wAw5Q4zP5y9ASVMbFHN57HQ== X-Received: by 2002:a05:622a:148b:b0:3b9:c153:f169 with SMTP id t11-20020a05622a148b00b003b9c153f169mr32427969qtx.0.1675681584497; Mon, 06 Feb 2023 03:06:24 -0800 (PST) X-Google-Smtp-Source: AK7set8k97+edLmOUz3s0q2J7w5+F6w+RWYwvg2nxrV5v4LrXBIAI50GVERbmhmnkPD0bV11tlURJg== X-Received: by 2002:a05:622a:148b:b0:3b9:c153:f169 with SMTP id t11-20020a05622a148b00b003b9c153f169mr32427927qtx.0.1675681584159; Mon, 06 Feb 2023 03:06:24 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id w9-20020ac87189000000b003b9b41a32b7sm6972553qto.81.2023.02.06.03.06.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Feb 2023 03:06:23 -0800 (PST) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv2 4/6] gdb: error if 'thread' or 'task' keywords are overused In-Reply-To: <52913f3f-834b-dc13-1f15-9eef8db802e7@palves.net> References: <52913f3f-834b-dc13-1f15-9eef8db802e7@palves.net> Date: Mon, 06 Feb 2023 11:06:21 +0000 Message-ID: <87k00vvys2.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: Pedro Alves writes: > On 2023-01-20 9:46 a.m., Andrew Burgess via Gdb-patches wrote: >> When creating a breakpoint or watchpoint, the 'thread' and 'task' >> keywords can be used to create a thread or task specific breakpoint or >> watchpoint. >> >> Currently, a thread or task specific breakpoint can only apply for a >> single thread or task, if multiple threads or tasks are specified when >> creating the breakpoint (or watchpoint), then the last specified id >> will be used. >> >> The exception to the above is that when the 'thread' keyword is used >> during the creation of a watchpoint, GDB will give an error if >> 'thread' is given more than once. >> >> In this commit I propose making this behaviour consistent, if the >> 'thread' or 'task' keywords are used more than once when creating >> either a breakpoint or watchpoint, then GDB will give an error. > > The patch looks fine, but does it make sense to allow both thread and task > at the same time? Thanks for the review. I've made the fix Eli suggested, and pushed this patch. Given Joel's feedback I'll take a look at a follow up patch the prevents thread and task being used together. Thanks, Andrew > > For instance, with gdb.ada/tasks.exp : > > (gdb) b foo thread 1 task 2 > Breakpoint 1 at 0x555555557bd6: file /home/pedro/gdb/rocm/gdb/src/gdb/testsuite/gdb.ada/tasks/foo.adb, line 16. > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y 0x0000555555557bd6 in foo at /home/pedro/gdb/rocm/gdb/src/gdb/testsuite/gdb.ada/tasks/foo.adb:16 thread 1 > stop only in thread 1 > > Pedro Alves > >> >> I haven't updated the manual, we don't explicitly say that these >> keywords can be repeated, and (to me), given the keyword takes a >> single id, I don't think it makes much sense to repeat the keyword. >> As such, I see this more as adding a missing error to GDB, rather than >> making some big change. However, I have added an entry to the NEWS >> file as I guess it is possible that some people might hit this new >> error with an existing (I claim, badly written) GDB script. >> >> I've added some new tests to check for the new error. >> >> Just one test needed updating, gdb.linespec/keywords.exp, this test >> did use the 'thread' keyword twice, and expected the breakpoint to be >> created. Looking at what this test was for though, it was checking >> the use of '-force-condition', and I don't think that being able to >> repeat 'thread' was actually a critical part of this test. >> >> As such, I've updated this test to expect the error when 'thread' is >> repeated. >> --- >> gdb/NEWS | 9 +++++++++ >> gdb/breakpoint.c | 9 +++++++++ >> gdb/testsuite/gdb.ada/tasks.exp | 4 ++++ >> gdb/testsuite/gdb.linespec/keywords.exp | 10 ++++++++-- >> gdb/testsuite/gdb.threads/thread-specific-bp.exp | 4 ++++ >> gdb/testsuite/gdb.threads/watchthreads2.exp | 3 +++ >> 6 files changed, 37 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index c0aac212e30..fb49f62f7e6 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -9,6 +9,15 @@ >> This support requires that GDB be built with Python scripting >> enabled. >> >> +* For the break command, multiple uses of the 'thread' or 'task' >> + keywords will now give an error instead of just using the thread or >> + task id from the last instance of the keyword. >> + >> +* For the watch command, multiple uses of the 'task' keyword will now >> + give an error instead of just using the task id from the last >> + instance of the keyword. The 'thread' keyword already gave an error >> + when used multiple times with the watch command, this remains unchanged. >> + >> * New commands >> >> maintenance print record-instruction [ N ] >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index b2cd89511fb..1400fd49642 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c >> @@ -8801,6 +8801,9 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc, >> const char *tmptok; >> struct thread_info *thr; >> >> + if (*thread != -1) >> + error(_("You can specify only one thread.")); >> + >> tok = end_tok + 1; >> thr = parse_thread_id (tok, &tmptok); >> if (tok == tmptok) >> @@ -8812,6 +8815,9 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc, >> { >> char *tmptok; >> >> + if (*task != 0) >> + error(_("You can specify only one task.")); >> + >> tok = end_tok + 1; >> *task = strtol (tok, &tmptok, 0); >> if (tok == tmptok) >> @@ -10094,6 +10100,9 @@ watch_command_1 (const char *arg, int accessflag, int from_tty, >> { >> char *tmp; >> >> + if (task != 0) >> + error(_("You can specify only one task.")); >> + >> task = strtol (value_start, &tmp, 0); >> if (tmp == value_start) >> error (_("Junk after task keyword.")); >> diff --git a/gdb/testsuite/gdb.ada/tasks.exp b/gdb/testsuite/gdb.ada/tasks.exp >> index 23bf3937a20..4441d92719c 100644 >> --- a/gdb/testsuite/gdb.ada/tasks.exp >> +++ b/gdb/testsuite/gdb.ada/tasks.exp >> @@ -39,6 +39,10 @@ gdb_test "info tasks" \ >> "\r\n"] \ >> "info tasks before inserting breakpoint" >> >> +# Check that multiple uses of the 'task' keyword will give an error. >> +gdb_test "break break_me task 1 task 3" "You can specify only one task\\." >> +gdb_test "watch j task 1 task 3" "You can specify only one task\\." >> + >> # Insert a breakpoint that should stop only if task 1 stops. Since >> # task 1 never calls break_me, this shouldn't actually ever trigger. >> # The fact that this breakpoint is created _before_ the next one >> diff --git a/gdb/testsuite/gdb.linespec/keywords.exp b/gdb/testsuite/gdb.linespec/keywords.exp >> index bff64249542..dc66e32237c 100644 >> --- a/gdb/testsuite/gdb.linespec/keywords.exp >> +++ b/gdb/testsuite/gdb.linespec/keywords.exp >> @@ -80,8 +80,14 @@ foreach prefix {"" "thread 1 "} { >> foreach suffix {"" " " " thread 1"} { >> foreach cond {"" " if 1"} { >> with_test_prefix "prefix: '$prefix', suffix: '$suffix', cond: '$cond'" { >> - gdb_breakpoint "main ${prefix}-force-condition${suffix}${cond}"\ >> - "message" >> + >> + if { [regexp thread $prefix] && [regexp thread $suffix] } { >> + gdb_test "break main ${prefix}-force-condition${suffix}${cond}" \ >> + "You can specify only one thread\\." >> + } else { >> + gdb_breakpoint "main ${prefix}-force-condition${suffix}${cond}"\ >> + "message" >> + } >> } >> } >> } >> diff --git a/gdb/testsuite/gdb.threads/thread-specific-bp.exp b/gdb/testsuite/gdb.threads/thread-specific-bp.exp >> index d33b4f47258..008ae5ed05e 100644 >> --- a/gdb/testsuite/gdb.threads/thread-specific-bp.exp >> +++ b/gdb/testsuite/gdb.threads/thread-specific-bp.exp >> @@ -63,6 +63,10 @@ proc check_thread_specific_breakpoint {non_stop} { >> return -1 >> } >> >> + # Check that multiple uses of 'thread' keyword give an error. >> + gdb_test "break main thread $start_thre thread $main_thre" \ >> + "You can specify only one thread\\." >> + >> # Set a thread-specific breakpoint at "main". This can't ever >> # be hit, but that's OK. >> gdb_breakpoint "main thread $start_thre" >> diff --git a/gdb/testsuite/gdb.threads/watchthreads2.exp b/gdb/testsuite/gdb.threads/watchthreads2.exp >> index 09858aee486..a1398d668a4 100644 >> --- a/gdb/testsuite/gdb.threads/watchthreads2.exp >> +++ b/gdb/testsuite/gdb.threads/watchthreads2.exp >> @@ -71,6 +71,9 @@ if { $nr_started == $NR_THREADS } { >> return -1 >> } >> >> +# Check that multiple uses of the 'thread' keyword will give an error. >> +gdb_test "watch x thread 1 thread 2" "You can specify only one thread\\." >> + >> # Watch X, it will be modified by all threads. >> # We want this watchpoint to be set *after* all threads are running. >> gdb_test "watch x" "Hardware watchpoint 3: x" >>