From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id iOOgJa/Dy1/yXAAAWB0awg (envelope-from ) for ; Sat, 05 Dec 2020 12:30:23 -0500 Received: by simark.ca (Postfix, from userid 112) id 96E741F0B8; Sat, 5 Dec 2020 12:30:23 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E17A31E590 for ; Sat, 5 Dec 2020 12:30:22 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CD4FF387088C; Sat, 5 Dec 2020 17:30:21 +0000 (GMT) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by sourceware.org (Postfix) with ESMTPS id D5235385802D for ; Sat, 5 Dec 2020 17:30:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D5235385802D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f68.google.com with SMTP id p8so8464353wrx.5 for ; Sat, 05 Dec 2020 09:30:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=rcr6Ir1dtI7xT85TpLs5EK7gb2RKN5MvdPcdRcHYFyQ=; b=HtD0bWXmWTQf96PEv68K93FHZ2Mbk7+w3eTgxrQuIJE68bxKbsK3htRGYSuvu/j3F0 P/nXxmv5xj5qMgDnA6UtF5p/MCwRDDtHl6d5Ihf/8klJQbni5jvhyAXRRxBLHT1HQaaB Kj0rHoSNWimYxtwWhQQc42kMhsq2ao8ccVoXOWEScqioAwXeNqM8l4DfIMJ6Sb9TjG3D 1IcrMgvPeRpSppEwDLuyuMG4OXGe1E6WPyCR38KSop5EbE6q9aQybNh8dRGKTfLEU/c1 eAaKho12Y74yR39yIa0AJOVENOmWPv1dGzWdFhDsY6tD9eOSspZZcpUOwg3r0ujI+spb 6gRQ== X-Gm-Message-State: AOAM530VWK6QbHIuMasZigtJ/6WCTWYNLfZXtxkVNg5IvCHKqhlL1JK9 RXTm9nO7KpaJV/hnL8MxFW8= X-Google-Smtp-Source: ABdhPJzgggpWVWTZ8xMqWYcx2jsvIBH41QnFq1eIzltFKTIBjMY1BXoaVms2Pg2reQLnQdMtnZA7sg== X-Received: by 2002:adf:e5c4:: with SMTP id a4mr11312293wrn.56.1607189417802; Sat, 05 Dec 2020 09:30:17 -0800 (PST) Received: from ?IPv6:2001:8a0:f91f:e900:1d90:d745:3c32:c159? ([2001:8a0:f91f:e900:1d90:d745:3c32:c159]) by smtp.gmail.com with ESMTPSA id h98sm8850723wrh.69.2020.12.05.09.30.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 05 Dec 2020 09:30:16 -0800 (PST) Subject: Re: [PATCH v4 2/2] gdb/breakpoint: add flags to 'condition' and 'break' commands to force condition To: "Aktemur, Tankut Baris" , "gdb-patches@sourceware.org" References: From: Pedro Alves Message-ID: <30187eea-cd9c-67c9-6914-d196d7a02f8c@palves.net> Date: Sat, 5 Dec 2020 17:30:15 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "simark@simark.ca" Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi Tankut, Sorry for the delay. On 11/10/20 7:33 PM, Aktemur, Tankut Baris wrote: > On Thursday, October 29, 2020 6:30 PM, Pedro Alves wrote: >> On 10/13/20 1:25 PM, Tankut Baris Aktemur via Gdb-patches wrote: >>> @@ -9192,10 +9207,25 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc, >>> if (toklen >= 1 && strncmp (tok, "if", toklen) == 0) >>> { >>> tok = cond_start = end_tok + 1; >>> - parse_exp_1 (&tok, pc, block_for_pc (pc), 0); >>> + try >>> + { >>> + parse_exp_1 (&tok, pc, block_for_pc (pc), 0); >>> + } >>> + catch (const gdb_exception_error &) >>> + { >>> + if (!force) >>> + throw; >>> + else >>> + tok = tok + strlen (tok); >>> + } >>> cond_end = tok; >>> *cond_string = savestring (cond_start, cond_end - cond_start); >>> } >>> + else if (toklen >= 1 && strncmp (tok, "-force-condition", toklen) == 0) >>> + { >>> + tok = cond_start = end_tok + 1; >>> + force = true; >>> + } >>> else if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0) >>> { >>> const char *tmptok; >> >> Is it important to handle "-force-condition" in this position, as opposed >> to making it another option handled by string_to_explicit_location ? > > I have mixed feelings about this. On one hand, it feels more natural to me that > "-force-condition" belongs to the keywords group (i.e. 'thread', 'task', 'if') > because it's about defining the condition rather than the location. On the other > hand, I agree that having it start with "-" gives it a flexible option feeling. > We could handle "-force-condition" like an option, but then this would not work: > > (gdb) break main thread 1 -force-condition if foo > > Once we start typing keywords (in this case, 'thread'), no more options are expected. > Perhaps making "thread" and "task" also start with "-" and converting them to an > option would improve consistency and flexibility. Calling "-force-condition" a keyword is a real stretch, IMO. For all intents and purposes, it's a flag, like "-qualified" is. Why wouldn't one be able to write: (gdb) break -force-condition main thread 1 if foo With that, a user could do: (gdb) alias bf = break -force-condition (gdb) bf if foo But it doesn't work: (gdb) bf main No default breakpoint address now. ... because GDB is parsing that as if the user has typed: (gdb) b if main No default breakpoint address now. Also, I find it hard to justify that this works: (gdb) break main -force-condition if foo while this doesn't: (gdb) break main -qualified So IMO, the feature is misdesigned -- "-force-condition" should be an option just like "-qualified" is, and then if you want to handle "-force-condition" after the linespec, then really all options should be handled in that position. Note also that this doesn't work: (gdb) b main -force-condit if 0 Function "main -force-condit" not defined. Make breakpoint pending on future shared library load? (y or [n]) ... which is yet another inconsistency compared to other flags, which can be abbreviated as long as unambiguous: (gdb) b -qual main Anyhow, the patch is in, and I'm not going to ask to revert it. Onward. > >> As is, this doesn't work, for example: >> >> (gdb) b -function main -force > > This is a bug. I propose a patch to fix it. Please see below. > >> >> nor does: >> >> (gdb) b -force-condition main if 0 >> invalid explicit location argument, "-force-condition" > > Based on the current implementation, this is expected because > '-force-condition' is considered a keyword. > >> IMO, ideally all '-' options would be handled in the same place. > > Like I wrote above, this is a trade-off. It gives more flexibility but also > brings some limitations. > >> At some point, I think it would be nice to convert the >> "break" command to use gdb::option, but it isn't trivial. > > Converting all keywords except "if" (currently "thread", "task", and "-force-condition") > to '-' options is IMHO a sweet-spot solution here. It's likely to be easier than > gdb::option, but also improves positional flexibility. However, backwards compatibility > would be a bit of pain. Breaking backwards compatibility here would be bad. But there's no real need for that. We would instead handle all options in the spot where we handle the keywords. > > Thanks. > -Baris > > > From 35c23da750cb4aa7fa6a6e1919a180fb13901fda Mon Sep 17 00:00:00 2001 > From: Tankut Baris Aktemur > Date: Tue, 10 Nov 2020 16:09:15 +0100 > Subject: [PATCH] gdb/completer: improve tab completion to consider the > '-force-condition' flag > > The commit > > commit 733d554a4625db4ffb89b7a20e1cf27ab071ef4d > Author: Tankut Baris Aktemur > Date: Tue Oct 27 10:56:03 2020 +0100 > > gdb/breakpoint: add flags to 'condition' and 'break' commands to force condition > > introduced the '-force-condition' flag to the 'break' command. This > flag was defined as a keyword like 'thread', 'task', and 'if'. > However, it starts with '-'. This difference caused an uncovered case > when tab-completing a seemingly complete linespec. > > Below, we see "-force-condition" in the completion list, where both > the options and the keywords are listed: > > (gdb) break -function main > -force-condition -function -label -line -qualified > -source if task thread > > But tab-completing '-' lists only options: > > (gdb) break -function main - > -function -label -line -qualified -source > > This patch fixes the problem by adding keywords to the completion > list, so that we see: > > (gdb) break -function main - > -force-condition -function -label -line -qualified -source > > gdb/ChangeLog: > 2020-11-10 Tankut Baris Aktemur > > * completer.c (complete_explicit_location): Also add keywords > that start with '-' to the completion list. > > gdb/testsuite/ChangeLog: > 2020-11-10 Tankut Baris Aktemur > > * gdb.linespec/explicit.exp: Extend with a test to check completing > '-' after seemingly complete options. OK.