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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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