[RFA] candidates for ambiguous command in upper case

Message ID 1484058324-5368-1-git-send-email-guitton@adacore.com
State New, archived
Headers

Commit Message

Jerome Guitton Jan. 10, 2017, 2:25 p.m. UTC
  If you type an ambiguous command in lower case, gdb tells the command
is ambiguous and tells you which one could match. If you type the same
but in upper case, gdb also says it is ambiguous, but shows an empty
list of commands:

 (gdb) ex
 Ambiguous command "ex": exec-file, expression.
 (gdb) EX
 Ambiguous command "EX": .

Simple fix in attachment, with an additional test.
Tested on x86-linux. OK to apply?

gdb/ChangeLog:

	* cli-decode.c (lookup_cmd): case insensitive match when looking
	up candidates for ambigous command.

gdb/testsuite/ChangeLog:

	* gdb.base/completion.exp: Add test for ambiguous upper case
	command.
---
 gdb/cli/cli-decode.c                  |    2 +-
 gdb/testsuite/gdb.base/completion.exp |   13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
  

Comments

Yao Qi Jan. 10, 2017, 3:07 p.m. UTC | #1
On 17-01-10 15:25:24, Jerome Guitton wrote:
> If you type an ambiguous command in lower case, gdb tells the command
> is ambiguous and tells you which one could match. If you type the same
> but in upper case, gdb also says it is ambiguous, but shows an empty
> list of commands:
> 
>  (gdb) ex
>  Ambiguous command "ex": exec-file, expression.
>  (gdb) EX
>  Ambiguous command "EX": .
> 

IMO, there is nothing wrong.  There is no command starts from "EX".

In bash, I type "make" and tab, I get,
$ make
make            makeglossaries  makeindex       makeinfo        makejvf         make-ssl-cert   make_strings

If type "MAKE", and type, I get,
$ MAKEDEV
  
Jerome Guitton Jan. 10, 2017, 3:19 p.m. UTC | #2
Yao Qi (qiyaoltc@gmail.com):

> IMO, there is nothing wrong.  There is no command starts from "EX".

This is a bit weird to accept upper-case EXEC-FIL then... isn't it?

(gdb) exec-fil
No executable file now.
(gdb) EXEC-FIL
No executable file now.
  
Simon Marchi Jan. 10, 2017, 3:28 p.m. UTC | #3
On 2017-01-10 10:19, Jerome Guitton wrote:
> Yao Qi (qiyaoltc@gmail.com):
> 
>> IMO, there is nothing wrong.  There is no command starts from "EX".
> 
> This is a bit weird to accept upper-case EXEC-FIL then... isn't it?
> 
> (gdb) exec-fil
> No executable file now.
> (gdb) EXEC-FIL
> No executable file now.

I agree that if GDB accepts commands in upper case, the ambiguous 
command message should work accordingly.  The message as it is, with an 
empty list, doesn't make sense.

I also noticed that tab completion was case sensitive, should that be 
fixed as well?

For example,

   (gdb) inf<tab><tab>

shows "inferior  info", but

   (gdb) INF<tab><tab>

shows nothing.  Should it show "inferior  info", or "INFERIOR INFO"?
  
Jerome Guitton Jan. 10, 2017, 3:40 p.m. UTC | #4
Simon Marchi (simon.marchi@polymtl.ca):

> I also noticed that tab completion was case sensitive, should that
> be fixed as well?

I guess so. It seems that it could be changed by a similar patch in
complete_on_cmdlist.
  
Pedro Alves Jan. 10, 2017, 5 p.m. UTC | #5
On 01/10/2017 03:28 PM, Simon Marchi wrote:
> On 2017-01-10 10:19, Jerome Guitton wrote:
>> Yao Qi (qiyaoltc@gmail.com):
>>
>>> IMO, there is nothing wrong.  There is no command starts from "EX".
>>
>> This is a bit weird to accept upper-case EXEC-FIL then... isn't it?
>>
>> (gdb) exec-fil
>> No executable file now.
>> (gdb) EXEC-FIL
>> No executable file now.
> 
> I agree that if GDB accepts commands in upper case, the ambiguous
> command message should work accordingly.

Agreed.  I thought that the manual mentioned that gdb accepts
commands in either case, but I can't find it now.

