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 321A3385828F for ; Mon, 18 Dec 2023 11:33:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 321A3385828F Authentication-Results: sourceware.org; dmarc=pass (p=none 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 321A3385828F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702899217; cv=none; b=tESM3Hn516xQbqLqpCMElfzcdpsqesY4g4i1QjEcbWRzRh5bbn4GSIwwLQDO/dfo88LyG4h1bmyPNDQCulIRTnQ++WsBDgeP3rMRIFWh1A6U4CdZcMohDhJReS8wDHRG4faDdvv4mL4qt3371oP1nQtx1jp+vqzlpgFELalKDr0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702899217; c=relaxed/simple; bh=It1O9EkdxLJMppvg9m+STKff4bN072JR0Yo0oh0h520=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=aKAHjyt7GU1K+MdVpC+W2zmPM+2PQbH5XuUH+k/ZLf7V33YulKeoc/CEsahV1woG21phjMgDMEiB26I9l1xY/y/mbWQ6wx86jL+l0W7PSLm56ZhRaOP7iIoWQ1gL2xoMsQ3Xx9HzMbELScE8G5ifeU3TJFzG6cV3Wf9H+ayonIY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1702899206; 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: in-reply-to:in-reply-to:references:references; bh=avP9qQzmrdcNMy+PcWDKyFJkRo6m2bvFbpSYFxmTbpA=; b=eCL7hFdVhVyC97asTDAPn6P0z8JbgM1Z9Ple8kK1HiDKY7UH7jjfSRnF83DS8x8tCKD+VN lENdvmAkgzrJoEDC9NnxPEgEUbBmqVR43wUOwy98iLQ0tffsZbVXFG0SbVchjEw+DLlTGc HHI/GC2M0+/u8dgTs6BDN0v0gHeZdPw= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-88-yjaDMkNEOkW_mlG22ueJWg-1; Mon, 18 Dec 2023 06:33:25 -0500 X-MC-Unique: yjaDMkNEOkW_mlG22ueJWg-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-40c2a43b0f3so23223035e9.2 for ; Mon, 18 Dec 2023 03:33:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702899203; x=1703504003; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=avP9qQzmrdcNMy+PcWDKyFJkRo6m2bvFbpSYFxmTbpA=; b=musNqpWjrHt9Q5xML0eD0KKh8lgMIXWk4aCg52LOqoYrGFvE2lbjtq1QTZXnwyQQ7o KpmBkywMJv0wTlAmxREUCHEeshN0rZ+1l6uXLymBaX0zSUJ0Q2ikX49XwlPyckDzi8Je 9hX9zphCerV0tOxDBw0wQDXpPbJgVfSpuO/BFannaS3/VpxT8Up0UWd1oNRzJ/oOFU0u b42erXy4lUCM+1mqvOMhSMMtckeqtbP1e8pwv2zsUa3VE7vOxFIDDuT6VkvpmZamg1Or gPwCW1f5VDTrCybZqltFjup17+YYd6+hIas+P8qxy7AwfG5AiYa+1xIH6vDHmB/H7zfF SY+A== X-Gm-Message-State: AOJu0YxSIHo05d9K32fHozHZG8gXzsfL8F2w6QiAgwu/s+7PUAAv2YWf RuuL+x4648vXHI244SsXeGhkbo/ajLJ6gQthMNHoHNswbzjWnTNa2L7orTxFbcCOE/ksde5S9yX TqrJXf+VV8eakZO5p292qpTSefBEeHg== X-Received: by 2002:a05:600c:468c:b0:40c:6e48:a82f with SMTP id p12-20020a05600c468c00b0040c6e48a82fmr1578218wmo.181.1702899203443; Mon, 18 Dec 2023 03:33:23 -0800 (PST) X-Google-Smtp-Source: AGHT+IH/j644rbfBwAYtcU10g1enO8/h54cO7MK3ceF71SGO+kZ7FDgmMzRNkg84gAlihNZtEmPMxA== X-Received: by 2002:a05:600c:468c:b0:40c:6e48:a82f with SMTP id p12-20020a05600c468c00b0040c6e48a82fmr1578211wmo.181.1702899203013; Mon, 18 Dec 2023 03:33:23 -0800 (PST) Received: from localhost (105.226.159.143.dyn.plus.net. [143.159.226.105]) by smtp.gmail.com with ESMTPSA id dd14-20020a0560001e8e00b003364277e714sm13856992wrb.89.2023.12.18.03.33.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Dec 2023 03:33:22 -0800 (PST) From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org, Eli Zaretskii Subject: Re: [PATCHv7 07/11] gdb: parse pending breakpoint thread/task immediately In-Reply-To: <878r5v5gsi.fsf@tromey.com> References: <16e9732a00d369533933ea939753a940d8c5a560.1702507015.git.aburgess@redhat.com> <878r5v5gsi.fsf@tromey.com> Date: Mon, 18 Dec 2023 11:33:21 +0000 Message-ID: <87v88v4uf2.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=-5.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: Tom Tromey writes: >>>>>> "Andrew" == Andrew Burgess writes: > > Andrew> As a result of this commit the breakpoints 'extra_string' field is now > Andrew> only used by bp_dprintf type breakpoints to hold the printf format and > Andrew> arguments. This string should always be empty for other breakpoint > Andrew> types. This allows me to clean up some code in > Andrew> print_breakpoint_location. > > I wonder if this member can be pushed into struct dprintf_breakpoint. > (I don't think you should do that in this series, you've been waiting > long enough.) > > Andrew> 2. The handling of '-force-condition' is odd, if this flag appears > Andrew> immediately after a condition then it will be treated as part of the > Andrew> condition, e.g.: > > We probably should have required this to appear before the linespec. > > Maybe also thread/task/etc. So Baris also asked about this change on V6. Apparently when the -force-condition flag was first added there was a couple of requests that the implementation change to use a more GDB option like approach, and I think that makes sense. I looked into this a little, and it's a slightly more complex change. The existing breakpoint options are not handled like "normal" options, but are tied up with location parsing. And I don't think it makes sense to handle -force-condition (or thread/task/etc) there -- these options are related to the condition, not the location. So my thinking is that what should happen is that the location parsing code should export the gdb::option::* related data structures needed to parse the location related options. The condition related code should export the gdb::option::* related data structures needed to parse the condition related options. Then in breakpoint.c we'd create a gdb::option::option_def_group that can parse all of the various options. And finally, we can send those parsed options from breakpoint.c down into the various parts of GDB (the location parsing and condition parsing). However.... one of the breakpoint options is a filename, and that option currently supports filename completion. So, to move to the gdb::option::* approach we need a new gdb::option::filename_option_def, which we don't currently have. Actually we're going to need a couple of new *_option_def types, but I think filenames are the hardest. I'm aware of a couple of attempts to add a filename_option_def; Pedro has a branch `filename-options` on his github, and also Lancelot posted this a while back: https://inbox.sourceware.org/gdb-patches/20210213220752.32581-1-lsix@lancelotsix.com/ I prefer Pedro's approach, though I haven't finished re-reading Lancelot's branch (apparently I did review it when it was originally posted, but I don't remember it), so maybe Lancelot's work offers something Pedro's is missing. However, Pedro never posted his work, so I can't post anything based on that branch yet (Copyright), but I've reached out to ask for permission. Anyway.... the summary of all this, is that I have a multi-step plan to change how -force-condition (and possibly thread/task/etc, we'll see) are handled, but it requires a bunch of setup, so I don't plan to combine these tasks (unless that becomes a requirement). > > Andrew> +/* Return true if STR starts with PREFIX, otherwise return false. */ > Andrew> + > Andrew> +static bool > Andrew> +startswith (const char *str, const token &prefix) > Andrew> +{ > Andrew> + return strncmp (str, prefix.data (), prefix.length ()) == 0; > Andrew> +} > > I wouldn't mind this in common-utils.h alongside the other startswith, > but it's fine here too. > > Andrew> + test (" if blah ", "blah"); > Andrew> + test (" if blah thread 1", "blah", global_thread_num); > Andrew> + test (" if blah inferior 1", "blah", -1, 1); > Andrew> + test (" if blah thread 1 ", "blah", global_thread_num); > > It seems like there are some confounding cases, where gdb would previous > (rightly) give a syntax error, but where now they may be passed through > without notice. > > I thought of some... maybe in the "don't care" bucket. I mean, it would > be nice if we could do a little better, since I guess the user could > typo and then wind up with a breakpoint that's never really useful, > like: > > break some_future_function -force-condition if x == thread 5 > > Will this ever yield an error? Maybe the user accidentally left out the > right-hand-side of that 'thread'. Or maybe meant to write "thread thread". > > Probably the worst cases involve deliberately confounding macro > definitions, but these IMO are definitely "don't care". Maybe advising > the user to surround the 'if' in parens and that's enough, assuming: > > break func if ( x == thread ) > > ... does the right thing. I'll add some new tests for these cases and dig into how they behave. Thank, Andrew > > Tom