diff mbox

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

Message ID 49cd709f-c82e-a6e1-0d44-f8a6700764f5@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Dec. 6, 2017, 9:40 p.m. UTC 
  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 mbox

Patch

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