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

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

[Andrew Burgess]

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(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 665142446f4..70570130030 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -53,6 +53,7 @@
 #include "source.h"
 #include "gdbsupport/byte-vector.h"
 #include "gdbsupport/gdb_optional.h"
+#include "safe-ctype.h"
 
 /* Last specified output format.  */
 
@@ -1233,6 +1234,62 @@ print_command_1 (const char *args, int voidprint)
     print_value (val, print_opts);
 }
 
+/* Called from command completion function to skip over /FMT
+   specifications, allowing the rest of the line to be completed.  Returns
+   true if the /FMT is at the end of the current line and there is nothing
+   left to complete, otherwise false is returned.
+
+   In either case *ARGS can be updated to point after any part of /FMT that
+   is present.
+
+   This function is designed so that trying to complete '/' will offer no
+   completions, the user needs to insert the format specification
+   themselves.  Trying to complete '/FMT' (where FMT is any non-empty set
+   of alpha-numeric characters) will cause readline to insert a single
+   space, setting the user up to enter the expression.  */
+
+static bool
+skip_over_slash_fmt (completion_tracker &tracker, const char **args)
+{
+  const char *text = *args;
+
+  if (text[0] == '/')
+    {
+      bool in_fmt;
+      tracker.set_use_custom_word_point (true);
+
+      if (ISALNUM (text[1]) || ISSPACE(text[1]))
+	{
+	  /* Skip over the actual format specification.  */
+	  while (*text != '\0' && !ISSPACE (*text))
+	    ++text;
+
+	  if (*text == '\0')
+	    {
+	      in_fmt = true;
+	      tracker.add_completion (make_unique_xstrdup (text));
+	    }
+	  else
+	    {
+	      in_fmt = false;
+	      while (ISSPACE (*text))
+		++text;
+	    }
+	}
+      else if (text[1] == '\0')
+	{
+	  in_fmt = true;
+	  ++text;
+	}
+
+      tracker.advance_custom_word_point_by (text - (*args));
+      *args = text;
+      return in_fmt;
+    }
+
+  return false;
+}
+
 /* See valprint.h.  */
 
 void
@@ -1245,6 +1302,9 @@ print_command_completer (struct cmd_list_element *ignore,
       (tracker, &text, gdb::option::PROCESS_OPTIONS_REQUIRE_DELIMITER, group))
     return;
 
+  if (skip_over_slash_fmt (tracker, &text))
+    return;
+
   const char *word = advance_to_expression_complete_word_point (tracker, text);
   expression_completer (ignore, tracker, text, word);
 }
@@ -1735,6 +1795,21 @@ x_command (const char *exp, int from_tty)
 	set_internalvar (lookup_internalvar ("__"), last_examine_value.get ());
     }
 }
