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

  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
  

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
  

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
  

>>>>> "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
  

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