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 DD7273858D20 for ; Fri, 3 Feb 2023 16:41:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DD7273858D20 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=1675442514; 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=3dqPrIYcJNVy6b6A6SkySoSdqqt05uHzSMkFohqJwvc=; b=BDjPlqYnTblct71omW6lhU+n3IHmPlGaC78IxqIcr/WWumkdY0Pw4FgkdW6uD2tskGyVBS uinze/ER7Y+sEasXoHyySUTQZVWQaN6t/ZzjB7nL0mZHuMboZf0HqXIzf0m6ebUB9kf5kO sorF+6u8z9NRI+xStmUi63IapCzYK3Q= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-606-nSsKRYGHOaW7CMlX7o0olw-1; Fri, 03 Feb 2023 11:41:53 -0500 X-MC-Unique: nSsKRYGHOaW7CMlX7o0olw-1 Received: by mail-qk1-f198.google.com with SMTP id u11-20020a05620a430b00b007052a66d201so3633002qko.23 for ; Fri, 03 Feb 2023 08:41:52 -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=3dqPrIYcJNVy6b6A6SkySoSdqqt05uHzSMkFohqJwvc=; b=xUZKqcUjvy7DDOLYQvcDaOKOUsxJahie94sIqIKffbPxAjk+oSVCiVnWjbtANVUe0Z frBmjtANR7A7B5zlugqMYKsFr/zw//H5qdEVfIua3LJzGzurMUvDdmZbLwC+DPvUhkXE 9MlhtoPslx6gps+OH4PJqZyI3UxSWA3p8j64qQ653bYdBYA543gS4TGqKXBAlKCctFLR tIw823TF3sAuEhVnH9WbwX2+ukRcqfXxLLKDrT6yPCG0s0oedO7Owexe2Y6YshtlJq3/ SeZxqbtus7TROiC9MJARr5EOz7yyEjimGy10cGtoWQbccJmnBPMu4/d0pEjna2T9fpAE kAZg== X-Gm-Message-State: AO0yUKXDdibKvTGh0xZ1RlDzLatuNyi9t/668NGhkeIZ6nS+kdCgaknd yrDDgLe+8XyQkFCosJkyDlKtPqZ3HCwP8Uqv3nZVhatwprLR0oy7Zlg4bAn9MW3jFpm067cogLs zxxJrwlwO2k1YhyKuHUvurw== X-Received: by 2002:ac8:5fd1:0:b0:3ba:101e:88c6 with SMTP id k17-20020ac85fd1000000b003ba101e88c6mr2590020qta.48.1675442512240; Fri, 03 Feb 2023 08:41:52 -0800 (PST) X-Google-Smtp-Source: AK7set/YF9Pl+gRU5xehEEEgTvRm1uqsQzOe5NCgccZOEcXWP3sJCYBq1k9CdEIryTO5/grjI3YiFg== X-Received: by 2002:ac8:5fd1:0:b0:3ba:101e:88c6 with SMTP id k17-20020ac85fd1000000b003ba101e88c6mr2589977qta.48.1675442511833; Fri, 03 Feb 2023 08:41:51 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id p1-20020ac84081000000b003a530a32f67sm1803725qtl.65.2023.02.03.08.41.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Feb 2023 08:41:51 -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: Fri, 03 Feb 2023 16:41:49 +0000 Message-ID: <87mt5ur99u.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? I don't know enough about Ada tasks to comment here. If someone who knows can say categorically that threads and tasks can't coexist than I'd be happy to add a patch to prevent them 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" >>