> The message as it is, with an empty list, doesn't make sense.
> 
> I also noticed that tab completion was case sensitive, should that be
> fixed as well?
> 
> For example,
> 
>   (gdb) inf<tab><tab>
> 
> shows "inferior  info", but
> 
>   (gdb) INF<tab><tab>
> 
> shows nothing.  Should it show "inferior  info", or "INFERIOR INFO"?

I think the former.  I.e., show the canonical lowercase.  That
is likely to end up much easier and saner to implement.

I've been playing with completion recently [1], and I've noticed
that

 (gdb) handle sigu<tab>

completes to:

 (gdb) handle SIGU<bell>

 (gdb) handle sig<tab>

Or really:

 (gdb) handle sigu M-?
 SIGURG   SIGUSR1  SIGUSR2  

In my branch I've completely changed how GDB hands over
completion matches to readline, and I've had to make
sure to preserve that behavior, as caught by some test.

[1] - this WIP branch gets rid of the need to quote
      linespecs as in "b 'function(int<tab>" among other things:

  https://github.com/palves/gdb/commits/palves/cp-linespec

Thanks,
Pedro Alves
  
Yao Qi Jan. 11, 2017, 5:25 p.m. UTC | #6
On 17-01-10 17:00:28, Pedro Alves wrote:
> On 01/10/2017 03:28 PM, Simon Marchi wrote:
> > On 2017-01-10 10:19, Jerome Guitton wrote:
> >> Yao Qi (qiyaoltc@gmail.com):
> >>
> >>> IMO, there is nothing wrong.  There is no command starts from "EX".
> >>
> >> This is a bit weird to accept upper-case EXEC-FIL then... isn't it?
> >>
> >> (gdb) exec-fil
> >> No executable file now.
> >> (gdb) EXEC-FIL
> >> No executable file now.
> > 
> > I agree that if GDB accepts commands in upper case, the ambiguous
> > command message should work accordingly.
> 
> Agreed.  I thought that the manual mentioned that gdb accepts
> commands in either case, but I can't find it now.
> 

I don't find gdb accepts commands in either case in the manual, and
I am surprised that gdb does so.  Actually, gdb does so since 1988!

commit 7b4ac7e1ed2c4616bce56d1760807798be87ac9e
Author: gdb-2.4+.aux.coff <gdb@fsf.org>
Date:   Sat Jan 16 04:39:57 1988 +0000

    gdb-2.4+.aux.coff

in lookup_cmd function,

+  /* Find end of command name.  */
+
+  p = *line;
+  while (*p == '-'
+        || (*p >= 'a' && *p <= 'z')
+        || (*p >= 'A' && *p <= 'Z')
+        || (*p >= '1' && *p <= '9'))
+    {
+      if (*p >= 'A' && *p <= 'Z')
+       *p += 'a' - 'A';
+      p++;
+    }

however, I don't see any reason to do so.  At least, we need to be
clear that whether gdb accepts upper case commands or not.
  
Luis Machado Jan. 11, 2017, 5:34 p.m. UTC | #7
On 01/11/2017 11:25 AM, Yao Qi wrote:
> On 17-01-10 17:00:28, Pedro Alves wrote:
>> On 01/10/2017 03:28 PM, Simon Marchi wrote:
>>> On 2017-01-10 10:19, Jerome Guitton wrote:
>>>> Yao Qi (qiyaoltc@gmail.com):
>>>>
>>>>> IMO, there is nothing wrong.  There is no command starts from "EX".
>>>>
>>>> This is a bit weird to accept upper-case EXEC-FIL then... isn't it?
>>>>
>>>> (gdb) exec-fil
>>>> No executable file now.
>>>> (gdb) EXEC-FIL
>>>> No executable file now.
>>>
>>> I agree that if GDB accepts commands in upper case, the ambiguous
>>> command message should work accordingly.
>>
>> Agreed.  I thought that the manual mentioned that gdb accepts
>> commands in either case, but I can't find it now.
>>
>
> I don't find gdb accepts commands in either case in the manual, and
> I am surprised that gdb does so.  Actually, gdb does so since 1988!
>
> commit 7b4ac7e1ed2c4616bce56d1760807798be87ac9e
> Author: gdb-2.4+.aux.coff <gdb@fsf.org>
> Date:   Sat Jan 16 04:39:57 1988 +0000
>
>     gdb-2.4+.aux.coff
>
> in lookup_cmd function,
>
> +  /* Find end of command name.  */
> +
> +  p = *line;
> +  while (*p == '-'
> +        || (*p >= 'a' && *p <= 'z')
> +        || (*p >= 'A' && *p <= 'Z')
> +        || (*p >= '1' && *p <= '9'))
> +    {
> +      if (*p >= 'A' && *p <= 'Z')
> +       *p += 'a' - 'A';
> +      p++;
> +    }
>
> however, I don't see any reason to do so.  At least, we need to be
> clear that whether gdb accepts upper case commands or not.
>

