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

[Pedro Alves <pedro@palves.net>]

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<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.

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 <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.

OK.