[PATCH] Fix 'Undefined command' error message

[PATCH] Fix 'Undefined command' error message

[Andrew STUBBS]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Hi,

The following demonstrates the problem:

   (gdb) abc_def
   Undefined command: "abc".  Try "help".

Non-alphanumeric characters, such as underscore, are permitted in 
command names (including user defined commands), but are not understood 
when giving error messages.

The attached patch fixes the problem.

OK?

Andrew Stubbs

[-- Attachment #2: cli-decode.patch --]
[-- Type: text/plain, Size: 844 bytes --]

2005-11-15  Andrew Stubbs  <andrew.stubbs@st.com>

	* cli-decode.c (lookup_cmd): Allow all the same characters in
	command names that lookup_cmd_composition() does.

Index: src/gdb/cli/cli-decode.c
===================================================================
--- src.orig/gdb/cli/cli-decode.c	2005-05-26 21:49:02.000000000 +0100
+++ src/gdb/cli/cli-decode.c	2005-11-09 11:39:52.000000000 +0000
@@ -1196,7 +1196,12 @@ lookup_cmd (char **line, struct cmd_list
 	    {
 	      char *p = *line, *q;
 
-	      while (isalnum (*p) || *p == '-')
+	      while (*p && (isalnum (*p) || *p == '-' || *p == '_' ||
+#if defined(TUI)
+			    (tui_active &&
+			     (*p == '+' || *p == '<' || *p == '>' || *p == '$')) ||
+#endif
+			    (xdb_commands && (*p == '!' || *p == '/' || *p == '?'))))
 		p++;
 
 	      q = (char *) alloca (p - *line + 1);

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Tue, 15 Nov 2005 14:09:14 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> 
> -	      while (isalnum (*p) || *p == '-')
> +	      while (*p && (isalnum (*p) || *p == '-' || *p == '_' ||

Why is there a need for testing *p to be non-zero?  AFAIK, isalnum is
well defined for a zero argument.

> +#if defined(TUI)
> +			    (tui_active &&
> +			     (*p == '+' || *p == '<' || *p == '>' || *p == '$')) ||
> +#endif

What is the reason for additional tests under TUI?

> +			    (xdb_commands && (*p == '!' || *p == '/' || *p == '?'))))

Is this for some kind of compatibility with XDB?

(Yes, I know that you simply lifted code from elsewhere in
cli-decode.c, but still...  At the very least, the non-trivial
characters warrant a comment that explains why they are there.)

Re: [PATCH] Fix 'Undefined command' error message

[Andrew STUBBS]

Eli Zaretskii wrote:
> Why is there a need for testing *p to be non-zero?  AFAIK, isalnum is
> well defined for a zero argument.
 >
> What is the reason for additional tests under TUI?
> 
> Is this for some kind of compatibility with XDB?

I do not know the answer to these questions, although I'm sure I could 
look into it. There are already two instances of this code in this file 
and each is implemented identically to the others.

The reason the code is like this here is because it must replicate the 
tests in lookup_cmd_1 if it is to give the right message. I do not know 
why lookup_cmd_1 is the way it is.

If you prefer I would be happy to pull out the code into a static function.

Andrew

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Wed, 16 Nov 2005 13:02:04 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> Cc: gdb-patches@sources.redhat.com
> 
> Eli Zaretskii wrote:
> > Why is there a need for testing *p to be non-zero?  AFAIK, isalnum is
> > well defined for a zero argument.
>  >
> > What is the reason for additional tests under TUI?
> > 
> > Is this for some kind of compatibility with XDB?
> 
> I do not know the answer to these questions, although I'm sure I could 
> look into it. There are already two instances of this code in this file 
> and each is implemented identically to the others.

At least the test for *p non-zero should be removed, I think.

> The reason the code is like this here is because it must replicate the 
> tests in lookup_cmd_1 if it is to give the right message. I do not know 
> why lookup_cmd_1 is the way it is.

Anyone?

Re: [PATCH] Fix 'Undefined command' error message

[Daniel Jacobowitz]

On Tue, Nov 15, 2005 at 08:13:38PM +0200, Eli Zaretskii wrote:
> > Date: Tue, 15 Nov 2005 14:09:14 +0000
> > From: Andrew STUBBS <andrew.stubbs@st.com>
> > 
> > -	      while (isalnum (*p) || *p == '-')
> > +	      while (*p && (isalnum (*p) || *p == '-' || *p == '_' ||
> 
> Why is there a need for testing *p to be non-zero?  AFAIK, isalnum is
> well defined for a zero argument.

Just to clarify when the end of the loop is?  You're right, it should
not be necessary.

> > +#if defined(TUI)
> > +			    (tui_active &&
> > +			     (*p == '+' || *p == '<' || *p == '>' || *p == '$')) ||
> > +#endif
> 
> What is the reason for additional tests under TUI?

The TUI has commands which include these.  E.g. + is scroll forward;
search for add_com in tui/.

> > +			    (xdb_commands && (*p == '!' || *p == '/' || *p == '?'))))
> 
> Is this for some kind of compatibility with XDB?

And xdb has some commands which include these, which GDB emulates, I
presume.  ! is for an xdb-specific alias to shell.  (With a comment
from Andrew above it mentioning that using a different syntax would be
more useful.  Personally, I think we don't need to bind ! to that so
much as we need to bind it to bash-style command history.)

/ and ? are bound to forward and reverse search in the source file.

Found these with grep also, although it took me a little longer.

Does anyone use xdb compatibility any more?  I suspect we could remove
it, but I've no way to know.

The other characters could presumably be allowed unconditionally
without doing any harm, but I haven't tried that.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Wed, 16 Nov 2005 15:13:00 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Andrew STUBBS <andrew.stubbs@st.com>, gdb-patches@sources.redhat.com
> 
> > > +#if defined(TUI)
> > > +			    (tui_active &&
> > > +			     (*p == '+' || *p == '<' || *p == '>' || *p == '$')) ||
> > > +#endif
> > 
> > What is the reason for additional tests under TUI?
> 
> The TUI has commands which include these.  E.g. + is scroll forward;
> search for add_com in tui/.

I'd prefer to have the same characters allowed in all CLI commands.
Any other way would be very confusing to users.

Thanks for the other clarifications.  I think we should at least add
comments that explain the respective characters.

Re: [PATCH] Fix 'Undefined command' error message

[Andrew STUBBS]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

Eli Zaretskii wrote:
> I'd prefer to have the same characters allowed in all CLI commands.
> Any other way would be very confusing to users.
> 
> Thanks for the other clarifications.  I think we should at least add
> comments that explain the respective characters.

I have moved all three instances of this code in to one function, added 
comments, removed the pointless '*p' and allowed the TUI characters all 
the time.

The XDB characters clash with existing GDB commands so I have left those 
disabled unless in XDB mode. Specifically commands such as 'x/i' fail 
because the command there is actually only 'x'. Obviously this does not 
affect commands such as 'x /i'.

Andrew Stubbs

[-- Attachment #2: cli-decode-2.patch --]
[-- Type: text/plain, Size: 5520 bytes --]

2005-11-21  Andrew Stubbs  <andrew.stubbs@st.com>

	* cli-decode.c (find_command_name_length): New function.
	(lookup_cmd_1): Replace loop reading command name with
	find_command_name_length().
	(lookup_cmd): Likewise.
	(lookup_cmd_composition): Likewise.

Index: src/gdb/cli/cli-decode.c
===================================================================
--- src.orig/gdb/cli/cli-decode.c	2005-11-18 16:30:29.000000000 +0000
+++ src/gdb/cli/cli-decode.c	2005-11-21 16:41:02.000000000 +0000
@@ -977,6 +977,31 @@ find_cmd (char *command, int len, struct
   return found;
 }
 
+static int
+find_command_name_length (const char *text)
+{
+  const char *p = text;
+
+  /* Treating underscores as part of command words is important
+     so that "set args_foo()" doesn't get interpreted as
+     "set args _foo()".  */
+  /* Some characters are only used for TUI specific commands. However, they
+     are always allowed for the sake of consistency.
+     The XDB compatibility characters are only allowed when using the right
+     mode because they clash with other GDB commands - specifically '/' is
+     used as a suffix for print, examine and display.
+     Note that this is larger than the character set allowed when creating
+     user-defined commands.  */
+  while (isalnum (*p) || *p == '-' || *p == '_' ||
+	 /* Characters used by TUI specific commands.  */
+	 *p == '+' || *p == '<' || *p == '>' || *p == '$' ||
+	 /* Characters used for XDB compatibility.  */
+	 (xdb_commands && (*p == '!' || *p == '/' || *p == '?')))
+    p++;
+
+  return p - text;
+}
+
 /* This routine takes a line of TEXT and a CLIST in which to start the
    lookup.  When it returns it will have incremented the text pointer past
    the section of text it matched, set *RESULT_LIST to point to the list in
@@ -1017,7 +1042,7 @@ struct cmd_list_element *
 lookup_cmd_1 (char **text, struct cmd_list_element *clist,
 	      struct cmd_list_element **result_list, int ignore_help_classes)
 {
-  char *p, *command;
+  char *command;
   int len, tmp, nfound;
   struct cmd_list_element *found, *c;
   char *line = *text;
@@ -1025,27 +1050,13 @@ lookup_cmd_1 (char **text, struct cmd_li
   while (**text == ' ' || **text == '\t')
     (*text)++;
 
-  /* Treating underscores as part of command words is important
-     so that "set args_foo()" doesn't get interpreted as
-     "set args _foo()".  */
-  /* NOTE: cagney/2003-02-13 The `tui_active' was previously
-     `tui_version'.  */
-  for (p = *text;
-       *p && (isalnum (*p) || *p == '-' || *p == '_' ||
-#if defined(TUI)
-	      (tui_active &&
-	       (*p == '+' || *p == '<' || *p == '>' || *p == '$')) ||
-#endif
-	      (xdb_commands && (*p == '!' || *p == '/' || *p == '?')));
-       p++)
-    ;
+  /* Identify the name of the command.  */
+  len = find_command_name_length (*text);
 
   /* If nothing but whitespace, return 0.  */
-  if (p == *text)
+  if (len == 0)
     return 0;
 
-  len = p - *text;
-
   /* *text and p now bracket the first command word to lookup (and
      it's length is len).  We copy this into a local temporary */
 
@@ -1092,7 +1103,7 @@ lookup_cmd_1 (char **text, struct cmd_li
 
   /* We've matched something on this list.  Move text pointer forward. */
 
-  *text = p;
+  *text += len;
 
   if (found->cmd_pointer)
     {
@@ -1194,14 +1205,12 @@ lookup_cmd (char **line, struct cmd_list
 	    error (_("Lack of needed %scommand"), cmdtype);
 	  else
 	    {
-	      char *p = *line, *q;
+	      char *q;
+	      int len = find_command_name_length (*line);
 
-	      while (isalnum (*p) || *p == '-')
-		p++;
-
-	      q = (char *) alloca (p - *line + 1);
-	      strncpy (q, *line, p - *line);
-	      q[p - *line] = '\0';
+	      q = (char *) alloca (len + 1);
+	      strncpy (q, *line, len);
+	      q[len] = '\0';
 	      undef_cmd_error (cmdtype, q);
 	    }
 	}
@@ -1379,7 +1388,7 @@ lookup_cmd_composition (char *text,
                       struct cmd_list_element **prefix_cmd, 
                       struct cmd_list_element **cmd)
 {
-  char *p, *command;
+  char *command;
   int len, tmp, nfound;
   struct cmd_list_element *cur_list;
   struct cmd_list_element *prev_cmd;
@@ -1399,28 +1408,14 @@ lookup_cmd_composition (char *text,
       while (*text == ' ' || *text == '\t')
       (text)++;
       
-      /* Treating underscores as part of command words is important
-       so that "set args_foo()" doesn't get interpreted as
-       "set args _foo()".  */
-      /* NOTE: cagney/2003-02-13 The `tui_active' was previously
-	 `tui_version'.  */
-      for (p = text;
-         *p && (isalnum (*p) || *p == '-' || *p == '_' ||
-#if defined(TUI)
-                (tui_active &&
-                 (*p == '+' || *p == '<' || *p == '>' || *p == '$')) ||
-#endif
-                (xdb_commands && (*p == '!' || *p == '/' || *p == '?')));
-         p++)
-      ;
+      /* Identify the name of the command.  */
+      len = find_command_name_length (text);
       
       /* If nothing but whitespace, return.  */
-      if (p == text)
-      return 0;
-      
-      len = p - text;
+      if (len == 0)
+	return 0;
       
-      /* text and p now bracket the first command word to lookup (and
+      /* text is the start of the first command word to lookup (and
        it's length is len).  We copy this into a local temporary */
       
       command = (char *) alloca (len + 1);
@@ -1473,7 +1468,7 @@ lookup_cmd_composition (char *text,
       else
       return 1;
       
-      text = p;
+      text += len;
     }
 }
 

Re: [PATCH] Fix 'Undefined command' error message

[Andreas Schwab]

Andrew STUBBS <andrew.stubbs@st.com> writes:

> The XDB characters clash with existing GDB commands so I have left those 
> disabled unless in XDB mode. Specifically commands such as 'x/i' fail 
> because the command there is actually only 'x'. Obviously this does not 
> affect commands such as 'x /i'.

The same clash exists with '+'.  Things like "u+1" won't work any more,
IIUC.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix 'Undefined command' error message

[Andrew STUBBS]

Andreas Schwab wrote:
> Andrew STUBBS <andrew.stubbs@st.com> writes:
> 
>>The XDB characters clash with existing GDB commands so I have left those 
>>disabled unless in XDB mode. Specifically commands such as 'x/i' fail 
>>because the command there is actually only 'x'. Obviously this does not 
>>affect commands such as 'x /i'.
> 
> The same clash exists with '+'.  Things like "u+1" won't work any more,
> IIUC.

Hmmm, this looks more like a parser exploit than a feature. Either that 
or the TUI can be considered broken.

Should I leave these characters as TUI only or always allow them? Does 
anybody have a strong opinion either way? I would tend to favour leaving 
it as is.

Andrew

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Wed, 23 Nov 2005 15:56:07 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
> 
> > The same clash exists with '+'.  Things like "u+1" won't work any more,
> > IIUC.
> 
> Hmmm, this looks more like a parser exploit than a feature. Either that 
> or the TUI can be considered broken.
> 
> Should I leave these characters as TUI only or always allow them?

What TUI commands use these problematic characters, and how does TUI
avoid the problems pointed out by Andreas?

> Does anybody have a strong opinion either way?

I will make up my mind after I understand the answers to my questions
above.

> I would tend to favour leaving it as is.

That's the fallback, yes.  But I'd like to see if we can do better.

Re: [PATCH] Fix 'Undefined command' error message

[Andrew STUBBS]

Eli Zaretskii wrote:
> What TUI commands use these problematic characters, and how does TUI
> avoid the problems pointed out by Andreas?

I don't know what uses '$', but the other three, '>', '<' and '+', are 
commands in their own right and are used the move the source window 
right, left and up through the file. '-' moves down. There may be other 
commands I don't know about.

The TUI does *not* avoid the problems pointed out by Andreas. 'u+1' must 
be written 'u +1'. The latter syntax seems more correct to me in any 
case - the '+' is part of the operand, not the command. That said, many 
people are probably used to using the command like Andreas.

Andrew

Re: [PATCH] Fix 'Undefined command' error message

[Daniel Jacobowitz]

On Wed, Nov 23, 2005 at 07:05:24PM +0000, Andrew STUBBS wrote:
> Eli Zaretskii wrote:
> >What TUI commands use these problematic characters, and how does TUI
> >avoid the problems pointed out by Andreas?
> 
> I don't know what uses '$', but the other three, '>', '<' and '+', are 
> commands in their own right and are used the move the source window 
> right, left and up through the file. '-' moves down. There may be other 
> commands I don't know about.
> 
> The TUI does *not* avoid the problems pointed out by Andreas. 'u+1' must 
> be written 'u +1'. The latter syntax seems more correct to me in any 
> case - the '+' is part of the operand, not the command. That said, many 
> people are probably used to using the command like Andreas.

I had no idea this was possible (or what it did, until I tried it)... I
wouldn't cry about breaking it if there was any benefit.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Wed, 23 Nov 2005 14:19:40 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, schwab@suse.de,
> 	gdb-patches@sources.redhat.com
> 
> > The TUI does *not* avoid the problems pointed out by Andreas. 'u+1' must 
> > be written 'u +1'. The latter syntax seems more correct to me in any 
> > case - the '+' is part of the operand, not the command. That said, many 
> > people are probably used to using the command like Andreas.
> 
> I had no idea this was possible (or what it did, until I tried it)... I
> wouldn't cry about breaking it if there was any benefit.

Sorry, Daniel, I don't think I get what you mean.  Could you please
elaborate on what you suggest we do?

Re: [PATCH] Fix 'Undefined command' error message

[Daniel Jacobowitz]

On Wed, Nov 23, 2005 at 09:53:46PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 23 Nov 2005 14:19:40 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>, schwab@suse.de,
> > 	gdb-patches@sources.redhat.com
> > 
> > > The TUI does *not* avoid the problems pointed out by Andreas. 'u+1' must 
> > > be written 'u +1'. The latter syntax seems more correct to me in any 
> > > case - the '+' is part of the operand, not the command. That said, many 
> > > people are probably used to using the command like Andreas.
> > 
> > I had no idea this was possible (or what it did, until I tried it)... I
> > wouldn't cry about breaking it if there was any benefit.
> 
> Sorry, Daniel, I don't think I get what you mean.  Could you please
> elaborate on what you suggest we do?

"u+1" in current versions of GDB is the same as "until +1" or "u +1",
which is "until the next source line".  I don't believe this is
documented behavior, and I also don't think it's particularly valuable
behavior - just an implementation accident.  So if "u+1" returned
'unknown command "u+1"' instead, I think that would be an acceptable
change, especially if it reduces some special-cases in the handling
of commands.

Right now it works in CLI, but not in TUI:
(gdb) u+1
Undefined command: "u".  Try "help".

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Wed, 23 Nov 2005 16:07:32 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Andrew STUBBS <andrew.stubbs@st.com>, schwab@suse.de,
> 	gdb-patches@sources.redhat.com
> 
> So if "u+1" returned 'unknown command "u+1"' instead, I think that
> would be an acceptable change, especially if it reduces some
> special-cases in the handling of commands.

Thanks.  Yes, I agree with that.

Andrew, does this resolve your doubt about how to go about this issue?

Re: [PATCH] Fix 'Undefined command' error message

[Andrew STUBBS]

Eli Zaretskii wrote:
>>Date: Wed, 23 Nov 2005 16:07:32 -0500
>>From: Daniel Jacobowitz <drow@false.org>
>>Cc: Andrew STUBBS <andrew.stubbs@st.com>, schwab@suse.de,
>>	gdb-patches@sources.redhat.com
>>
>>So if "u+1" returned 'unknown command "u+1"' instead, I think that
>>would be an acceptable change, especially if it reduces some
>>special-cases in the handling of commands.
> 
> 
> Thanks.  Yes, I agree with that.
> 
> Andrew, does this resolve your doubt about how to go about this issue?
> 

Yes, I think so. But just to comfirm, you are giving me permission to 
commit the patch as it is?

Thanks

Andrew

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Thu, 24 Nov 2005 10:31:15 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> Cc: gdb-patches@sources.redhat.com
> 
> >>So if "u+1" returned 'unknown command "u+1"' instead, I think that
> >>would be an acceptable change, especially if it reduces some
> >>special-cases in the handling of commands.
> > 
> > 
> > Thanks.  Yes, I agree with that.
> > 
> > Andrew, does this resolve your doubt about how to go about this issue?
> 
> Yes, I think so. But just to comfirm, you are giving me permission to 
> commit the patch as it is?

If the "u+1" issue is the only reason that prevented that (I simply
forgot what exactly you said in your original message, sorry), then
yes, you can commit the patches now.

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Wed, 23 Nov 2005 19:05:24 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> Cc: schwab@suse.de, gdb-patches@sources.redhat.com
> 
> The TUI does *not* avoid the problems pointed out by Andreas. 'u+1' must 
> be written 'u +1'.

Yuck! we should at least document this in the manual.

Re: [PATCH] Fix 'Undefined command' error message

[Eli Zaretskii]

> Date: Mon, 21 Nov 2005 17:43:06 +0000
> From: Andrew STUBBS <andrew.stubbs@st.com>
> Cc: gdb-patches@sources.redhat.com
> 
> Eli Zaretskii wrote:
> > I'd prefer to have the same characters allowed in all CLI commands.
> > Any other way would be very confusing to users.
> > 
> > Thanks for the other clarifications.  I think we should at least add
> > comments that explain the respective characters.
> 
> I have moved all three instances of this code in to one function, added 
> comments, removed the pointless '*p' and allowed the TUI characters all 
> the time.
> 
> The XDB characters clash with existing GDB commands so I have left those 
> disabled unless in XDB mode. Specifically commands such as 'x/i' fail 
> because the command there is actually only 'x'. Obviously this does not 
> affect commands such as 'x /i'.

Thanks, I'm happy with this patch.