[RFA 1/3] Remove cleanups from prepare_execute_command

[RFA 1/3] Remove cleanups from prepare_execute_command

[Tom Tromey]

This changes prepare_execute_command to return a scoped_value_mark
rather than a cleanup.

ChangeLog
2017-10-17  Tom Tromey  <tom@tromey.com>

	* mi/mi-main.c (mi_cmd_execute): Update.
	* top.h (prepare_execute_command): Return scoped_value_mark.
	* value.h (class scoped_value_mark): Use DISABLE_COPY_AND_ASSIGN.
	Add move constructor.
	* top.c (prepare_execute_command): Return scoped_value_mark.
	(execute_command): Update.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/mi/mi-main.c |  5 +----
 gdb/top.c        | 18 +++++-------------
 gdb/top.h        |  3 ++-
 gdb/value.h      |  4 ++++
 5 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 8dc955da1f..3caf904e28 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2095,9 +2095,7 @@ mi_execute_command (const char *cmd, int from_tty)
 static void
 mi_cmd_execute (struct mi_parse *parse)
 {
-  struct cleanup *cleanup;
-
-  cleanup = prepare_execute_command ();
+  scoped_value_mark cleanup = prepare_execute_command ();
 
   if (parse->all && parse->thread_group != -1)
     error (_("Cannot specify --thread-group together with --all"));
@@ -2189,7 +2187,6 @@ mi_cmd_execute (struct mi_parse *parse)
 
       error_stream (stb);
     }
-  do_cleanups (cleanup);
 }
 
 /* FIXME: This is just a hack so we can get some extra commands going.
diff --git a/gdb/top.c b/gdb/top.c
index 3b3bbee4ac..9f5dd58d21 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -438,15 +438,9 @@ do_chdir_cleanup (void *old_dir)
 }
 #endif
 
-struct cleanup *
-prepare_execute_command (void)
+scoped_value_mark
+prepare_execute_command ()
 {
-  struct value *mark;
-  struct cleanup *cleanup;
-
-  mark = value_mark ();
-  cleanup = make_cleanup_value_free_to_mark (mark);
-
   /* With multiple threads running while the one we're examining is
      stopped, the dcache can get stale without us being able to detect
      it.  For the duration of the command, though, use the dcache to
@@ -454,7 +448,7 @@ prepare_execute_command (void)
   if (non_stop)
     target_dcache_invalidate ();
 
-  return cleanup;
+  return scoped_value_mark ();
 }
 
 /* Tell the user if the language has changed (except first time) after
@@ -534,12 +528,12 @@ maybe_wait_sync_command_done (int was_sync)
 void
 execute_command (char *p, int from_tty)
 {
-  struct cleanup *cleanup_if_error, *cleanup;
+  struct cleanup *cleanup_if_error;
   struct cmd_list_element *c;
   char *line;
 
   cleanup_if_error = make_bpstat_clear_actions_cleanup ();
-  cleanup = prepare_execute_command ();
+  scoped_value_mark cleanup = prepare_execute_command ();
 
   /* Force cleanup of any alloca areas if using C alloca instead of
      a builtin alloca.  */
@@ -548,7 +542,6 @@ execute_command (char *p, int from_tty)
   /* This can happen when command_line_input hits end of file.  */
   if (p == NULL)
     {
-      do_cleanups (cleanup);
       discard_cleanups (cleanup_if_error);
       return;
     }
@@ -623,7 +616,6 @@ execute_command (char *p, int from_tty)
 
   check_frame_language_change ();
 
-  do_cleanups (cleanup);
   discard_cleanups (cleanup_if_error);
 }
 
diff --git a/gdb/top.h b/gdb/top.h
index ab21e5e013..ab65ddb741 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -22,6 +22,7 @@
 
 #include "buffer.h"
 #include "event-loop.h"
+#include "value.h"
 
 struct tl_interp_info;
 
@@ -249,7 +250,7 @@ extern void check_frame_language_change (void);
 /* Prepare for execution of a command.
    Call this before every command, CLI or MI.
    Returns a cleanup to be run after the command is completed.  */
-extern struct cleanup *prepare_execute_command (void);
+extern scoped_value_mark prepare_execute_command (void);
 
 /* This function returns a pointer to the string that is used
    by gdb for its command prompt.  */