I think it makes for a more coherent command-handling environment. So 
load would be the same as LOAD, Load, LoAd, lOaD.

I can't think of a scenario where differentiating uppercase/lowercase 
commands in GDB's CLI would make a difference. Though i've noticed that 
MI commands are case-sensitive, which is incoherent. We ought to fix 
that too.
  
Pedro Alves Jan. 11, 2017, 8:24 p.m. UTC | #8
On 01/11/2017 05:25 PM, Yao Qi wrote:
> On 17-01-10 17:00:28, Pedro Alves wrote:
>> On 01/10/2017 03:28 PM, Simon Marchi wrote:
>>> On 2017-01-10 10:19, Jerome Guitton wrote:
>>>> Yao Qi (qiyaoltc@gmail.com):
>>>>
>>>>> IMO, there is nothing wrong.  There is no command starts from "EX".
>>>>
>>>> This is a bit weird to accept upper-case EXEC-FIL then... isn't it?
>>>>
>>>> (gdb) exec-fil
>>>> No executable file now.
>>>> (gdb) EXEC-FIL
>>>> No executable file now.
>>>
>>> I agree that if GDB accepts commands in upper case, the ambiguous
>>> command message should work accordingly.
>>
>> Agreed.  I thought that the manual mentioned that gdb accepts
>> commands in either case, but I can't find it now.
>>
> 
> I don't find gdb accepts commands in either case in the manual, and
> I am surprised that gdb does so.  Actually, gdb does so since 1988!

I guess I probably discovered it by typing some command with
Caps Lock on by accident, and seeing it work.

I can only guess on original motivation -- maybe to make GDB usable
with uppercase-only terminals?  Those were common at some point in
the past.  I think Linux's terminal subsystem might still have
support for those.   But then symbol searching was not case-insensitive,
("set case-sensitive off" support was only added much later), so I
can't quite imagine how that'd be much usable.

Thanks,
Pedro Alves
  
Jerome Guitton Jan. 12, 2017, 10:18 a.m. UTC | #9
Pedro Alves (palves@redhat.com):

> I can only guess on original motivation -- maybe to make GDB usable
> with uppercase-only terminals?  Those were common at some point in
> the past.  I think Linux's terminal subsystem might still have
> support for those.   But then symbol searching was not case-insensitive,
> ("set case-sensitive off" support was only added much later), so I
> can't quite imagine how that'd be much usable.

I'm not sure of the original motivation; but I guess that if it has
been there for thirty years it is now considered as the standard
interface by a few users.

In any case, even if we decide to change that, consistency would be
good, at least for CLI; the same policy for casing should apply in all
cases of completion.

Documenting it in the user manual would be good indeed; I'm not sure
where the proper place would be though.
  
Pedro Alves Jan. 12, 2017, 4:37 p.m. UTC | #10
On 01/12/2017 10:18 AM, Jerome Guitton wrote:
> Pedro Alves (palves@redhat.com):
> 
>> I can only guess on original motivation -- maybe to make GDB usable
>> with uppercase-only terminals?  Those were common at some point in
>> the past.  I think Linux's terminal subsystem might still have
>> support for those.   But then symbol searching was not case-insensitive,
>> ("set case-sensitive off" support was only added much later), so I
>> can't quite imagine how that'd be much usable.
> 
> I'm not sure of the original motivation; but I guess that if it has
> been there for thirty years it is now considered as the standard
> interface by a few users.

Dunno, if it doesn't make sense, and nobody uses it, then it's one
less thing to maintain and test.  We don't treat command options
as case insensitive.  E.g.:

 (gdb) watch -locaTION *main
 No symbol "locaTION" in current context.

 (gdb) p /x obj
 $1 = 0x4007a0
 (gdb) p /X obj
 No symbol "X" in current context.

