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 379C43858C2F for ; Wed, 13 Dec 2023 13:51:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 379C43858C2F 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 379C43858C2F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702475483; cv=none; b=jLBZI39rJFTqx6manuEs41X7zRaxUr/sSmDlN756KnpCp9pqZYoO4BUPZWfO1BUK9D800UqBH5wfJUyeU+KcmJFNpdm9nJdIFUY1oxLSDQXDohW2kSUP3zpoj6vLmBzv91xVZ7Wbm1M9S+SdgLy+MDCQ3VMHlsjUaxAOIMfBDME= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702475483; c=relaxed/simple; bh=J9+3VIA+vuyJjYjfJ1OQL9oSkpsN8PAFKr9oKy07j4A=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=TIuyf9xC2VnIteFXkDe3b7aCh7BD+MPgvj6lKiba9KRMreGdtT77/20llfKurBgj1b/c2hzHzKlxnkwWybQm4IyzXkvkwDpbd6l90c4uDx7q5VLgaqhaZEmEzzoSENjwpuOQnMWjHGCJTpXGQ7kEqSUW9zhNvrGY4W4xn/kIPss= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1702475480; 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=g12mLpi55Rzz0sVCZ4xemaUYLZUWm3omtrkra881D80=; b=MZ6DCcT9aV1SXbqXM8qMOTH67V2ermAc0N189LvQKaGGOTNfKnu2rKweKyjcOP9cuJhRrV ssgiV++fIdo5wo2Wm4fVmk9i9HSe8hgMCCJAdGa1KG6MVuz21RbUxLQYx0yZr7sw19m3cP w7Aztx3IiP3ttaJNxUYDyDeabMMPhBM= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-449-JoXlEwfUNlOQkNfZu-8UYg-1; Wed, 13 Dec 2023 08:51:19 -0500 X-MC-Unique: JoXlEwfUNlOQkNfZu-8UYg-1 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-332ee20a3f0so5874073f8f.0 for ; Wed, 13 Dec 2023 05:51:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702475478; x=1703080278; 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=g12mLpi55Rzz0sVCZ4xemaUYLZUWm3omtrkra881D80=; b=h0QPWgUAIKrRDWEm7GagIR+5uqRThfip8K8+3fojak0sNRrmoLBfpL/PFXf/6PhYOw h/zWJgZk5VqN4Bncko8COW+WfqxWkVBiDIORfN72uaEvduaDPOnTuaPBfvO0gdWPKSXP YYCC+fEWpWficis9q9d4xEvC9ltVDh8aQ4K8DvqlMVTP+peQAuuk9VgpEejYEvVpOu0z n3zA1aC51KV47gUFq7NbxCYfB9CObx/vqSdBylxK9TDdWybEXYu9H71gHNpeoOrNgtPD xxQMJL1ESDjDxTY6SdZH4iNomrDpAxhXuZw9ToYc1CnGLWoI9X6Gl0oKNiAS4r+H++qt Un8w== X-Gm-Message-State: AOJu0YzH7vtq7vuesw25Fe5EbeheZCNbBkfXE+iautRUGUVWwduLP6oX LNfV7KqZucIiSkyRSCDK8Y/sGFUsWOrakMOaTSsWd763srZsXiMA581eRORn3/+zMuO6FZxDIHv m3AHhaAByZZ5EdWadOQiN9XSKLQ9i+w== X-Received: by 2002:adf:e84d:0:b0:336:3f72:7f62 with SMTP id d13-20020adfe84d000000b003363f727f62mr194784wrn.16.1702475477904; Wed, 13 Dec 2023 05:51:17 -0800 (PST) X-Google-Smtp-Source: AGHT+IGKgDe6BdNrtlGA4XnocKUviggFtCxfY6fKANb6D0beGHV0H98RTIGIv22K3irnxJk8yoTZYA== X-Received: by 2002:adf:e84d:0:b0:336:3f72:7f62 with SMTP id d13-20020adfe84d000000b003363f727f62mr194780wrn.16.1702475477452; Wed, 13 Dec 2023 05:51:17 -0800 (PST) Received: from localhost (105.226.159.143.dyn.plus.net. [143.159.226.105]) by smtp.gmail.com with ESMTPSA id n6-20020a5d6606000000b00332ff137c29sm13478044wru.79.2023.12.13.05.51.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Dec 2023 05:51:16 -0800 (PST) From: Andrew Burgess To: "Aktemur, Tankut Baris" , "gdb-patches@sourceware.org" Cc: Eli Zaretskii Subject: RE: [PATCHv6 06/10] gdb: parse pending breakpoint thread/task immediately In-Reply-To: References: <2b291071a7869e0ca69aa89d3f197c68512439c1.1701513409.git.aburgess@redhat.com> Date: Wed, 13 Dec 2023 13:51:16 +0000 Message-ID: <87jzpii52j.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,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,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: "Aktemur, Tankut Baris" writes: > On Saturday, December 2, 2023 11:42 AM, Andrew Burgess wrote: >> diff --git a/gdb/break-cond-parse.c b/gdb/break-cond-parse.c >> new file mode 100644 >> index 00000000000..b59bd7aeeec >> --- /dev/null >> +++ b/gdb/break-cond-parse.c >> @@ -0,0 +1,463 @@ >> +/* Copyright (C) 2023 Free Software Foundation, Inc. >> + >> + This file is part of GDB. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +#include "defs.h" >> +#include "gdbsupport/gdb_assert.h" >> +#include "gdbsupport/selftest.h" >> +#include "test-target.h" >> +#include "scoped-mock-context.h" >> +#include "break-cond-parse.h" >> +#include "tid-parse.h" >> +#include "ada-lang.h" >> + >> +/* When parsing tokens from a string, which direction are we parsing? >> + >> + Given the following string and pointer 'ptr': >> + >> + ABC DEF GHI JKL >> + ^ >> + ptr >> + >> + Parsing 'forward' will return the token 'GHI' and update 'ptr' to point >> + between GHI and JKL. Parsing 'backward' will return the token 'DEF' and >> + update 'ptr' to point between ABC and DEF. >> +*/ >> + >> +enum class parse_direction >> +{ >> + /* Parse the next token forwards. */ >> + forward, >> + >> + /* Parse the previous token backwards. */ >> + backward >> +}; >> + >> +struct token >> +{ >> + /* Create a new token. START points to the first character of the new >> + token, while END points at the last character of the new token. >> + >> + Neither START or END can be nullptr, and both START and END must point >> + into the same C style string (i.e. there should be no null character >> + between START and END). END must be equal to, or greater than START, >> + that is, it is not possible to create a zero length token. */ >> + >> + token (const char *start, const char *end) > > Have you considered using std::string_view? > >> + : m_start (start), >> + m_end (end) >> + { >> + gdb_assert (m_end >= m_start); >> + gdb_assert (m_start + strlen (m_start) > m_end); >> + } >> + >> + /* Return a pointer to the first character of this token. */ >> + const char *start () const >> + { >> + return m_start; >> + } >> + >> + /* Return a pointer to the last character of this token. */ >> + const char *end () const >> + { >> + return m_end; >> + } >> + >> + /* Return the length of the token. */ >> + size_t length () const >> + { >> + /* The + 1 is because the character at m_end is part of the token. */ >> + return m_end - m_start + 1; >> + } >> + >> + /* Return true if this token matches STR. */ >> + bool matches (const char *str) const >> + { >> + return strncmp (m_start, str, length ()) == 0; >> + } >> + >> +private: >> + /* The first character of this token. */ >> + const char *m_start; >> + >> + /* The last character of this token. */ >> + const char *m_end; >> +}; >> + >> +/* Find the next token in DIRECTION from *CURR. */ >> + >> +static token >> +find_next_token (const char **curr, parse_direction direction) >> +{ >> + const char *tok_start, *tok_end; >> + >> + gdb_assert (**curr != '\0'); >> + >> + if (direction == parse_direction::forward) >> + { >> + *curr = skip_spaces (*curr); >> + tok_start = *curr; >> + *curr = skip_to_space (*curr); >> + tok_end = *curr - 1; >> + } >> + else >> + { >> + gdb_assert (direction == parse_direction::backward); >> + >> + while (isspace (**curr)) >> + --(*curr); >> + >> + tok_end = *curr; >> + >> + while (!isspace (**curr)) >> + --(*curr); >> + >> + tok_start = (*curr) + 1; >> + } >> + >> + return token (tok_start, tok_end); >> +} >> + >> +/* See break-cond-parse.h. */ >> + >> +void >> +create_breakpoint_parse_arg_string >> + (const char *tok, gdb::unique_xmalloc_ptr *cond_string, >> + int *thread, int *inferior, int *task, >> + gdb::unique_xmalloc_ptr *rest, bool *force) >> +{ >> + /* Set up the defaults. */ >> + cond_string->reset (); >> + rest->reset (); >> + *thread = -1; >> + *inferior = -1; >> + *task = -1; >> + *force = false; >> + >> + if (tok == nullptr) >> + return; >> + >> + /* The "-force-condition" flag gets some special treatment. If this >> + token is immediately after the condition string then we treat this as >> + part of the condition string rather than a separate token. This is >> + just a quirk of how this token used to be parsed, and has been >> + retained for backwards compatibility. Maybe this should be updated >> + in the future. */ > > (I was the author of the -force-condition patch.) > The quirks about the -force-condition flag were not a deliberate feature. > For whatever it's worth, I think it's OK to de-prioritize backwards > compatibility for this flag. I doubt that it would impact many users. > > In fact, in a post-merge comment, Pedro and Tom had suggested converting > the flag to an option rather than a keyword: > > https://sourceware.org/pipermail/gdb-patches/2020-October/172952.html > https://sourceware.org/pipermail/gdb-patches/2020-December/173802.html > https://sourceware.org/pipermail/gdb-patches/2020-December/173999.html > > I (embarrassingly) did not have the opportunity to get back to it. Maybe > it's now the correct time to address the concerns? I can gladly help. > What do you think? I think this sounds like a good idea, but .... ... I took a quick look at how this might be done, and the problem I see is that the current 'break' option parsing is buried within string_to_explicit_location_spec, which is called from string_to_location_spec, which is called from multiple commands. Which makes sense, currently, all of the options for 'break' are about specifying the location. In contrast, -force-condition has nothing to do with the location, but relates (as you know) to the condition. We could handle -force-condition in string_to_explicit_location_spec, but then commands like 'edit', which take a location, but not a condition would accept -force-condition, and we'd have to have some mechanism to the 'force' flag true/false value back out of string_to_explicit_location_spec, either passing a container around, or storing the 'force' state within the locspec... this all sounds like the wrong approach. Better I think would be to pull the option parsing out of string_to_explicit_location_spec, and move it into the individual commands. What we'd ideally want is to convert the current bespoke option parsing (for location spec options) to use GDB's generic option parsing mechanism. It's possible to bind multiple gdb::option::option_def into a single gdb::option::option_def_group, so we can imagine that the 'break' command would use a common gdb::option::option_def from location.h, which defines all the location spec arguments, and then a separate gdb::option::option_def which adds the -force-condition flag. The only issue then is that we'd need to pass the location spec related arguments into string_to_explicit_location_spec somehow.... Phew. Now the next problem is that the gdb::option::option_def contains a list of gdb::option::*_option_def objects, the precise type of which defines how the options are parsed. It we look at how the arguments to things like -function are parsed, these get special language specific handling, which the current option mechanism doesn't support, so I think we'd need to add that, which will mean at least extending the option process, or maybe even some reworking of the option handling... All that is to say that I agree with Pedro that this would be better done as an real option ... but I really don't want to tie this work to that refactoring if at all possible... Thanks, Andrew > > ... >> try >> { >> ops->create_sals_from_location_spec (locspec, &canonical); >> @@ -9285,6 +9122,13 @@ create_breakpoint (struct gdbarch *gdbarch, >> throw; >> } >> >> + /* The only bp_dprintf should have anything in EXTRA_STRING_COPY by this > > This sentence sounds odd. Did you mean "Only the bp_dprintf type should have..."? > >> + point. For all other breakpoints this indicates an error. We could >> + place this check earlier in the function, but we prefer to see errors >> + from the location spec parser before we see this error message. */ >> + if (type_wanted != bp_dprintf && extra_string_copy.get () != nullptr) >> + error (_("Garbage '%s' at end of command"), extra_string_copy.get ()); >> + >> if (!pending && canonical.lsals.empty ()) >> return 0; >> > > Regards > -Baris > > > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928