[2/3] gdb: make macro_expand_next return a gdb::unique_xmalloc_ptr<char>

Message ID 20200628165628.96730-3-simon.marchi@polymtl.ca
State New
Headers show
Series
  • Small cleanups in macro code
Related show

Commit Message

Jose E. Marchesi via Gdb-patches June 28, 2020, 4:56 p.m.
For some reason, macro_expand_next does not return a
gdb::unique_xmalloc_ptr<char>, like its counterparts macro_expand and
macro_expand_once.  This patch fixes that.

macro_buffer::release now returns a gdb::unique_xmalloc_ptr<char> too,
which required updating the other callers.  The `.release (). release
()` in macro_stringify looks a bit funny, but it's because one release
is for the macro_buffer, and the other is for the unique ptr.

I removed the ATTRIBUTE_UNUSED_RESULT on macro_buffer::release, I don't
really understand why it's there.  I don't see how this method could be
called without using the result, that would be an obvious memory leak.
The commit that introduced it (4e4a8b932b7 "Add ATTRIBUTE_UNUSED_RESULT
to macro_buffer") doesn't give any details.

gdb/ChangeLog:

	* c-exp.y (scan_macro_expansion): Don't free `expansion`.
	(lex_one_token): Update.
	* macroexp.c (struct macro_buffer) <release>: Return
	gdb::unique_xmalloc_ptr<char>.
	(macro_stringify): Update.
	(macro_expand): Update.
	(macro_expand_next): Return gdb::unique_xmalloc_ptr<char>.
	* macroexp.h (macro_expand_next): Likewise.

Change-Id: I67a74d0d479d2c20cdc82161ead7c54cea034f56
---
 gdb/c-exp.y    | 18 +++++++-----------
 gdb/macroexp.c | 18 ++++++++----------
 gdb/macroexp.h |  3 ++-
 3 files changed, 17 insertions(+), 22 deletions(-)

-- 
2.27.0

Comments

Tom Tromey June 30, 2020, 8:53 p.m. | #1
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:


Simon> I removed the ATTRIBUTE_UNUSED_RESULT on macro_buffer::release, I don't
Simon> really understand why it's there.  I don't see how this method could be
Simon> called without using the result, that would be an obvious memory leak.
Simon> The commit that introduced it (4e4a8b932b7 "Add ATTRIBUTE_UNUSED_RESULT
Simon> to macro_buffer") doesn't give any details.

Sorry about the lack of details!

Simon> -  /* Release the text of the buffer to the caller, which is now
Simon> -     responsible for freeing it.  */
Simon> -  ATTRIBUTE_UNUSED_RESULT char *release ()
Simon> +  /* Release the text of the buffer to the caller.  */
Simon> +  gdb::unique_xmalloc_ptr<char> release ()

In the old code, the caller had to remember to free the resulting char*.
ATTRIBUTE_UNUSED_RESULT meant that one couldn't write "x.release();"
without at least doing something with the result.

There was at least one case of this happening, though I didn't read
through the thread to find out exactly where it was:

https://sourceware.org/pipermail/gdb-patches/2019-February/155835.html

Using a unique pointer eliminates this common source of bugs, so I think
removing the attribute makes sense.

This patch looks fine to me.

Tom
Jose E. Marchesi via Gdb-patches June 30, 2020, 10:26 p.m. | #2
On 2020-06-30 4:53 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Simon> I removed the ATTRIBUTE_UNUSED_RESULT on macro_buffer::release, I don't

> Simon> really understand why it's there.  I don't see how this method could be

> Simon> called without using the result, that would be an obvious memory leak.

> Simon> The commit that introduced it (4e4a8b932b7 "Add ATTRIBUTE_UNUSED_RESULT

> Simon> to macro_buffer") doesn't give any details.

> 

> Sorry about the lack of details!

> 

> Simon> -  /* Release the text of the buffer to the caller, which is now

> Simon> -     responsible for freeing it.  */

> Simon> -  ATTRIBUTE_UNUSED_RESULT char *release ()

> Simon> +  /* Release the text of the buffer to the caller.  */

> Simon> +  gdb::unique_xmalloc_ptr<char> release ()

> 

> In the old code, the caller had to remember to free the resulting char*.

> ATTRIBUTE_UNUSED_RESULT meant that one couldn't write "x.release();"

> without at least doing something with the result.

> 

> There was at least one case of this happening, though I didn't read

> through the thread to find out exactly where it was:

> 

> https://sourceware.org/pipermail/gdb-patches/2019-February/155835.html

> 

> Using a unique pointer eliminates this common source of bugs, so I think

> removing the attribute makes sense.

> 

> This patch looks fine to me.

> 

> Tom

> 


Ok thanks, that makes sense!

Simon

Patch

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 61fa2fe684db..7fc23c4c8d28 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -2551,17 +2551,13 @@  static const struct token ident_tokens[] =
 
 
 static void
-scan_macro_expansion (char *expansion)
+scan_macro_expansion (const char *expansion)
 {
-  const char *copy;
-
   /* We'd better not be trying to push the stack twice.  */
   gdb_assert (! cpstate->macro_original_text);
 
-  /* Copy to the obstack, and then free the intermediate
-     expansion.  */
-  copy = obstack_strdup (&cpstate->expansion_obstack, expansion);
-  xfree (expansion);
+  /* Copy to the obstack.  */
+  const char *copy = obstack_strdup (&cpstate->expansion_obstack, expansion);
 
   /* Save the old lexptr value, so we can return to it when we're done
      parsing the expanded text.  */
@@ -2631,11 +2627,11 @@  lex_one_token (struct parser_state *par_state, bool *is_quoted_name)
   /* Check if this is a macro invocation that we need to expand.  */
   if (! scanning_macro_expansion ())
     {
-      char *expanded = macro_expand_next (&pstate->lexptr,
-					  *expression_macro_scope);
+      gdb::unique_xmalloc_ptr<char> expanded
+	= macro_expand_next (&pstate->lexptr, *expression_macro_scope);
 
-      if (expanded)
-        scan_macro_expansion (expanded);
+      if (expanded != nullptr)
+        scan_macro_expansion (expanded.get ());
     }
 
   pstate->prev_lexptr = pstate->lexptr;
diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index 92823807f153..e1d185d30c8e 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -128,15 +128,14 @@  struct macro_buffer
       xfree (text);
   }
 
-  /* Release the text of the buffer to the caller, which is now
-     responsible for freeing it.  */
-  ATTRIBUTE_UNUSED_RESULT char *release ()
+  /* Release the text of the buffer to the caller.  */
+  gdb::unique_xmalloc_ptr<char> release ()
   {
     gdb_assert (! shared);
     gdb_assert (size);
     char *result = text;
     text = NULL;
-    return result;
+    return gdb::unique_xmalloc_ptr<char> (result);
   }
 
   /* Resize the buffer to be at least N bytes long.  Raise an error if
@@ -708,7 +707,7 @@  macro_stringify (const char *str)
   stringify (&buffer, str, len);
   buffer.appendc ('\0');
 
-  return buffer.release ();
+  return buffer.release ().release ();
 }
 
 
@@ -1429,7 +1428,7 @@  macro_expand (const char *source, const macro_scope &scope)
 
   dest.appendc ('\0');
 
-  return gdb::unique_xmalloc_ptr<char> (dest.release ());
+  return dest.release ();
 }
 
 
@@ -1439,8 +1438,7 @@  macro_expand_once (const char *source, const macro_scope &scope)
   error (_("Expand-once not implemented yet."));
 }
 
-
-char *
+gdb::unique_xmalloc_ptr<char>
 macro_expand_next (const char **lexptr, const macro_scope &scope)
 {
   struct macro_buffer tok;
@@ -1454,7 +1452,7 @@  macro_expand_next (const char **lexptr, const macro_scope &scope)
 
   /* Get the text's first preprocessing token.  */
   if (! get_token (&tok, &src))
-    return 0;
+    return nullptr;
 
   /* If it's a macro invocation, expand it.  */
   if (maybe_expand (&dest, &tok, &src, 0, scope))
@@ -1469,6 +1467,6 @@  macro_expand_next (const char **lexptr, const macro_scope &scope)
   else
     {
       /* It wasn't a macro invocation.  */
-      return 0;
+      return nullptr;
     }
 }
diff --git a/gdb/macroexp.h b/gdb/macroexp.h
index ec992f22796d..511991cacd20 100644
--- a/gdb/macroexp.h
+++ b/gdb/macroexp.h
@@ -68,7 +68,8 @@  gdb::unique_xmalloc_ptr<char> macro_expand_once (const char *source,
    much have to do tokenization to find the end of the string that
    needs to be macro-expanded.  Our C/C++ tokenizer isn't really
    designed to be called by anything but the yacc parser engine.  */
-char *macro_expand_next (const char **lexptr, const macro_scope &scope);
+gdb::unique_xmalloc_ptr<char> macro_expand_next (const char **lexptr,
+						 const macro_scope &scope);
 
 /* Functions to classify characters according to cpp rules.  */