Etc.

If we removed support for case insensitive commands, then it may
open up interesting uses.

> In any case, even if we decide to change that, consistency would be
> good, at least for CLI; the same policy for casing should apply in all
> cases of completion.
> 
> Documenting it in the user manual would be good indeed; I'm not sure
> where the proper place would be though.

The place where I had expected to find it was where we talk about
unambiguous command abbreviations.  I.e., the very first thing
the manual talks about, once GDB is up and running:

 https://sourceware.org/gdb/current/onlinedocs/gdb/Commands.html#Commands

Thanks,
Pedro Alves
  
Jerome Guitton Jan. 16, 2017, 4:32 p.m. UTC | #11
Pedro Alves (palves@redhat.com):

> Dunno, if it doesn't make sense, and nobody uses it, then it's one
> less thing to maintain and test.

Hard to figure out if someone uses it, I guess. This originally got
reported by one of our users as a minor inconsistancy, who also said
that he would be OK if gdb commands were case sensitive.

The patch that I'm suggesting is fairly simple, but my feeling is that
it would be just as easy to remove the feature.

I've tested the opposite: making the match case sensitive is a matter
of removing 4 characters in cli-decode.c. Not much consequences in the
testsuite. "handle sigq" still completes to "handle SIGQUIT".

So, what do we want to do?
1. Remove the feature?
2. Improve its consistency?
3. Keep things as is?
  
Pedro Alves Jan. 17, 2017, 1:58 a.m. UTC | #12
On 01/16/2017 04:32 PM, Jerome Guitton wrote:
> Pedro Alves (palves@redhat.com):
> 
>> Dunno, if it doesn't make sense, and nobody uses it, then it's one
>> less thing to maintain and test.
> 
> Hard to figure out if someone uses it, I guess. This originally got
> reported by one of our users as a minor inconsistancy, who also said
> that he would be OK if gdb commands were case sensitive.
> 
> The patch that I'm suggesting is fairly simple, but my feeling is that
> it would be just as easy to remove the feature.
> 
> I've tested the opposite: making the match case sensitive is a matter
> of removing 4 characters in cli-decode.c. Not much consequences in the
> testsuite. "handle sigq" still completes to "handle SIGQUIT".
> 
> So, what do we want to do?
> 1. Remove the feature?
> 2. Improve its consistency?
> 3. Keep things as is?
> 

I vote #1.

Thanks,
Pedro Alves
  
Luis Machado Jan. 17, 2017, 4:29 p.m. UTC | #13
On 01/16/2017 07:58 PM, Pedro Alves wrote:
> On 01/16/2017 04:32 PM, Jerome Guitton wrote:
>> Pedro Alves (palves@redhat.com):
>>
>>> Dunno, if it doesn't make sense, and nobody uses it, then it's one
>>> less thing to maintain and test.
>>
>> Hard to figure out if someone uses it, I guess. This originally got
>> reported by one of our users as a minor inconsistancy, who also said
>> that he would be OK if gdb commands were case sensitive.
>>
>> The patch that I'm suggesting is fairly simple, but my feeling is that
>> it would be just as easy to remove the feature.
>>
>> I've tested the opposite: making the match case sensitive is a matter
>> of removing 4 characters in cli-decode.c. Not much consequences in the
>> testsuite. "handle sigq" still completes to "handle SIGQUIT".
>>
>> So, what do we want to do?
>> 1. Remove the feature?
>> 2. Improve its consistency?
>> 3. Keep things as is?
>>
>
> I vote #1.

I don't see a problem with #1 as long as we keep it consistent 
throughout from now on.
  
Pedro Alves Jan. 17, 2017, 4:34 p.m. UTC | #14
On 01/17/2017 04:29 PM, Luis Machado wrote:
> 
> I don't see a problem with #1 as long as we keep it consistent
> throughout from now on.

Can you expand on exactly what you mean by the "as long" part?

Thanks,
Pedro Alves
  
Luis Machado Jan. 17, 2017, 4:51 p.m. UTC | #15
On 01/17/2017 10:34 AM, Pedro Alves wrote:
> On 01/17/2017 04:29 PM, Luis Machado wrote:
>>
>> I don't see a problem with #1 as long as we keep it consistent
>> throughout from now on.
>
> Can you expand on exactly what you mean by the "as long" part?
>
> Thanks,
> Pedro Alves
>
>