diff --git a/gdb/value.h b/gdb/value.h
index bc97ec0728..fb6f81fb12 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -730,6 +730,10 @@ class scoped_value_mark
     free_to_mark ();
   }
 
+  scoped_value_mark (scoped_value_mark &&other) = default;
+
+  DISABLE_COPY_AND_ASSIGN (scoped_value_mark);
+
   /* Free the values currently on the value stack.  */
   void free_to_mark ()
   {
-- 
2.13.6

[RFA 3/3] Remove cleanups from break-catch-syscall.c

[Tom Tromey]

This removes the remaining cleanups from break-catch-syscall.c by
storing temporary strings in a vector.

ChangeLog
2017-10-17  Tom Tromey  <tom@tromey.com>

	* break-catch-syscall.c (catch_syscall_completer): Use
	std::string, gdb::unique_xmalloc_ptr.
---
 gdb/ChangeLog             |  5 +++++
 gdb/break-catch-syscall.c | 35 ++++++++++++++++++-----------------
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 01e761ce37..2bbfee0ac4 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -560,9 +560,7 @@ catch_syscall_completer (struct cmd_list_element *cmd,
                          const char *text, const char *word)
 {
   struct gdbarch *gdbarch = get_current_arch ();
-  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
-  const char **group_list = NULL;
-  const char **syscall_list = NULL;
+  gdb::unique_xmalloc_ptr<const char *> group_list;
   const char *prefix;
   int i;
 
@@ -575,34 +573,37 @@ catch_syscall_completer (struct cmd_list_element *cmd,
   if (startswith (prefix, "g:") || startswith (prefix, "group:"))
     {
       /* Perform completion inside 'group:' namespace only.  */
-      group_list = get_syscall_group_names (gdbarch);
+      group_list.reset (get_syscall_group_names (gdbarch));
       if (group_list != NULL)
-	complete_on_enum (tracker, group_list, word, word);
+	complete_on_enum (tracker, group_list.get (), word, word);
     }
   else
     {
       /* Complete with both, syscall names and groups.  */
-      syscall_list = get_syscall_names (gdbarch);
-      group_list = get_syscall_group_names (gdbarch);
+      gdb::unique_xmalloc_ptr<const char *> syscall_list
+	(get_syscall_names (gdbarch));
+      group_list.reset (get_syscall_group_names (gdbarch));
+
+      const char **group_ptr = group_list.get ();
+
+      /* Hold on to strings while we're using them.  */
+      std::vector<std::string> holders;
 
       /* Append "group:" prefix to syscall groups.  */
-      for (i = 0; group_list[i] != NULL; i++)
+      for (i = 0; group_ptr[i] != NULL; i++)
 	{
-	  char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
+	  std::string prefixed_group = string_printf ("group:%s",
+						      group_ptr[i]);
 
-	  group_list[i] = prefixed_group;
-	  make_cleanup (xfree, prefixed_group);
+	  group_ptr[i] = prefixed_group.c_str ();
+	  holders.push_back (prefixed_group);
 	}
 
       if (syscall_list != NULL)
-	complete_on_enum (tracker, syscall_list, word, word);
+	complete_on_enum (tracker, syscall_list.get (), word, word);
       if (group_list != NULL)
-	complete_on_enum (tracker, group_list, word, word);
+	complete_on_enum (tracker, group_ptr, word, word);
     }
-
-  xfree (syscall_list);
-  xfree (group_list);
-  do_cleanups (cleanups);
 }
 
 static void
-- 
2.13.6

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Simon Marchi]

On 2017-10-18 00:06, Tom Tromey wrote:
> This removes the remaining cleanups from break-catch-syscall.c by
> storing temporary strings in a vector.
> 
> ChangeLog
> 2017-10-17  Tom Tromey  <tom@tromey.com>
> 
> 	* break-catch-syscall.c (catch_syscall_completer): Use
> 	std::string, gdb::unique_xmalloc_ptr.
> ---
>  gdb/ChangeLog             |  5 +++++
>  gdb/break-catch-syscall.c | 35 ++++++++++++++++++-----------------
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
> index 01e761ce37..2bbfee0ac4 100644
> --- a/gdb/break-catch-syscall.c
> +++ b/gdb/break-catch-syscall.c
> @@ -560,9 +560,7 @@ catch_syscall_completer (struct cmd_list_element 
> *cmd,
>                           const char *text, const char *word)
>  {
>    struct gdbarch *gdbarch = get_current_arch ();
> -  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
> -  const char **group_list = NULL;
> -  const char **syscall_list = NULL;
> +  gdb::unique_xmalloc_ptr<const char *> group_list;
>    const char *prefix;
>    int i;
> 
> @@ -575,34 +573,37 @@ catch_syscall_completer (struct cmd_list_element 
> *cmd,
>    if (startswith (prefix, "g:") || startswith (prefix, "group:"))
>      {
>        /* Perform completion inside 'group:' namespace only.  */
> -      group_list = get_syscall_group_names (gdbarch);
> +      group_list.reset (get_syscall_group_names (gdbarch));
>        if (group_list != NULL)
> -	complete_on_enum (tracker, group_list, word, word);
> +	complete_on_enum (tracker, group_list.get (), word, word);
>      }
>    else
>      {
>        /* Complete with both, syscall names and groups.  */
> -      syscall_list = get_syscall_names (gdbarch);
> -      group_list = get_syscall_group_names (gdbarch);
> +      gdb::unique_xmalloc_ptr<const char *> syscall_list
> +	(get_syscall_names (gdbarch));
> +      group_list.reset (get_syscall_group_names (gdbarch));
> +
> +      const char **group_ptr = group_list.get ();
> +
> +      /* Hold on to strings while we're using them.  */
> +      std::vector<std::string> holders;
> 
>        /* Append "group:" prefix to syscall groups.  */
> -      for (i = 0; group_list[i] != NULL; i++)
> +      for (i = 0; group_ptr[i] != NULL; i++)
>  	{
> -	  char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
> +	  std::string prefixed_group = string_printf ("group:%s",
> +						      group_ptr[i]);
> 
> -	  group_list[i] = prefixed_group;
> -	  make_cleanup (xfree, prefixed_group);
> +	  group_ptr[i] = prefixed_group.c_str ();
> +	  holders.push_back (prefixed_group);

Maybe an opportunity to std::move?  Alternatively:

   holders.emplace_back (string_printf ("group:%s", group_ptr[i]));
   group_ptr[i] = holders.back ().c_str ();

LGTM in any case, as well as the two previous patches.

Simon

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Maybe an opportunity to std::move?  Alternatively:
Simon>   holders.emplace_back (string_printf ("group:%s", group_ptr[i]));
Simon>   group_ptr[i] = holders.back ().c_str ();

I made this change.  Thanks for noticing this.

Tom

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
Simon> Maybe an opportunity to std::move?  Alternatively:
Simon> holders.emplace_back (string_printf ("group:%s", group_ptr[i]));
Simon> group_ptr[i] = holders.back ().c_str ();

Tom> I made this change.  Thanks for noticing this.

Oops, I meant to say, I made the std::move change.

Tom

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Simon Marchi]

On 2017-10-18 12:06 AM, Tom Tromey wrote:
> This removes the remaining cleanups from break-catch-syscall.c by
> storing temporary strings in a vector.
> 
> ChangeLog
> 2017-10-17  Tom Tromey  <tom@tromey.com>
> 
> 	* break-catch-syscall.c (catch_syscall_completer): Use
> 	std::string, gdb::unique_xmalloc_ptr.
> ---
>  gdb/ChangeLog             |  5 +++++
>  gdb/break-catch-syscall.c | 35 ++++++++++++++++++-----------------
>  2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
> index 01e761ce37..2bbfee0ac4 100644
> --- a/gdb/break-catch-syscall.c
> +++ b/gdb/break-catch-syscall.c
> @@ -560,9 +560,7 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>                           const char *text, const char *word)
>  {
>    struct gdbarch *gdbarch = get_current_arch ();
> -  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
> -  const char **group_list = NULL;
> -  const char **syscall_list = NULL;
> +  gdb::unique_xmalloc_ptr<const char *> group_list;
>    const char *prefix;
>    int i;
>  
> @@ -575,34 +573,37 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>    if (startswith (prefix, "g:") || startswith (prefix, "group:"))
>      {
>        /* Perform completion inside 'group:' namespace only.  */
> -      group_list = get_syscall_group_names (gdbarch);
> +      group_list.reset (get_syscall_group_names (gdbarch));
>        if (group_list != NULL)
> -	complete_on_enum (tracker, group_list, word, word);
> +	complete_on_enum (tracker, group_list.get (), word, word);
>      }
>    else
>      {
>        /* Complete with both, syscall names and groups.  */
> -      syscall_list = get_syscall_names (gdbarch);
> -      group_list = get_syscall_group_names (gdbarch);
> +      gdb::unique_xmalloc_ptr<const char *> syscall_list
> +	(get_syscall_names (gdbarch));
> +      group_list.reset (get_syscall_group_names (gdbarch));
> +
> +      const char **group_ptr = group_list.get ();
> +
> +      /* Hold on to strings while we're using them.  */
> +      std::vector<std::string> holders;
>  
>        /* Append "group:" prefix to syscall groups.  */
> -      for (i = 0; group_list[i] != NULL; i++)
> +      for (i = 0; group_ptr[i] != NULL; i++)
>  	{
> -	  char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
> +	  std::string prefixed_group = string_printf ("group:%s",
> +						      group_ptr[i]);
>  
> -	  group_list[i] = prefixed_group;
> -	  make_cleanup (xfree, prefixed_group);
> +	  group_ptr[i] = prefixed_group.c_str ();
> +	  holders.push_back (prefixed_group);
>  	}

Err, I think there's something that doesn't make sense here actually.  We
record in group_ptr[i] a pointer to the buffer of the temporary std::string,
that gets deleted when we go out of scope (end of the iteration).  That
causes this fail:

Running /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/catch-syscall.exp ...
FAIL: gdb.base/catch-syscall.exp: complete catch syscall group suggests 'group:' prefix (pattern 2)

By hand, you can do

(gdb) catch syscall g<TAB>

There should be many entries starting with group:, in the failing case there's only
one.  Presumably because in group_ptr all the pointers point to the same location,
that contains the last group added.  The completion mechanism then removes duplicates.

It is not enough to assign holders.back ().c_str () (after having pushed the string in
the vector), because when the vector gets reallocated it can now point to stale memory.
I think we have to do it in two pass, prepare the vector of std::string, and then get
pointers to the strings.  Something like this:


commit f9cab673480425a130b73394c3a63d256eadf314
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Fri Oct 20 16:22:40 2017 -0400

    Fix syscall group completion

diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index 82d3e36..095284c 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -562,7 +562,6 @@ catch_syscall_completer (struct cmd_list_element *cmd,
   struct gdbarch *gdbarch = get_current_arch ();
   gdb::unique_xmalloc_ptr<const char *> group_list;
   const char *prefix;
-  int i;

   /* Completion considers ':' to be a word separator, so we use this to
      verify whether the previous word was a group prefix.  If so, we
@@ -590,14 +589,11 @@ catch_syscall_completer (struct cmd_list_element *cmd,
       std::vector<std::string> holders;

       /* Append "group:" prefix to syscall groups.  */
-      for (i = 0; group_ptr[i] != NULL; i++)
-	{
-	  std::string prefixed_group = string_printf ("group:%s",
-						      group_ptr[i]);
+      for (int i = 0; group_ptr[i] != NULL; i++)
+	holders.push_back (string_printf ("group:%s", group_ptr[i]));

-	  group_ptr[i] = prefixed_group.c_str ();
-	  holders.push_back (std::move (prefixed_group));
-	}
+      for (int i = 0; group_ptr[i] != NULL; i++)
+	group_ptr[i] = holders[i].c_str ();

       if (syscall_list != NULL)
 	complete_on_enum (tracker, syscall_list.get (), word, word);

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Tom Tromey]

Simon> It is not enough to assign holders.back ().c_str () (after having pushed the string in
Simon> the vector), because when the vector gets reallocated it can now point to stale memory.
Simon> I think we have to do it in two pass, prepare the vector of std::string, and then get
Simon> pointers to the strings.

Sorry about that.  I think I considered this after the earlier review
and believed that the resizing wouldn't affect the locations; my
thinking being that growing the vector must surely use the move
constructor to move the strings into place -- since isn't this pretty
much the whole reason rvalues even exist?  But perhaps this doesn't
happen sometimes for some reason.

Your patch seems reasonable to me.

Tom

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Pedro Alves]

On 10/25/2017 05:41 AM, Tom Tromey wrote:
> Simon> It is not enough to assign holders.back ().c_str () (after having pushed the string in
> Simon> the vector), because when the vector gets reallocated it can now point to stale memory.
> Simon> I think we have to do it in two pass, prepare the vector of std::string, and then get
> Simon> pointers to the strings.
> 
> Sorry about that.  I think I considered this after the earlier review
> and believed that the resizing wouldn't affect the locations; my
> thinking being that growing the vector must surely use the move
> constructor to move the strings into place -- since isn't this pretty
> much the whole reason rvalues even exist?  But perhaps this doesn't
> happen sometimes for some reason.

std::vector will copy instead of move on resize if:

 - the element type is copyable and,
 - the move ctor is not marked noexcept.

This has to do with strong/basic exception guarantees.

But in this case the element type is a std::string, and
std::string can (and most implementations do) make use of
the SSO (small string optimization).  I.e., very simplified,
something like:

string
{
  struct free_store_data
  {
    char *ptr;
    size_t capacity;
  };

  union
  {
    char internal_buf[sizeof (free_store_data)];
    free_store_data free_store_buf;
  } m_data;
  size_t m_size;
};

When the string data fits in the internal buffer, moving the
object must mean copying the internal buffer
and in that case "moved_from.c_str()" will still be left
pointing to the internal buffer of the moved-from
std::string.

Thanks,
Pedro Alves

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> But in this case the element type is a std::string, and
Pedro> std::string can (and most implementations do) make use of
Pedro> the SSO (small string optimization).

The missing piece -- thanks.

Tom

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Sergio Durigan Junior]

On Friday, October 20 2017, Simon Marchi wrote:

> On 2017-10-18 12:06 AM, Tom Tromey wrote:
>> This removes the remaining cleanups from break-catch-syscall.c by
>> storing temporary strings in a vector.
>> 
>> ChangeLog
>> 2017-10-17  Tom Tromey  <tom@tromey.com>
>> 
>> 	* break-catch-syscall.c (catch_syscall_completer): Use
>> 	std::string, gdb::unique_xmalloc_ptr.
>> ---
>>  gdb/ChangeLog             |  5 +++++
>>  gdb/break-catch-syscall.c | 35 ++++++++++++++++++-----------------
>>  2 files changed, 23 insertions(+), 17 deletions(-)
>> 
>> diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
>> index 01e761ce37..2bbfee0ac4 100644
>> --- a/gdb/break-catch-syscall.c
>> +++ b/gdb/break-catch-syscall.c
>> @@ -560,9 +560,7 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>>                           const char *text, const char *word)
>>  {
>>    struct gdbarch *gdbarch = get_current_arch ();
>> -  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
>> -  const char **group_list = NULL;
>> -  const char **syscall_list = NULL;
>> +  gdb::unique_xmalloc_ptr<const char *> group_list;
>>    const char *prefix;
>>    int i;
>>  
>> @@ -575,34 +573,37 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>>    if (startswith (prefix, "g:") || startswith (prefix, "group:"))
>>      {
>>        /* Perform completion inside 'group:' namespace only.  */
>> -      group_list = get_syscall_group_names (gdbarch);
>> +      group_list.reset (get_syscall_group_names (gdbarch));
>>        if (group_list != NULL)
>> -	complete_on_enum (tracker, group_list, word, word);
>> +	complete_on_enum (tracker, group_list.get (), word, word);
>>      }
>>    else
>>      {
>>        /* Complete with both, syscall names and groups.  */
>> -      syscall_list = get_syscall_names (gdbarch);
>> -      group_list = get_syscall_group_names (gdbarch);
>> +      gdb::unique_xmalloc_ptr<const char *> syscall_list
>> +	(get_syscall_names (gdbarch));
>> +      group_list.reset (get_syscall_group_names (gdbarch));
>> +
>> +      const char **group_ptr = group_list.get ();
>> +
>> +      /* Hold on to strings while we're using them.  */
>> +      std::vector<std::string> holders;
>>  
>>        /* Append "group:" prefix to syscall groups.  */
>> -      for (i = 0; group_list[i] != NULL; i++)
>> +      for (i = 0; group_ptr[i] != NULL; i++)
>>  	{
>> -	  char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
>> +	  std::string prefixed_group = string_printf ("group:%s",
>> +						      group_ptr[i]);
>>  
>> -	  group_list[i] = prefixed_group;
>> -	  make_cleanup (xfree, prefixed_group);
>> +	  group_ptr[i] = prefixed_group.c_str ();
>> +	  holders.push_back (prefixed_group);
>>  	}
>
> Err, I think there's something that doesn't make sense here actually.  We
> record in group_ptr[i] a pointer to the buffer of the temporary std::string,
> that gets deleted when we go out of scope (end of the iteration).  That
> causes this fail:
>
> Running /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/catch-syscall.exp ...
> FAIL: gdb.base/catch-syscall.exp: complete catch syscall group suggests 'group:' prefix (pattern 2)

Hey guys,

Any news on this?  I was about to report this regression, but it seems
you were already dealing with it here.

Thanks,

> By hand, you can do
>
> (gdb) catch syscall g<TAB>
>
> There should be many entries starting with group:, in the failing case there's only
> one.  Presumably because in group_ptr all the pointers point to the same location,
> that contains the last group added.  The completion mechanism then removes duplicates.
>
> It is not enough to assign holders.back ().c_str () (after having pushed the string in
> the vector), because when the vector gets reallocated it can now point to stale memory.
> I think we have to do it in two pass, prepare the vector of std::string, and then get
> pointers to the strings.  Something like this:
>
>
> commit f9cab673480425a130b73394c3a63d256eadf314
> Author: Simon Marchi <simon.marchi@ericsson.com>
> Date:   Fri Oct 20 16:22:40 2017 -0400
>
>     Fix syscall group completion
>
> diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
> index 82d3e36..095284c 100644
> --- a/gdb/break-catch-syscall.c
> +++ b/gdb/break-catch-syscall.c
> @@ -562,7 +562,6 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>    struct gdbarch *gdbarch = get_current_arch ();
>    gdb::unique_xmalloc_ptr<const char *> group_list;
>    const char *prefix;
> -  int i;
>
>    /* Completion considers ':' to be a word separator, so we use this to
>       verify whether the previous word was a group prefix.  If so, we
> @@ -590,14 +589,11 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>        std::vector<std::string> holders;
>
>        /* Append "group:" prefix to syscall groups.  */
> -      for (i = 0; group_ptr[i] != NULL; i++)
> -	{
> -	  std::string prefixed_group = string_printf ("group:%s",
> -						      group_ptr[i]);
> +      for (int i = 0; group_ptr[i] != NULL; i++)
> +	holders.push_back (string_printf ("group:%s", group_ptr[i]));
>
> -	  group_ptr[i] = prefixed_group.c_str ();
> -	  holders.push_back (std::move (prefixed_group));
> -	}
> +      for (int i = 0; group_ptr[i] != NULL; i++)
> +	group_ptr[i] = holders[i].c_str ();
>
>        if (syscall_list != NULL)
>  	complete_on_enum (tracker, syscall_list.get (), word, word);

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [RFA 3/3] Remove cleanups from break-catch-syscall.c

[Simon Marchi]

On 2017-12-06 01:58 PM, Sergio Durigan Junior wrote:
> On Friday, October 20 2017, Simon Marchi wrote:
> 
>> On 2017-10-18 12:06 AM, Tom Tromey wrote:
>>> This removes the remaining cleanups from break-catch-syscall.c by
>>> storing temporary strings in a vector.
>>>
>>> ChangeLog
>>> 2017-10-17  Tom Tromey  <tom@tromey.com>
>>>
>>> 	* break-catch-syscall.c (catch_syscall_completer): Use
>>> 	std::string, gdb::unique_xmalloc_ptr.
>>> ---
>>>  gdb/ChangeLog             |  5 +++++
>>>  gdb/break-catch-syscall.c | 35 ++++++++++++++++++-----------------
>>>  2 files changed, 23 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
>>> index 01e761ce37..2bbfee0ac4 100644
>>> --- a/gdb/break-catch-syscall.c
>>> +++ b/gdb/break-catch-syscall.c
>>> @@ -560,9 +560,7 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>>>                           const char *text, const char *word)
>>>  {
>>>    struct gdbarch *gdbarch = get_current_arch ();
>>> -  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
>>> -  const char **group_list = NULL;
>>> -  const char **syscall_list = NULL;
>>> +  gdb::unique_xmalloc_ptr<const char *> group_list;
>>>    const char *prefix;
>>>    int i;
>>>  
>>> @@ -575,34 +573,37 @@ catch_syscall_completer (struct cmd_list_element *cmd,
>>>    if (startswith (prefix, "g:") || startswith (prefix, "group:"))
>>>      {
>>>        /* Perform completion inside 'group:' namespace only.  */
>>> -      group_list = get_syscall_group_names (gdbarch);
>>> +      group_list.reset (get_syscall_group_names (gdbarch));
>>>        if (group_list != NULL)
>>> -	complete_on_enum (tracker, group_list, word, word);
>>> +	complete_on_enum (tracker, group_list.get (), word, word);
>>>      }
>>>    else
>>>      {
>>>        /* Complete with both, syscall names and groups.  */
>>> -      syscall_list = get_syscall_names (gdbarch);
>>> -      group_list = get_syscall_group_names (gdbarch);
>>> +      gdb::unique_xmalloc_ptr<const char *> syscall_list
>>> +	(get_syscall_names (gdbarch));
>>> +      group_list.reset (get_syscall_group_names (gdbarch));
>>> +
>>> +      const char **group_ptr = group_list.get ();
>>> +
>>> +      /* Hold on to strings while we're using them.  */
>>> +      std::vector<std::string> holders;
>>>  
>>>        /* Append "group:" prefix to syscall groups.  */
>>> -      for (i = 0; group_list[i] != NULL; i++)
>>> +      for (i = 0; group_ptr[i] != NULL; i++)
>>>  	{
>>> -	  char *prefixed_group = xstrprintf ("group:%s", group_list[i]);
>>> +	  std::string prefixed_group = string_printf ("group:%s",
>>> +						      group_ptr[i]);
>>>  
>>> -	  group_list[i] = prefixed_group;
>>> -	  make_cleanup (xfree, prefixed_group);
>>> +	  group_ptr[i] = prefixed_group.c_str ();
>>> +	  holders.push_back (prefixed_group);
>>>  	}
>>
>> Err, I think there's something that doesn't make sense here actually.  We
>> record in group_ptr[i] a pointer to the buffer of the temporary std::string,
>> that gets deleted when we go out of scope (end of the iteration).  That
>> causes this fail:
>>
>> Running /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/catch-syscall.exp ...
>> FAIL: gdb.base/catch-syscall.exp: complete catch syscall group suggests 'group:' prefix (pattern 2)
> 
> Hey guys,
> 
> Any news on this?  I was about to report this regression, but it seems
> you were already dealing with it here.
> 
> Thanks,

Seems like it fell between the cracks, I guess I thought Tom would take
care of it and vice versa.  Thanks for the reminder!

I pushed it:

From 9a93831ccc0ba3ba447552069f230e6d93dcbf3f Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Wed, 6 Dec 2017 16:27:33 -0500
Subject: [PATCH] Fix syscall group completion

The test gdb.base/catch-syscall.exp has been failing since commit

  3d415c26bad3a15eed00d2ddf85c4268df77a4cc
  Remove cleanups from break-catch-syscall.c

The reason is that we are putting into the group_ptr array a pointer to
the buffer of the local string object.  If the string is small enough to
fit in the internal string buffer (used for small string optimizations),
the pointer will point to the local object directly.  So even if we
std::move the string to the vector, the pointer in group_ptr will still
point to the local object.  When we reuse that object (technically a new
instance, but most likely the same memory) for the next syscall, we'll
overwrite the previous string.  The result is that we'll get less
results than expected, since there will be duplicates.

We'll also run into problems if we push the string to the vector, and
then record the c_str () pointer using the string object in the vector.
The vector might get reallocated, the string may move in memory, and our
pointer in group_ptr will point to stale memory.

Instead, we have to push all the strings first, then, when we know the
vector won't change anymore, build the group_ptr array.  This is what
this patch does.

gdb/ChangeLog:

	* break-catch-syscall.c (catch_syscall_completer): Get pointers
	to syscall group strings after building the string vector.
---
 gdb/ChangeLog             |  5 +++++
 gdb/break-catch-syscall.c | 12 ++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 93acc17..c9a4098 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-12-06  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* break-catch-syscall.c (catch_syscall_completer): Get pointers
+	to syscall group strings after building the string vector.
+
 2017-12-06  Pedro Alves  <palves@redhat.com>

 	* remote.c (remote_query_supported): Don't send "xmlRegisters=" if
diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index a8312f2..dd7b379 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -559,7 +559,6 @@ catch_syscall_completer (struct cmd_list_element *cmd,
   struct gdbarch *gdbarch = get_current_arch ();
   gdb::unique_xmalloc_ptr<const char *> group_list;
   const char *prefix;
-  int i;

   /* Completion considers ':' to be a word separator, so we use this to
      verify whether the previous word was a group prefix.  If so, we
@@ -587,14 +586,11 @@ catch_syscall_completer (struct cmd_list_element *cmd,
       std::vector<std::string> holders;

       /* Append "group:" prefix to syscall groups.  */
-      for (i = 0; group_ptr[i] != NULL; i++)
-	{
-	  std::string prefixed_group = string_printf ("group:%s",
-						      group_ptr[i]);
+      for (int i = 0; group_ptr[i] != NULL; i++)
+	holders.push_back (string_printf ("group:%s", group_ptr[i]));

-	  group_ptr[i] = prefixed_group.c_str ();
-	  holders.push_back (std::move (prefixed_group));
-	}
+      for (int i = 0; group_ptr[i] != NULL; i++)
+	group_ptr[i] = holders[i].c_str ();

       if (syscall_list != NULL)
 	complete_on_enum (tracker, syscall_list.get (), word, word);
-- 
2.7.4

[RFA 2/3] Remove cleanup from call_function_by_hand_dummy

[Tom Tromey]

This changes call_function_by_hand_dummy to use std::string, removing
a cleanup.

ChangeLog
2017-10-17  Tom Tromey  <tom@tromey.com>

	* infcall.c (call_function_by_hand_dummy): Use std::string.
---
 gdb/ChangeLog |  4 ++++
 gdb/infcall.c | 14 ++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/gdb/infcall.c b/gdb/infcall.c
index d384d16c29..03749f3dc2 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -1308,10 +1308,8 @@ When the function is done executing, GDB will silently stop."),
 
     {
       /* Make a copy as NAME may be in an objfile freed by dummy_frame_pop.  */
-      char *name = xstrdup (get_function_name (funaddr,
-					       name_buf, sizeof (name_buf)));
-      make_cleanup (xfree, name);
-
+      std::string name = get_function_name (funaddr, name_buf,
+					    sizeof (name_buf));
 
       if (stopped_by_random_signal)
 	{
@@ -1339,7 +1337,7 @@ GDB has restored the context to what it was before the call.\n\
 To change this behavior use \"set unwindonsignal off\".\n\
 Evaluation of the expression containing the function\n\
 (%s) will be abandoned."),
-		     name);
+		     name.c_str ());
 	    }
 	  else
 	    {
@@ -1358,7 +1356,7 @@ To change this behavior use \"set unwindonsignal on\".\n\
 Evaluation of the expression containing the function\n\
 (%s) will be abandoned.\n\
 When the function is done executing, GDB will silently stop."),
-		     name);
+		     name.c_str ());
 	    }
 	}
 
@@ -1380,7 +1378,7 @@ context to its original state before the call.\n\
 To change this behaviour use \"set unwind-on-terminating-exception off\".\n\
 Evaluation of the expression containing the function (%s)\n\
 will be abandoned."),
-		 name);
+		 name.c_str ());
 	}
       else if (stop_stack_dummy == STOP_NONE)
 	{
@@ -1404,7 +1402,7 @@ The program being debugged stopped while in a function called from GDB.\n\
 Evaluation of the expression containing the function\n\
 (%s) will be abandoned.\n\
 When the function is done executing, GDB will silently stop."),
-		 name);
+		 name.c_str ());
 	}
 
     }
-- 
2.13.6