Re: [PATCH] gdb: improve command completion for 'print', 'x', and 'display'

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simark@simark.ca> [2020-11-26 14:12:34 -0500]:

> On 2020-11-16 10:42 a.m., Andrew Burgess wrote:
> > The /FMT specification on the print command currently breaks command
> > completion, so:
> > 
> >   (gdb) p var.<TAB><TAB>
> >   .... list of fields in var .....
> > 
> > But,
> > 
> >   (gdb) p/d var.<TAB><TAB>
> >   ..... list of all symbols .....
> > 
> > After this commit this issue is now resolved.
> > 
> > There are some other details around tab-completion and /FMT which
> > hopefully this commit improves.  So, before:
> > 
> >   (gdb) p/<TAB><TAB>
> >   .... lists all symbols .....
> > 
> > After:
> > 
> >   (gdb) p/<TAB><TAB>		# Nothing changes...
> > 
> > The thinking here is that after a / the user must type a FMT, but we
> > don't offer tab completion on FMT characters.  Placing a symbol
> > directly after a / will not do what the user expects, so offering that
> > seems wrong.
> > 
> > Similarly, before we had:
> > 
> >   (gdb) p/d<TAB><TAB>
> >   ... lists all symbols starting with 'd' ....
> > 
> > But afterwards:
> > 
> >   (gdb) p/d<TAB><TAB>		# Adds a single space, so we get:
> >   (gdb) p/d <CURSOR>
> > 
> > As before, typing a symbol where FMT is expected will not do what the
> > user expects.  If the user has added a FMT string then upon tab
> > completion GDB assumes the FMT string is complete and prepares the
> > user to type an expression.
> > 
> > In this commit I have also added completion functions for the 'x' and
> > 'display' commands.  These commands also support /FMT specifiers and
> > so share some code with 'print'.
> > 
> > gdb/ChangeLog:
> > 
> > 	* printcmd.c: Include 'safe-ctype.c'.
> > 	(skip_over_slash_fmt): New function.
> > 	(print_command_completer): Call skip_over_slash_fmt.
> > 	(display_and_x_command_completer): New function.
> > 	(_initialize_printcmd): Add command completion for 'x' and
> > 	'display'.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.base/completion.exp: Add new tests.
> > ---
> >  gdb/ChangeLog                         |  9 +++
> >  gdb/printcmd.c                        | 81 ++++++++++++++++++++++++++-
> >  gdb/testsuite/ChangeLog               |  4 ++
> >  gdb/testsuite/gdb.base/completion.exp | 26 +++++++++
> >  4 files changed, 118 insertions(+), 2 deletions(-)
> 
> I noticed this warning in the code added by this patch, I think it's not a false positive:
> 
> /home/simark/src/binutils-gdb/gdb/printcmd.c: In function ‘bool skip_over_slash_fmt(completion_tracker&, const char**)’:
> /home/simark/src/binutils-gdb/gdb/printcmd.c:1285:14: warning: ‘in_fmt’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>  1285 |       return in_fmt;
>       |              ^~~~~~

Thanks for bringing this to my attention.  I plan to push the patch
below to resolve this issue.

Thanks,
Andrew

---

commit 7ab525f25d2918c6e4073b346107b8730f07b0b9
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Nov 27 10:46:07 2020 +0000

    gdb: fix potentially uninitialised variable
    
    In commit:
    
      commit 037d7135de575c9e0c20e9158c105979bfee339c
      Date:   Mon Nov 16 11:36:56 2020 +0000
    
          gdb: improve command completion for 'print', 'x', and 'display'
    
    A potential use of an uninitialised variable was introduced.  This is
    fixed in this commit.
    
    Previously when analysing /FMT strings for tab completion we
    considered two possibilities, either the user has typed '/', or the
    user has typed '/' followed by an alpha-numeric character, as these
    are the only valid FMT string characters.
    
    This meant that if the user type, for example '/@' and then tried to
    tab complete gdb would use an uninitialised variable.
    
    Currently only the first character after the '/' is checked to see if
    it is alpha-numeric, so if a user typed '/x@@' then gdb would be happy
    to treat this as a FMT string.
    
    Given the goal of this change was primarily to allow tab completion of
    symbols later in the command when a /FMT was used then I decided to
    just make the /FMT skipping less smart.  Now any characters after the
    '/' up to the first white space, will be treated as a FMT string.
    
    gdb/ChangeLog:
    
            * printcmd.c (skip_over_slash_fmt): Reorder code to ensure in_fmt
            is always initialized.

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index a9c64b97c81..e95b8802950 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1258,27 +1258,38 @@ skip_over_slash_fmt (completion_tracker &tracker, const char **args)
       bool in_fmt;
       tracker.set_use_custom_word_point (true);
 
-      if (ISALNUM (text[1]) || ISSPACE (text[1]))
+      if (text[1] == '\0')
 	{
-	  /* Skip over the actual format specification.  */
+	  /* The user tried to complete after typing just the '/' character
+	     of the /FMT string.  Step the completer past the '/', but we
+	     don't offer any completions.  */
+	  in_fmt = true;
+	  ++text;
+	}
+      else
+	{
+	  /* The user has typed some characters after the '/', we assume
+	     this is a complete /FMT string, first skip over it.  */
 	  text = skip_to_space (text);
 
 	  if (*text == '\0')
 	    {
+	      /* We're at the end of the input string.  The user has typed
+		 '/FMT' and asked for a completion.  Push an empty
+		 completion string, this will cause readline to insert a
+		 space so the user now has '/FMT '.  */
 	      in_fmt = true;
 	      tracker.add_completion (make_unique_xstrdup (text));
 	    }
 	  else
 	    {
+	      /* The user has already typed things after the /FMT, skip the
+		 whitespace and return false.  Whoever called this function
+		 should then try to complete what comes next.  */
 	      in_fmt = false;
 	      text = skip_spaces (text);
 	    }
 	}
-      else if (text[1] == '\0')
-	{
-	  in_fmt = true;
-	  ++text;
-	}
 
       tracker.advance_custom_word_point_by (text - *args);
       *args = text;