+
+/* Command completion for the 'display' and 'x' commands.  */
+
+static void
+display_and_x_command_completer (struct cmd_list_element *ignore,
+				 completion_tracker &tracker,
+				 const char *text, const char * /*word*/)
+{
+  if (skip_over_slash_fmt (tracker, &text))
+    return;
+
+  const char *word = advance_to_expression_complete_word_point (tracker, text);
+  expression_completer (ignore, tracker, text, word);
+}
+
 \f
 
 /* Add an expression to the auto-display chain.
@@ -2713,7 +2788,7 @@ Describe what symbol is at location ADDR.\n\
 Usage: info symbol ADDR\n\
 Only for symbols with fixed locations (global or static scope)."));
 
-  add_com ("x", class_vars, x_command, _("\
+  c = add_com ("x", class_vars, x_command, _("\
 Examine memory: x/FMT ADDRESS.\n\
 ADDRESS is an expression for the memory address to examine.\n\
 FMT is a repeat count followed by a format letter and a size letter.\n\
@@ -2727,6 +2802,7 @@ examined backward from the address.\n\n\
 Defaults for format and size letters are those previously used.\n\
 Default count is 1.  Default address is following last thing printed\n\
 with this command or \"print\"."));
+  set_cmd_completer_handle_brkchars (c, display_and_x_command_completer);
 
   add_info ("display", info_display_command, _("\
 Expressions to display when program stops, with code numbers.\n\
@@ -2741,7 +2817,7 @@ No argument means cancel all automatic-display expressions.\n\
 Do \"info display\" to see current list of code numbers."),
 	   &cmdlist);
 
-  add_com ("display", class_vars, display_command, _("\
+  c = add_com ("display", class_vars, display_command, _("\
 Print value of expression EXP each time the program stops.\n\
 Usage: display[/FMT] EXP\n\
 /FMT may be used before EXP as in the \"print\" command.\n\
@@ -2750,6 +2826,7 @@ as in the \"x\" command, and then EXP is used to get the address to examine\n\
 and examining is done as in the \"x\" command.\n\n\
 With no argument, display all currently requested auto-display expressions.\n\
 Use \"undisplay\" to cancel display requests previously made."));
+  set_cmd_completer_handle_brkchars (c, display_and_x_command_completer);
 
   add_cmd ("display", class_vars, enable_display_command, _("\
 Enable some expressions to be displayed when program stops.\n\
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index ac7f61ddfbc..ad4477af612 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -172,6 +172,11 @@ if { ![readline_is_used] } {
     return -1
 }
 
+# The bulk of this test script pre-dates the completion-support
+# library, and should probably (where possible) be converted.
+# However, for now, new tests are being added using this library.
+load_lib completion-support.exp
+
 set test "complete 'hfgfh'"
 send_gdb "hfgfh\t"
 gdb_test_multiple "" "$test" {
@@ -922,3 +927,24 @@ gdb_test_multiple "" "$test" {
 	pass "$test"
     }
 }
+
+# Test completion of 'p', 'x', and 'display' all using a /FMT.
+foreach_with_prefix spc { " " "" } {
+    test_gdb_complete_multiple "p${spc}/d some_union_global." "" "f" {
+	"f1"
+	"f2"
+    }
+
+    test_gdb_complete_unique "x${spc}/1w values\[0\].b"\
+	"x${spc}/1w values\[0\].b_field"
+
+    test_gdb_complete_unique "display${spc}/x values\[0\].z"\
+	"display${spc}/x values\[0\].z_field"
+}
+
+# Test 'p' using both options and /FMT.
+test_gdb_complete_multiple "p -array on -- /d some_union_global." \
+    "" "f" {
+	"f1"
+	"f2"
+    }
-- 
2.25.4

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

[Pedro Alves]

Hi Andrew,

[Apologies if you receive multiple copies, I've been having
email issues today.]

Very nice.  See comments below.

On 11/16/20 3:42 PM, 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>
> 

AFAICS, the new tests miss coverage for these two improvements.

This would do it:

--- c/gdb/testsuite/gdb.base/completion.exp
+++ w/gdb/testsuite/gdb.base/completion.exp
@@ -935,6 +935,9 @@ foreach_with_prefix spc { " " "" } {
        "f2"
     }
 
+    test_gdb_complete_none "p${spc}/"
+    test_gdb_complete_unique "p${spc}/d" "p${spc}/d"
+

> 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(-)
> 
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 665142446f4..70570130030 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -53,6 +53,7 @@
>  #include "source.h"
>  #include "gdbsupport/byte-vector.h"
>  #include "gdbsupport/gdb_optional.h"
> +#include "safe-ctype.h"
>  
>  /* Last specified output format.  */
>  
> @@ -1233,6 +1234,62 @@ print_command_1 (const char *args, int voidprint)
>      print_value (val, print_opts);
>  }
>  
> +/* Called from command completion function to skip over /FMT
> +   specifications, allowing the rest of the line to be completed.  Returns
> +   true if the /FMT is at the end of the current line and there is nothing
> +   left to complete, otherwise false is returned.
> +
> +   In either case *ARGS can be updated to point after any part of /FMT that
> +   is present.
> +
> +   This function is designed so that trying to complete '/' will offer no
> +   completions, the user needs to insert the format specification
> +   themselves.  Trying to complete '/FMT' (where FMT is any non-empty set
> +   of alpha-numeric characters) will cause readline to insert a single
> +   space, setting the user up to enter the expression.  */
> +
> +static bool
> +skip_over_slash_fmt (completion_tracker &tracker, const char **args)
> +{
> +  const char *text = *args;
> +
> +  if (text[0] == '/')
> +    {
> +      bool in_fmt;
> +      tracker.set_use_custom_word_point (true);
> +
> +      if (ISALNUM (text[1]) || ISSPACE(text[1]))

Missing space before parens.

> +      tracker.advance_custom_word_point_by (text - (*args));

The parens in (*args) are redundant.

This is OK with me with these issues addressed.

Thanks,
Pedro Alves

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> The /FMT specification on the print command currently breaks command
Andrew> completion, so:

Note that this is PR cli/16256

Andrew> +      if (ISALNUM (text[1]) || ISSPACE(text[1]))

Missing space before a paren here.

Andrew> +	{
Andrew> +	  /* Skip over the actual format specification.  */
Andrew> +	  while (*text != '\0' && !ISSPACE (*text))
Andrew> +	    ++text;

There's skip_spaces and skip_to_space for this kind of thing..

Andrew> +	      while (ISSPACE (*text))
Andrew> +		++text;

..and this.

Thank you for doing this.

Tom

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

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2020-11-18 09:11:56 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> The /FMT specification on the print command currently breaks command
> Andrew> completion, so:
> 
> Note that this is PR cli/16256
> 
> Andrew> +      if (ISALNUM (text[1]) || ISSPACE(text[1]))
> 
> Missing space before a paren here.
> 
> Andrew> +	{
> Andrew> +	  /* Skip over the actual format specification.  */
> Andrew> +	  while (*text != '\0' && !ISSPACE (*text))
> Andrew> +	    ++text;
> 
> There's skip_spaces and skip_to_space for this kind of thing..

Thanks for the suggestion.  I pushed the patch below to address
this issue.

Thanks,
Andrew

---

commit b3ff61f8155f296633f96206c926b545b97053b3
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu Nov 19 10:09:42 2020 +0000

    gdb: make use of skip_to_space and skip_spaces
    
    Some late feedback on this commit:
    
      commit 037d7135de575c9e0c20e9158c105979bfee339c
      Date:   Mon Nov 16 11:36:56 2020 +0000
    
          gdb: improve command completion for 'print', 'x', and 'display'
    
    Suggested making use of the skip_to_space and skip_spaces helper
    functions.  There should be no user visible changes after this commit.
    
    gdb/ChangeLog:
    
            * printcmd.c (skip_over_slash_fmt): Make use of skip_to_space and
            skip_spaces.

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 8c05ac8c833..a9c64b97c81 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1261,8 +1261,7 @@ skip_over_slash_fmt (completion_tracker &tracker, const char **args)
       if (ISALNUM (text[1]) || ISSPACE (text[1]))
 	{
 	  /* Skip over the actual format specification.  */
-	  while (*text != '\0' && !ISSPACE (*text))
-	    ++text;
+	  text = skip_to_space (text);
 
 	  if (*text == '\0')
 	    {
@@ -1272,8 +1271,7 @@ skip_over_slash_fmt (completion_tracker &tracker, const char **args)
 	  else
 	    {
 	      in_fmt = false;
-	      while (ISSPACE (*text))
-		++text;
+	      text = skip_spaces (text);
 	    }
 	}
       else if (text[1] == '\0')

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

[Pedro Alves]

On 11/19/20 10:14 AM, Andrew Burgess wrote:
>> Andrew> The /FMT specification on the print command currently breaks command
>> Andrew> completion, so:
>>
>> Note that this is PR cli/16256
>>
>> Andrew> +      if (ISALNUM (text[1]) || ISSPACE(text[1]))
>>
>> Missing space before a paren here.
>>
>> Andrew> +	{
>> Andrew> +	  /* Skip over the actual format specification.  */
>> Andrew> +	  while (*text != '\0' && !ISSPACE (*text))
>> Andrew> +	    ++text;
>>
>> There's skip_spaces and skip_to_space for this kind of thing..
> Thanks for the suggestion.  I pushed the patch below to address
> this issue.

I assumed it was on purpose that those weren't used, since you've
included safe-ctype.h and are (rightfully IMO) using ISSPACE and not 
isspace.  skip_spaces/skip_to_space use isspace not ISSPACE.
Probably doesn't make a difference in practice.  Not sure there's
any locale where isspace/ISSPACE return a different result.
ISSPACE is cheaper, so maybe we should make skip_spaces/skip_to_space
use it.

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> I assumed it was on purpose that those weren't used, since you've
Pedro> included safe-ctype.h and are (rightfully IMO) using ISSPACE and not 
Pedro> isspace.  skip_spaces/skip_to_space use isspace not ISSPACE.
Pedro> Probably doesn't make a difference in practice.  Not sure there's
Pedro> any locale where isspace/ISSPACE return a different result.
Pedro> ISSPACE is cheaper, so maybe we should make skip_spaces/skip_to_space
Pedro> use it.

skip_* are intended for use by gdb commands -- they originally lived in
gdb/cli/ -- so if they do the wrong thing in some situation, they should
definitely be changed.

Tom

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

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2020-11-20 08:55:28 -0700]:

> >>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> I assumed it was on purpose that those weren't used, since you've
> Pedro> included safe-ctype.h and are (rightfully IMO) using ISSPACE and not 
> Pedro> isspace.  skip_spaces/skip_to_space use isspace not ISSPACE.
> Pedro> Probably doesn't make a difference in practice.  Not sure there's
> Pedro> any locale where isspace/ISSPACE return a different result.
> Pedro> ISSPACE is cheaper, so maybe we should make skip_spaces/skip_to_space
> Pedro> use it.
> 
> skip_* are intended for use by gdb commands -- they originally lived in
> gdb/cli/ -- so if they do the wrong thing in some situation, they should
> definitely be changed.

Like this?

Thanks,
Andrew

---

commit e3a987aa1906be82aee0d777a92e7ac8d6502c22
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Nov 20 17:23:03 2020 +0000

    gdbsupport: make use of safe-ctype functions from libiberty
    
    Make use of the safe-ctype replacements for the standard ctype
    character checking functions.
    
    gdbsupport/ChangeLog:
    
            * gdbsupport/common-utils.cc: Change 'ctype.h' include to
            'safe-ctype.h'.
            (extract_string_maybe_quoted): Use safe-ctype function versions.
            (is_digit_in_base): Likewise.
            (digit_to_int): Likewise.
            (strtoulst): Likewise.
            (skip_spaces): Likewise.
            (skip_to_space): Likewise.

diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
index b5e4d2928ec..4f5c26d075c 100644
--- a/gdbsupport/common-utils.cc
+++ b/gdbsupport/common-utils.cc
@@ -20,7 +20,7 @@
 #include "common-defs.h"
 #include "common-utils.h"
 #include "host-defs.h"
-#include <ctype.h>
+#include "safe-ctype.h"
 
 void *
 xzalloc (size_t size)
@@ -177,7 +177,7 @@ extract_string_maybe_quoted (const char **arg)
   /* Parse p similarly to gdb_argv buildargv function.  */
   while (*p != '\0')
     {
-      if (isspace (*p) && !squote && !dquote && !bsquote)
+      if (ISSPACE (*p) && !squote && !dquote && !bsquote)
 	break;
       else
 	{
@@ -230,21 +230,21 @@ extract_string_maybe_quoted (const char **arg)
 static int
 is_digit_in_base (unsigned char digit, int base)
 {
-  if (!isalnum (digit))
+  if (!ISALNUM (digit))
     return 0;
   if (base <= 10)
-    return (isdigit (digit) && digit < base + '0');
+    return (ISDIGIT (digit) && digit < base + '0');
   else
-    return (isdigit (digit) || tolower (digit) < base - 10 + 'a');
+    return (ISDIGIT (digit) || TOLOWER (digit) < base - 10 + 'a');
 }
 
 static int
 digit_to_int (unsigned char c)
 {
-  if (isdigit (c))
+  if (ISDIGIT (c))
     return c - '0';
   else
-    return tolower (c) - 'a' + 10;
+    return TOLOWER (c) - 'a' + 10;
 }
 
 /* As for strtoul, but for ULONGEST results.  */
@@ -258,7 +258,7 @@ strtoulst (const char *num, const char **trailer, int base)
   int i = 0;
 
   /* Skip leading whitespace.  */
-  while (isspace (num[i]))
+  while (ISSPACE (num[i]))
     i++;
 
   /* Handle prefixes.  */
@@ -325,7 +325,7 @@ skip_spaces (char *chp)
 {
   if (chp == NULL)
     return NULL;
-  while (*chp && isspace (*chp))
+  while (*chp && ISSPACE (*chp))
     chp++;
   return chp;
 }
@@ -337,7 +337,7 @@ skip_spaces (const char *chp)
 {
   if (chp == NULL)
     return NULL;
-  while (*chp && isspace (*chp))
+  while (*chp && ISSPACE (*chp))
     chp++;
   return chp;
 }
@@ -349,7 +349,7 @@ skip_to_space (const char *chp)
 {
   if (chp == NULL)
     return NULL;
-  while (*chp && !isspace (*chp))
+  while (*chp && !ISSPACE (*chp))
     chp++;
   return chp;
 }

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

[Simon Marchi]

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;
      |              ^~~~~~


Simon

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

[Andrew Burgess]

* 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;

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

[Simon Marchi]

On 2020-11-27 6:13 a.m., Andrew Burgess wrote:
> * 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;
>

I don't really know about how completion work, but based on the (very
useful) comments you put, it makes sense.

Thanks!

Simon

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> skip_* are intended for use by gdb commands -- they originally lived in
>> gdb/cli/ -- so if they do the wrong thing in some situation, they should
>> definitely be changed.

Andrew> Like this?

Works for me, thanks.

Tom

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

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2020-12-03 10:03:06 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> >> skip_* are intended for use by gdb commands -- they originally lived in
> >> gdb/cli/ -- so if they do the wrong thing in some situation, they should
> >> definitely be changed.
> 
> Andrew> Like this?
> 
> Works for me, thanks.

Turns out that libinproctrace.so did not link against libiberty, so I
fixed that and pushed the patch below.

Thanks,
Andrew


--

commit 966484941738b7a474fb7e4fe29eb5693fc9096c
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Nov 20 17:23:03 2020 +0000

    gdbsupport: make use of safe-ctype functions from libiberty
    
    Make use of the safe-ctype replacements for the standard ctype
    character checking functions in gdbsupport/common-utils.cc.  The
    gdbsupport library is included into both gdb and gdbserver, and on the
    gdbserver side there are two targets, gdbserver itself, and also
    libinproctrace.so.
    
    libiberty was already being included in the gdbserver link command,
    but was missing from the libinproctrace.so link.  As a result, after
    changing gdbsupport/common-utils.cc to depend on libiberty,
    libinproctrace.so would no longer link until I modified its link line.
    
    gdbserver/ChangeLog:
    
            * Makefile.in (IPA_LIB): Include libiberty library.
    
    gdbsupport/ChangeLog:
    
            * gdbsupport/common-utils.cc: Change 'ctype.h' include to
            'safe-ctype.h'.
            (extract_string_maybe_quoted): Use safe-ctype function versions.
            (is_digit_in_base): Likewise.
            (digit_to_int): Likewise.
            (strtoulst): Likewise.
            (skip_spaces): Likewise.
            (skip_to_space): Likewise.

diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index 1969ed0ec37..e12848c1ce2 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -395,7 +395,7 @@ $(IPA_LIB): $(sort $(IPA_OBJS)) ${CDEPS}
 	$(ECHO_CXXLD) $(CC_LD) -shared -fPIC -Wl,--soname=$(IPA_LIB) \
 		-Wl,--no-undefined $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		 $(CXXFLAGS) \
-		-o $(IPA_LIB) ${IPA_OBJS} -ldl -pthread
+		-o $(IPA_LIB) ${IPA_OBJS} $(LIBIBERTY) -ldl -pthread
 
 # Put the proper machine-specific files first, so M-. on a machine
 # specific routine gets the one for the correct machine.
diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
index b5e4d2928ec..4f5c26d075c 100644
--- a/gdbsupport/common-utils.cc
+++ b/gdbsupport/common-utils.cc
@@ -20,7 +20,7 @@
 #include "common-defs.h"
 #include "common-utils.h"
 #include "host-defs.h"
-#include <ctype.h>
+#include "safe-ctype.h"
 
 void *
 xzalloc (size_t size)
@@ -177,7 +177,7 @@ extract_string_maybe_quoted (const char **arg)
   /* Parse p similarly to gdb_argv buildargv function.  */
   while (*p != '\0')
     {
-      if (isspace (*p) && !squote && !dquote && !bsquote)
+      if (ISSPACE (*p) && !squote && !dquote && !bsquote)
 	break;
       else
 	{
@@ -230,21 +230,21 @@ extract_string_maybe_quoted (const char **arg)
 static int
 is_digit_in_base (unsigned char digit, int base)
 {
-  if (!isalnum (digit))
+  if (!ISALNUM (digit))
     return 0;
   if (base <= 10)
-    return (isdigit (digit) && digit < base + '0');
+    return (ISDIGIT (digit) && digit < base + '0');
   else
-    return (isdigit (digit) || tolower (digit) < base - 10 + 'a');
+    return (ISDIGIT (digit) || TOLOWER (digit) < base - 10 + 'a');
 }
 
 static int
 digit_to_int (unsigned char c)
 {
-  if (isdigit (c))
+  if (ISDIGIT (c))
     return c - '0';
   else
-    return tolower (c) - 'a' + 10;
+    return TOLOWER (c) - 'a' + 10;
 }
 
 /* As for strtoul, but for ULONGEST results.  */
@@ -258,7 +258,7 @@ strtoulst (const char *num, const char **trailer, int base)
   int i = 0;
 
   /* Skip leading whitespace.  */
-  while (isspace (num[i]))
+  while (ISSPACE (num[i]))
     i++;
 
   /* Handle prefixes.  */
@@ -325,7 +325,7 @@ skip_spaces (char *chp)
 {
   if (chp == NULL)
     return NULL;
-  while (*chp && isspace (*chp))
+  while (*chp && ISSPACE (*chp))
     chp++;
   return chp;
 }
@@ -337,7 +337,7 @@ skip_spaces (const char *chp)
 {
   if (chp == NULL)
     return NULL;
-  while (*chp && isspace (*chp))
+  while (*chp && ISSPACE (*chp))
     chp++;
   return chp;
 }
@@ -349,7 +349,7 @@ skip_to_space (const char *chp)
 {
   if (chp == NULL)
     return NULL;
-  while (*chp && !isspace (*chp))
+  while (*chp && !ISSPACE (*chp))
     chp++;
   return chp;
 }

gdbserver build broken with -fsanitize=address

[Tom de Vries]

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

On 12/11/20 11:07 PM, Andrew Burgess wrote:
> * Tom Tromey <tom@tromey.com> [2020-12-03 10:03:06 -0700]:
> 
>>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
>>
>>>> skip_* are intended for use by gdb commands -- they originally lived in
>>>> gdb/cli/ -- so if they do the wrong thing in some situation, they should
>>>> definitely be changed.
>>
>> Andrew> Like this?
>>
>> Works for me, thanks.
> 
> Turns out that libinproctrace.so did not link against libiberty, so I
> fixed that and pushed the patch below.
> 

Hi,

I tried to build gdb with asan (using CFLAGS/CXXFLAGS="-O0 -g
-fsanitize=address" and LDFLAGS=-lasan), and ran into:
...
/usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: ../libiberty/libiberty.a(safe-ctype.o):
relocation R_X86_64_32 against `.data' can not be used when making a
shared object; recompile with -fPIC
collect2: error: ld returned 1 exit status
make[1]: *** [libinproctrace.so] Error 1
...

Using this patch, I got things building again:
...
diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index a2fe302d247..671cc67177e 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -102,7 +102,7 @@ CC_LD = $(CXX) $(CXX_DIALECT)
 INCLUDE_DIR = ${srcdir}/../include
 INCLUDE_DEP = $$(INCLUDE_DIR)

-LIBIBERTY_BUILDDIR = ../libiberty
+LIBIBERTY_BUILDDIR = ../libiberty/noasan
 LIBIBERTY = $(LIBIBERTY_BUILDDIR)/libiberty.a

 GDBSUPPORT_BUILDDIR = ../gdbsupport
...
but this means that libiberty/noasan is also used for the gdbserver
binary, which is probably too restrictive.

I've looked at gcc-repo/src/libcc1/Makefile.am, where we have:
...
override CXXFLAGS := $(filter-out -fsanitize=address,$(CXXFLAGS))
override LDFLAGS := $(filter-out -fsanitize=address,$(LDFLAGS))
# Can be simplified when libiberty becomes a normal convenience library.

libiberty_normal = ../libiberty/libiberty.a
libiberty_noasan = ../libiberty/noasan/libiberty.a
libiberty_pic = ../libiberty/pic/libiberty.a
Wc=-Wc,
libiberty = $(if $(wildcard $(libiberty_noasan)),$(Wc)$(libiberty_noasan), \
            $(if $(wildcard $(libiberty_pic)),$(Wc)$(libiberty_pic), \
            $(Wc)$(libiberty_normal)))
libiberty_dep = $(patsubst $(Wc)%,%,$(libiberty))
...
but I haven't been able to decode what this does yet.

Thanks,
- Tom

> Thanks,
> Andrew
> 
> 
> --
> 
> commit 966484941738b7a474fb7e4fe29eb5693fc9096c
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Fri Nov 20 17:23:03 2020 +0000
> 
>     gdbsupport: make use of safe-ctype functions from libiberty
>     
>     Make use of the safe-ctype replacements for the standard ctype
>     character checking functions in gdbsupport/common-utils.cc.  The
>     gdbsupport library is included into both gdb and gdbserver, and on the
>     gdbserver side there are two targets, gdbserver itself, and also
>     libinproctrace.so.
>     
>     libiberty was already being included in the gdbserver link command,
>     but was missing from the libinproctrace.so link.  As a result, after
>     changing gdbsupport/common-utils.cc to depend on libiberty,
>     libinproctrace.so would no longer link until I modified its link line.
>     
>     gdbserver/ChangeLog:
>     
>             * Makefile.in (IPA_LIB): Include libiberty library.
>     
>     gdbsupport/ChangeLog:
>     
>             * gdbsupport/common-utils.cc: Change 'ctype.h' include to
>             'safe-ctype.h'.
>             (extract_string_maybe_quoted): Use safe-ctype function versions.
>             (is_digit_in_base): Likewise.
>             (digit_to_int): Likewise.
>             (strtoulst): Likewise.
>             (skip_spaces): Likewise.
>             (skip_to_space): Likewise.
> 
> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> index 1969ed0ec37..e12848c1ce2 100644
> --- a/gdbserver/Makefile.in
> +++ b/gdbserver/Makefile.in
> @@ -395,7 +395,7 @@ $(IPA_LIB): $(sort $(IPA_OBJS)) ${CDEPS}
>  	$(ECHO_CXXLD) $(CC_LD) -shared -fPIC -Wl,--soname=$(IPA_LIB) \
>  		-Wl,--no-undefined $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
>  		 $(CXXFLAGS) \
> -		-o $(IPA_LIB) ${IPA_OBJS} -ldl -pthread
> +		-o $(IPA_LIB) ${IPA_OBJS} $(LIBIBERTY) -ldl -pthread
>  
>  # Put the proper machine-specific files first, so M-. on a machine
>  # specific routine gets the one for the correct machine.
> diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
> index b5e4d2928ec..4f5c26d075c 100644
> --- a/gdbsupport/common-utils.cc
> +++ b/gdbsupport/common-utils.cc
> @@ -20,7 +20,7 @@
>  #include "common-defs.h"
>  #include "common-utils.h"
>  #include "host-defs.h"
> -#include <ctype.h>
> +#include "safe-ctype.h"
>  
>  void *
>  xzalloc (size_t size)
> @@ -177,7 +177,7 @@ extract_string_maybe_quoted (const char **arg)
>    /* Parse p similarly to gdb_argv buildargv function.  */
>    while (*p != '\0')
>      {
> -      if (isspace (*p) && !squote && !dquote && !bsquote)
> +      if (ISSPACE (*p) && !squote && !dquote && !bsquote)
>  	break;
>        else
>  	{
> @@ -230,21 +230,21 @@ extract_string_maybe_quoted (const char **arg)
>  static int
>  is_digit_in_base (unsigned char digit, int base)
>  {
> -  if (!isalnum (digit))
> +  if (!ISALNUM (digit))
>      return 0;
>    if (base <= 10)
> -    return (isdigit (digit) && digit < base + '0');
> +    return (ISDIGIT (digit) && digit < base + '0');
>    else
> -    return (isdigit (digit) || tolower (digit) < base - 10 + 'a');
> +    return (ISDIGIT (digit) || TOLOWER (digit) < base - 10 + 'a');
>  }
>  
>  static int
>  digit_to_int (unsigned char c)
>  {
> -  if (isdigit (c))
> +  if (ISDIGIT (c))
>      return c - '0';
>    else
> -    return tolower (c) - 'a' + 10;
> +    return TOLOWER (c) - 'a' + 10;
>  }
>  
>  /* As for strtoul, but for ULONGEST results.  */
> @@ -258,7 +258,7 @@ strtoulst (const char *num, const char **trailer, int base)
>    int i = 0;
>  
>    /* Skip leading whitespace.  */
> -  while (isspace (num[i]))
> +  while (ISSPACE (num[i]))
>      i++;
>  
>    /* Handle prefixes.  */
> @@ -325,7 +325,7 @@ skip_spaces (char *chp)
>  {
>    if (chp == NULL)
>      return NULL;
> -  while (*chp && isspace (*chp))
> +  while (*chp && ISSPACE (*chp))
>      chp++;
>    return chp;
>  }
> @@ -337,7 +337,7 @@ skip_spaces (const char *chp)
>  {
>    if (chp == NULL)
>      return NULL;
> -  while (*chp && isspace (*chp))
> +  while (*chp && ISSPACE (*chp))
>      chp++;
>    return chp;
>  }
> @@ -349,7 +349,7 @@ skip_to_space (const char *chp)
>  {
>    if (chp == NULL)
>      return NULL;
> -  while (*chp && !isspace (*chp))
> +  while (*chp && !ISSPACE (*chp))
>      chp++;
>    return chp;
>  }
> 

[PATCH][gdb/build] Fix gdbserver build with -fsanitize=address

[Tom de Vries]

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

[ was: Re: gdbserver build broken with -fsanitize=address ]

On 1/6/21 8:39 AM, Tom de Vries wrote:
> [ was: Re: [PATCH] gdb: improve command completion for 'print', 'x', and
> 'display' ]
> 
> On 12/11/20 11:07 PM, Andrew Burgess wrote:
>> * Tom Tromey <tom@tromey.com> [2020-12-03 10:03:06 -0700]:
>>
>>>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
>>>
>>>>> skip_* are intended for use by gdb commands -- they originally lived in
>>>>> gdb/cli/ -- so if they do the wrong thing in some situation, they should
>>>>> definitely be changed.
>>>
>>> Andrew> Like this?
>>>
>>> Works for me, thanks.
>>
>> Turns out that libinproctrace.so did not link against libiberty, so I
>> fixed that and pushed the patch below.
>>
> 
> Hi,
> 
> I tried to build gdb with asan (using CFLAGS/CXXFLAGS="-O0 -g
> -fsanitize=address" and LDFLAGS=-lasan), and ran into:
> ...
> /usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: ../libiberty/libiberty.a(safe-ctype.o):
> relocation R_X86_64_32 against `.data' can not be used when making a
> shared object; recompile with -fPIC
> collect2: error: ld returned 1 exit status
> make[1]: *** [libinproctrace.so] Error 1
> ...
> 
> Using this patch, I got things building again:
> ...
> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> index a2fe302d247..671cc67177e 100644
> --- a/gdbserver/Makefile.in
> +++ b/gdbserver/Makefile.in
> @@ -102,7 +102,7 @@ CC_LD = $(CXX) $(CXX_DIALECT)
>  INCLUDE_DIR = ${srcdir}/../include
>  INCLUDE_DEP = $$(INCLUDE_DIR)
> 
> -LIBIBERTY_BUILDDIR = ../libiberty
> +LIBIBERTY_BUILDDIR = ../libiberty/noasan
>  LIBIBERTY = $(LIBIBERTY_BUILDDIR)/libiberty.a
> 
>  GDBSUPPORT_BUILDDIR = ../gdbsupport
> ...
> but this means that libiberty/noasan is also used for the gdbserver
> binary, which is probably too restrictive.
> 

Attached patch fixes the issue.

Any comments?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-build-Fix-gdbserver-build-with-fsanitize-address.patch --]
[-- Type: text/x-patch, Size: 2447 bytes --]

[gdb/build] Fix gdbserver build with -fsanitize=address

When doing a gdbserver build with CFLAGS/CXXFLAGS/LDFLAGS=-fsanitize=address
we run into:
...
ld: ../libiberty/libiberty.a(safe-ctype.o):
relocation R_X86_64_32 against `.data' can not be used when making a
shared object; recompile with -fPIC
collect2: error: ld returned 1 exit status
make[1]: *** [libinproctrace.so] Error 1
...

This started with commit 96648494173 "gdbsupport: make use of safe-ctype
functions from libiberty", which introduced a dependency of libinproctrace.so
on libiberty.

Fix this in gdbserver/Makefile.in by using a setup similar to what is done in
gcc-repo/src/libcc1/Makefile.am, such that ../libiberty/noasan/libiberty.a is
used instead.

Build on x86_64-linux, both with and without -fsanitize=address.

gdbserver/ChangeLog:

2021-01-06  Tom de Vries  <tdevries@suse.de>

	* Makefile.in (LIBIBERTY_NORMAL, LIBIBERTY_NOASAN, LIBIBERTY_PIC):
	(LIBIBERTY_FOR_SHLIB): New var.
	(LIBIBERTY): Set using $(LIBIBERTY_NORMAL).
	(IPA_LIB): Use LIBIBERTY_FOR_SHLIB instead of LIBIBERTY in target rule.

---
 gdbserver/Makefile.in | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
index a2fe302d247..a14d3a7bc18 100644
--- a/gdbserver/Makefile.in
+++ b/gdbserver/Makefile.in
@@ -103,7 +103,16 @@ INCLUDE_DIR = ${srcdir}/../include
 INCLUDE_DEP = $$(INCLUDE_DIR)
 
 LIBIBERTY_BUILDDIR = ../libiberty
-LIBIBERTY = $(LIBIBERTY_BUILDDIR)/libiberty.a
+LIBIBERTY_NORMAL = $(LIBIBERTY_BUILDDIR)/libiberty.a
+LIBIBERTY_NOASAN = $(LIBIBERTY_BUILDDIR)/noasan/libiberty.a
+LIBIBERTY_PIC = $(LIBIBERTY_BUILDDIR)/pic/libiberty.a
+LIBIBERTY_FOR_SHLIB = \
+  $(if $(wildcard $(LIBIBERTY_NOASAN)),\
+       $(LIBIBERTY_NOASAN),\
+       $(if $(wildcard $(LIBIBERTY_PIC)),\
+	    $(LIBIBERTY_PIC),\
+	    $(LIBIBERTY_NORMAL)))
+LIBIBERTY = $(LIBIBERTY_NORMAL)
 
 GDBSUPPORT_BUILDDIR = ../gdbsupport
 GDBSUPPORT = $(GDBSUPPORT_BUILDDIR)/libgdbsupport.a
@@ -395,7 +404,7 @@ $(IPA_LIB): $(sort $(IPA_OBJS)) ${CDEPS}
 	$(ECHO_CXXLD) $(CC_LD) -shared -fPIC -Wl,--soname=$(IPA_LIB) \
 		-Wl,--no-undefined $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
 		 $(CXXFLAGS) \
-		-o $(IPA_LIB) ${IPA_OBJS} $(LIBIBERTY) -ldl -pthread
+		-o $(IPA_LIB) ${IPA_OBJS} $(LIBIBERTY_FOR_SHLIB) -ldl -pthread
 
 # Put the proper machine-specific files first, so M-. on a machine
 # specific routine gets the one for the correct machine.

Re: [PATCH][gdb/build] Fix gdbserver build with -fsanitize=address

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2021-01-06 17:29:31 +0100]:

> [ was: Re: gdbserver build broken with -fsanitize=address ]
> 
> On 1/6/21 8:39 AM, Tom de Vries wrote:
> > [ was: Re: [PATCH] gdb: improve command completion for 'print', 'x', and
> > 'display' ]
> > 
> > On 12/11/20 11:07 PM, Andrew Burgess wrote:
> >> * Tom Tromey <tom@tromey.com> [2020-12-03 10:03:06 -0700]:
> >>
> >>>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> >>>
> >>>>> skip_* are intended for use by gdb commands -- they originally lived in
> >>>>> gdb/cli/ -- so if they do the wrong thing in some situation, they should
> >>>>> definitely be changed.
> >>>
> >>> Andrew> Like this?
> >>>
> >>> Works for me, thanks.
> >>
> >> Turns out that libinproctrace.so did not link against libiberty, so I
> >> fixed that and pushed the patch below.
> >>
> > 
> > Hi,
> > 
> > I tried to build gdb with asan (using CFLAGS/CXXFLAGS="-O0 -g
> > -fsanitize=address" and LDFLAGS=-lasan), and ran into:
> > ...
> > /usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: ../libiberty/libiberty.a(safe-ctype.o):
> > relocation R_X86_64_32 against `.data' can not be used when making a
> > shared object; recompile with -fPIC
> > collect2: error: ld returned 1 exit status
> > make[1]: *** [libinproctrace.so] Error 1
> > ...
> > 
> > Using this patch, I got things building again:
> > ...
> > diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> > index a2fe302d247..671cc67177e 100644
> > --- a/gdbserver/Makefile.in
> > +++ b/gdbserver/Makefile.in
> > @@ -102,7 +102,7 @@ CC_LD = $(CXX) $(CXX_DIALECT)
> >  INCLUDE_DIR = ${srcdir}/../include
> >  INCLUDE_DEP = $$(INCLUDE_DIR)
> > 
> > -LIBIBERTY_BUILDDIR = ../libiberty
> > +LIBIBERTY_BUILDDIR = ../libiberty/noasan
> >  LIBIBERTY = $(LIBIBERTY_BUILDDIR)/libiberty.a
> > 
> >  GDBSUPPORT_BUILDDIR = ../gdbsupport
> > ...
> > but this means that libiberty/noasan is also used for the gdbserver
> > binary, which is probably too restrictive.
> > 
> 
> Attached patch fixes the issue.
> 
> Any comments?
> 
> Thanks,
> - Tom

> [gdb/build] Fix gdbserver build with -fsanitize=address
> 
> When doing a gdbserver build with CFLAGS/CXXFLAGS/LDFLAGS=-fsanitize=address
> we run into:
> ...
> ld: ../libiberty/libiberty.a(safe-ctype.o):
> relocation R_X86_64_32 against `.data' can not be used when making a
> shared object; recompile with -fPIC
> collect2: error: ld returned 1 exit status
> make[1]: *** [libinproctrace.so] Error 1
> ...
> 
> This started with commit 96648494173 "gdbsupport: make use of safe-ctype
> functions from libiberty", which introduced a dependency of libinproctrace.so
> on libiberty.
> 
> Fix this in gdbserver/Makefile.in by using a setup similar to what is done in
> gcc-repo/src/libcc1/Makefile.am, such that ../libiberty/noasan/libiberty.a is
> used instead.
> 
> Build on x86_64-linux, both with and without -fsanitize=address.
> 
> gdbserver/ChangeLog:
> 
> 2021-01-06  Tom de Vries  <tdevries@suse.de>
> 
> 	* Makefile.in (LIBIBERTY_NORMAL, LIBIBERTY_NOASAN, LIBIBERTY_PIC):
> 	(LIBIBERTY_FOR_SHLIB): New var.
> 	(LIBIBERTY): Set using $(LIBIBERTY_NORMAL).
> 	(IPA_LIB): Use LIBIBERTY_FOR_SHLIB instead of LIBIBERTY in target rule.

This looks good to me.

Thanks for fixing this.

Andrew

> 
> ---
>  gdbserver/Makefile.in | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> index a2fe302d247..a14d3a7bc18 100644
> --- a/gdbserver/Makefile.in
> +++ b/gdbserver/Makefile.in
> @@ -103,7 +103,16 @@ INCLUDE_DIR = ${srcdir}/../include
>  INCLUDE_DEP = $$(INCLUDE_DIR)
>  
>  LIBIBERTY_BUILDDIR = ../libiberty
> -LIBIBERTY = $(LIBIBERTY_BUILDDIR)/libiberty.a
> +LIBIBERTY_NORMAL = $(LIBIBERTY_BUILDDIR)/libiberty.a
> +LIBIBERTY_NOASAN = $(LIBIBERTY_BUILDDIR)/noasan/libiberty.a
> +LIBIBERTY_PIC = $(LIBIBERTY_BUILDDIR)/pic/libiberty.a
> +LIBIBERTY_FOR_SHLIB = \
> +  $(if $(wildcard $(LIBIBERTY_NOASAN)),\
> +       $(LIBIBERTY_NOASAN),\
> +       $(if $(wildcard $(LIBIBERTY_PIC)),\
> +	    $(LIBIBERTY_PIC),\
> +	    $(LIBIBERTY_NORMAL)))
> +LIBIBERTY = $(LIBIBERTY_NORMAL)
>  
>  GDBSUPPORT_BUILDDIR = ../gdbsupport
>  GDBSUPPORT = $(GDBSUPPORT_BUILDDIR)/libgdbsupport.a
> @@ -395,7 +404,7 @@ $(IPA_LIB): $(sort $(IPA_OBJS)) ${CDEPS}
>  	$(ECHO_CXXLD) $(CC_LD) -shared -fPIC -Wl,--soname=$(IPA_LIB) \
>  		-Wl,--no-undefined $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) \
>  		 $(CXXFLAGS) \
> -		-o $(IPA_LIB) ${IPA_OBJS} $(LIBIBERTY) -ldl -pthread
> +		-o $(IPA_LIB) ${IPA_OBJS} $(LIBIBERTY_FOR_SHLIB) -ldl -pthread
>  
>  # Put the proper machine-specific files first, so M-. on a machine
>  # specific routine gets the one for the correct machine.

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

[Pedro Alves]

On 27/11/20 11:13, Andrew Burgess wrote:
>     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.

Do we already have a test for this?

Thanks,
Pedro Alves

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

[Andrew Burgess]

* Pedro Alves <pedro@palves.net> [2021-01-08 00:59:25 +0000]:

> On 27/11/20 11:13, Andrew Burgess wrote:
> >     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.
> 
> Do we already have a test for this?

No.  My bad.

How about the below?

Thanks,
Andrew

--

commit 0e6dbf5e91a9aea24c8c54f1da3b0fddb5ea01fb
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Jan 8 10:27:58 2021 +0000

    gdb: add missing test for completion of invalid /FMT strings
    
    This commit:
    
      commit 3df8c6afdd6d38a7622ff5f4b1a64aff80334ab9
      Date:   Fri Nov 27 10:46:07 2020 +0000
    
          gdb: fix potentially uninitialised variable
    
    Was pushed with no test.  Naughty!
    
    The new test checks how GDB behaves when completing an invalid /FMT
    string.
    
    Currently GDB does no validation of the /FMT string during tab
    completion, and just assumes that any /FMT string is valid and
    complete when the user hits TAB. So:
    
      (gdb) p/@@<TAB>
    
    Will give:
    
      (gdb) p/@@ <CURSOR IS HERE>
    
    We already had a test in place for completion on a valid /FMT string,
    but the above commit fixed a bug in the logic for completing invalid
    /FMT strings.  Now we have a test for this too.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.base/completion.exp: Add a new test.

diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 98bca554e3a..076134cdc33 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -938,6 +938,11 @@ foreach_with_prefix spc { " " "" } {
     test_gdb_complete_none "p${spc}/"
     test_gdb_complete_unique "p${spc}/d" "p${spc}/d"
 
+    # Try completion on an invalid /FMT string.  GDB doesn't attempt
+    # to validate the /FMT string during completion, the string is
+    # just assumed to be complete when the user hits TAB.
+    test_gdb_complete_unique "p${spc}/@" "p${spc}/@"
+
     test_gdb_complete_unique "x${spc}/1w values\[0\].b"\
 	"x${spc}/1w values\[0\].b_field"
 

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

[Pedro Alves]

On 08/01/21 10:34, Andrew Burgess wrote:
> * Pedro Alves <pedro@palves.net> [2021-01-08 00:59:25 +0000]:
> 
>> On 27/11/20 11:13, Andrew Burgess wrote:
>>>     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.
>>
>> Do we already have a test for this?
> 
> No.  My bad.
> 
> How about the below?

LGTM, thanks!