Meaning that, if we go with #1, we should make sure we are not doing 
case-insensitive comparisons in gdb unless it is clearly stated we 
should do it (like handling of Windows paths i suppose).

For the record, i think #3 is not acceptable and we don't seem to have a 
good justification for keeping the behavior of #2 other than "we've been 
doing this since ...".

So i think #1 would be the best. I just wanted to point out we may want 
to make sure there are no other gdb places with the same undocumented 
behavior.

Does that make sense?
  
Pedro Alves Jan. 17, 2017, 5:04 p.m. UTC | #16
On 01/17/2017 04:51 PM, Luis Machado wrote:
> On 01/17/2017 10:34 AM, Pedro Alves wrote:
>> On 01/17/2017 04:29 PM, Luis Machado wrote:
>>>
>>> I don't see a problem with #1 as long as we keep it consistent
>>> throughout from now on.
>>
>> Can you expand on exactly what you mean by the "as long" part?

> Meaning that, if we go with #1, we should make sure we are not doing
> case-insensitive comparisons in gdb unless it is clearly stated we
> should do it (like handling of Windows paths i suppose).

Would you say that we should stop completing "handle sig" to "handle SIG"?
"handle" doesn't accept lowercase signal names, but it could be
argued that the completer is helping the user here.

Grepping for strncasecmp doesn't find all that many hits, and several
are in symbol lookup or path handling stuff where they're probably rightly
used.  

The ones in tracepoint.c do look like of the inconsistent-user-interface sort.
I had never realized we accept $REG etc. in tracepoint actions.

Thanks,
Pedro Alves
  
Luis Machado Jan. 17, 2017, 5:13 p.m. UTC | #17
On 01/17/2017 11:04 AM, Pedro Alves wrote:
> On 01/17/2017 04:51 PM, Luis Machado wrote:
>> On 01/17/2017 10:34 AM, Pedro Alves wrote:
>>> On 01/17/2017 04:29 PM, Luis Machado wrote:
>>>>
>>>> I don't see a problem with #1 as long as we keep it consistent
>>>> throughout from now on.
>>>
>>> Can you expand on exactly what you mean by the "as long" part?
>
>> Meaning that, if we go with #1, we should make sure we are not doing
>> case-insensitive comparisons in gdb unless it is clearly stated we
>> should do it (like handling of Windows paths i suppose).
>
> Would you say that we should stop completing "handle sig" to "handle SIG"?
> "handle" doesn't accept lowercase signal names, but it could be
> argued that the completer is helping the user here.
>

That sounds like a one-off case where it is useful.

> Grepping for strncasecmp doesn't find all that many hits, and several
> are in symbol lookup or path handling stuff where they're probably rightly
> used.
>

I'll go through the code to see if there are any other occurrences.

> The ones in tracepoint.c do look like of the inconsistent-user-interface sort.
> I had never realized we accept $REG etc. in tracepoint actions.
>

Neither did i. This one looks like it could be dropped.
  

Patch

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index d3be93c..d59fe9b 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1550,7 +1550,7 @@  lookup_cmd (const char **line, struct cmd_list_element *list, char *cmdtype,
 
 	  ambbuf[0] = 0;
 	  for (c = local_list; c; c = c->next)
-	    if (!strncmp (*line, c->name, amb_len))
+	    if (!strncasecmp (*line, c->name, amb_len))
 	      {
 		if (strlen (ambbuf) + strlen (c->name) + 6
 		    < (int) sizeof ambbuf)
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 4a3ee4b..9a7a221 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -265,6 +265,19 @@  gdb_test_multiple "" "$test" {
     }
 }
 
+set test "complete 'info T '"
+send_gdb "info T \t"
+gdb_test_multiple "" "$test" {
+    -re "^info T \\\x07$" {
+	send_gdb "\n"
+	gdb_test_multiple "" "$test" {
+	    -re "Ambiguous info command \"T \": target, tasks, terminal, threads, tp, tracepoints, tvariables, (type-printers, )?types\\..*$gdb_prompt $" {
+		pass "$test"
+	    }
+	}
+    }
+}
+
 set test "complete 'info t '"
 send_gdb "info t \t"
 gdb_test_multiple "" "$test" {