RE: [PATCH v4 2/2] gdb/breakpoint: add flags to 'condition' and 'break' commands to force condition

Aktemur, Tankut Baris via Gdb-patches
2020-11-10T19:33:48Z

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.

> As is, this doesn't work, for example:
>
> (gdb) b -function main -force<TAB>

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.

Thanks.
-Baris

From 35c23da750cb4aa7fa6a6e1919a180fb13901fda Mon Sep 17 00:00:00 2001
From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
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 <tankut.baris.aktemur@intel.com>
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 <TAB>
-force-condition -function -label -line -qualified
-source if task thread

But tab-completing '-' lists only options:

(gdb) break -function main -<TAB>
-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 -<TAB>
-force-condition -function -label -line -qualified -source

gdb/ChangeLog:
2020-11-10 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

* completer.c (complete_explicit_location): Also add keywords
that start with '-' to the completion list.

gdb/testsuite/ChangeLog:
2020-11-10 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

* gdb.linespec/explicit.exp: Extend with a test to check completing
'-' after seemingly complete options.
---
gdb/completer.c | 6 +++++-
gdb/testsuite/gdb.linespec/explicit.exp | 14 ++++++++++++++
2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 262c8556bf6..cf8c14e7828 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -892,7 +892,11 @@ complete_explicit_location (completion_tracker &tracker,
int keyword = skip_keyword (tracker, explicit_options, &text);

if (keyword == -1)
- complete_on_enum (tracker, explicit_options, text, text);
+ {
+ complete_on_enum (tracker, explicit_options, text, text);
+ /* There are keywords that start with "-". Include them, too. */
+ complete_on_enum (tracker, linespec_keywords, text, text);
+ }
else
{
/* Completing on value. */
diff --git a/gdb/testsuite/gdb.linespec/explicit.exp b/gdb/testsuite/gdb.linespec/explicit.exp
index 52a1fce5371..c33ab505e34 100644
--- a/gdb/testsuite/gdb.linespec/explicit.exp
+++ b/gdb/testsuite/gdb.linespec/explicit.exp
@@ -469,6 +469,20 @@ namespace eval $testfile {
}
}

+ # Test that after a seemingly finished option argument,
+ # completion for "-" matches both the explicit location
+ # options and the linespec keywords that start with "-".
+ with_test_prefix "complete '-' after options" {
+ test_gdb_complete_multiple "b -function myfunction " "-" "" {
+ "-force-condition"
+ "-function"
+ "-label"
+ "-line"
+ "-qualified"
+ "-source"
+ }
+ }
+
# Tests that ensure that after "if" we complete on expressions
# are in cpcompletion.exp.

--
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928