[PATCH 0/3] Small cleanups in macro code

[PATCH 0/3] Small cleanups in macro code

[Simon Marchi]

I am trying to fix this warning given by clang-10, which points out that we
have a user-defined destructor in class macro_buffer, but no user-defined copy
constructor:

      CXX    macroexp.o
    /home/simark/src/binutils-gdb/gdb/macroexp.c:125:3: error: definition of implicit copy constructor for 'macro_buffer' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated-copy-dtor]
      ~macro_buffer ()
      ^

I still have not managed to untangle this code, but I came up with this small
cleanups which should be quite obvious.

Simon Marchi (3):
  gdb: remove callback in macro expand functions
  gdb: make macro_expand_next return a gdb::unique_xmalloc_ptr<char>
  gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

 gdb/c-exp.y      | 19 +++++---------
 gdb/macrocmd.c   | 22 ++++++++--------
 gdb/macroexp.c   | 67 +++++++++++++++++++-----------------------------
 gdb/macroexp.h   | 59 ++++++++++++++++--------------------------
 gdb/macroscope.c | 14 +++++-----
 gdb/macroscope.h |  9 +++----
 gdb/macrotab.c   | 14 +++-------
 7 files changed, 79 insertions(+), 125 deletions(-)

-- 
2.27.0

[PATCH 1/3] gdb: remove callback in macro expand functions

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

I started to look into changing the callbacks in macroexp.h to use
gdb::function_view.  However, I noticed that the passed lookup function
was always `standard_macro_lookup`, which looks up a macro in a
`macro_scope` object.  Since that doesn't look like a very useful
abstraction, it would be simpler to just pass the scope around and have
the various functions call standard_macro_lookup themselves.  This is
what this patch does.

gdb/ChangeLog:

	* macroexp.h (macro_lookup_ftype): Remove.
	(macro_expand, macro_expand_once, macro_expand_next): Remove
	lookup function parameters, add scope parameter.
	* macroexp.c (scan, substitute_args, expand, maybe_expand,
	macro_expand, macro_expand_once, macro_expand_next): Likewise.
	* macroscope.h (standard_macro_lookup): Change parameter type
	to macro_scope.
	* macroscope.c (standard_macro_lookup): Likewise.
	* c-exp.y (lex_one_token): Update.
	* macrocmd.c (macro_expand_command): Likewise.
	(macro_expand_once_command): Likewise.

Change-Id: Id2431b1489359e1b0274dc2b81e5ea5d225d730c
---
 gdb/c-exp.y      |  3 +--
 gdb/macrocmd.c   | 22 ++++++++++----------
 gdb/macroexp.c   | 49 +++++++++++++++++----------------------------
 gdb/macroexp.h   | 52 ++++++++++++++++++------------------------------
 gdb/macroscope.c | 14 ++++++-------
 gdb/macroscope.h |  9 +++------
 6 files changed, 58 insertions(+), 91 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 5ec84eb8ed7a..61fa2fe684db 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -2632,8 +2632,7 @@ lex_one_token (struct parser_state *par_state, bool *is_quoted_name)
   if (! scanning_macro_expansion ())
     {
       char *expanded = macro_expand_next (&pstate->lexptr,
-                                          standard_macro_lookup,
-                                          expression_macro_scope);
+					  *expression_macro_scope);
 
       if (expanded)
         scan_macro_expansion (expanded);
diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
index 42915db90442..3e900fefaf50 100644
--- a/gdb/macrocmd.c
+++ b/gdb/macrocmd.c
@@ -47,9 +47,6 @@ macro_inform_no_debuginfo (void)
 static void
 macro_expand_command (const char *exp, int from_tty)
 {
-  gdb::unique_xmalloc_ptr<struct macro_scope> ms;
-  gdb::unique_xmalloc_ptr<char> expanded;
-
   /* You know, when the user doesn't specify any expression, it would be
      really cool if this defaulted to the last expression evaluated.
      Then it would be easy to ask, "Hey, what did I just evaluate?"  But
@@ -60,10 +57,12 @@ macro_expand_command (const char *exp, int from_tty)
            " expression you\n"
            "want to expand."));
 
-  ms = default_macro_scope ();
-  if (ms)
+  gdb::unique_xmalloc_ptr<macro_scope> ms = default_macro_scope ();
+
+  if (ms != nullptr)
     {
-      expanded = macro_expand (exp, standard_macro_lookup, ms.get ());
+      gdb::unique_xmalloc_ptr<char> expanded = macro_expand (exp, *ms);
+
       fputs_filtered ("expands to: ", gdb_stdout);
       fputs_filtered (expanded.get (), gdb_stdout);
       fputs_filtered ("\n", gdb_stdout);
@@ -76,9 +75,6 @@ macro_expand_command (const char *exp, int from_tty)
 static void
 macro_expand_once_command (const char *exp, int from_tty)
 {
-  gdb::unique_xmalloc_ptr<struct macro_scope> ms;
-  gdb::unique_xmalloc_ptr<char> expanded;
-
   /* You know, when the user doesn't specify any expression, it would be
      really cool if this defaulted to the last expression evaluated.
      And it should set the once-expanded text as the new `last
@@ -89,10 +85,12 @@ macro_expand_once_command (const char *exp, int from_tty)
            " the expression\n"
            "you want to expand."));
 
-  ms = default_macro_scope ();
-  if (ms)
+  gdb::unique_xmalloc_ptr<macro_scope> ms = default_macro_scope ();
+
+  if (ms != nullptr)
     {
-      expanded = macro_expand_once (exp, standard_macro_lookup, ms.get ());
+      gdb::unique_xmalloc_ptr<char> expanded = macro_expand_once (exp, *ms);
+
       fputs_filtered ("expands to: ", gdb_stdout);
       fputs_filtered (expanded.get (), gdb_stdout);
       fputs_filtered ("\n", gdb_stdout);
diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index 9015bc1c9d40..92823807f153 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -21,6 +21,7 @@
 #include "gdb_obstack.h"
 #include "macrotab.h"
 #include "macroexp.h"
+#include "macroscope.h"
 #include "c-lang.h"
 
 
@@ -877,9 +878,7 @@ gather_arguments (const char *name, struct macro_buffer *src, int nargs,
 static void scan (struct macro_buffer *dest,
                   struct macro_buffer *src,
                   struct macro_name_list *no_loop,
-                  macro_lookup_ftype *lookup_func,
-                  void *lookup_baton);
-
+		  const macro_scope &scope);
 
 /* A helper function for substitute_args.
    
@@ -959,8 +958,7 @@ substitute_args (struct macro_buffer *dest,
 		 int is_varargs, const struct macro_buffer *va_arg_name,
 		 const std::vector<struct macro_buffer> &argv,
                  struct macro_name_list *no_loop,
-                 macro_lookup_ftype *lookup_func,
-                 void *lookup_baton)
+		 const macro_scope &scope)
 {
   /* The token we are currently considering.  */
   struct macro_buffer tok;
@@ -1194,7 +1192,7 @@ substitute_args (struct macro_buffer *dest,
 		 referring to the argument's text, not the argument
 		 itself.  */
 	      struct macro_buffer arg_src (argv[arg].text, argv[arg].len);
-	      scan (dest, &arg_src, no_loop, lookup_func, lookup_baton);
+	      scan (dest, &arg_src, no_loop, scope);
 	      substituted = 1;
 	    }
 
@@ -1224,8 +1222,7 @@ expand (const char *id,
         struct macro_buffer *dest,
         struct macro_buffer *src,
         struct macro_name_list *no_loop,
-        macro_lookup_ftype *lookup_func,
-        void *lookup_baton)
+	const macro_scope &scope)
 {
   struct macro_name_list new_no_loop;
 
@@ -1243,7 +1240,7 @@ expand (const char *id,
       struct macro_buffer replacement_list (def->replacement,
 					    strlen (def->replacement));
 
-      scan (dest, &replacement_list, &new_no_loop, lookup_func, lookup_baton);
+      scan (dest, &replacement_list, &new_no_loop, scope);
       return 1;
     }
   else if (def->kind == macro_function_like)
@@ -1310,7 +1307,7 @@ expand (const char *id,
          expand an argument until we see how it's being used.  */
       struct macro_buffer substituted (0);
       substitute_args (&substituted, def, is_varargs, &va_arg_name,
-		       argv, no_loop, lookup_func, lookup_baton);
+		       argv, no_loop, scope);
 
       /* Now `substituted' is the macro's replacement list, with all
          argument values substituted into it properly.  Re-scan it for
@@ -1323,7 +1320,7 @@ expand (const char *id,
          `substituted's original text buffer after scanning it so we
          can free it.  */
       struct macro_buffer substituted_src (substituted.text, substituted.len);
-      scan (dest, &substituted_src, &new_no_loop, lookup_func, lookup_baton);
+      scan (dest, &substituted_src, &new_no_loop, scope);
 
       return 1;
     }
@@ -1344,8 +1341,7 @@ maybe_expand (struct macro_buffer *dest,
               struct macro_buffer *src_first,
               struct macro_buffer *src_rest,
               struct macro_name_list *no_loop,
-              macro_lookup_ftype *lookup_func,
-              void *lookup_baton)
+	      const macro_scope &scope)
 {
   gdb_assert (src_first->shared);
   gdb_assert (src_rest->shared);
@@ -1363,11 +1359,9 @@ maybe_expand (struct macro_buffer *dest,
       if (! currently_rescanning (no_loop, id.c_str ()))
         {
           /* Does this identifier have a macro definition in scope?  */
-          struct macro_definition *def = lookup_func (id.c_str (),
-						      lookup_baton);
+          macro_definition *def = standard_macro_lookup (id.c_str (), scope);
 
-          if (def && expand (id.c_str (), def, dest, src_rest, no_loop,
-                             lookup_func, lookup_baton))
+          if (def && expand (id.c_str (), def, dest, src_rest, no_loop, scope))
 	    return 1;
         }
     }
@@ -1385,8 +1379,7 @@ static void
 scan (struct macro_buffer *dest,
       struct macro_buffer *src,
       struct macro_name_list *no_loop,
-      macro_lookup_ftype *lookup_func,
-      void *lookup_baton)
+      const macro_scope &scope)
 {
   gdb_assert (src->shared);
   gdb_assert (! dest->shared);
@@ -1408,7 +1401,7 @@ scan (struct macro_buffer *dest,
           dest->last_token = dest->len;
         }
 
-      if (! maybe_expand (dest, &tok, src, no_loop, lookup_func, lookup_baton))
+      if (! maybe_expand (dest, &tok, src, no_loop, scope))
         /* We didn't end up expanding tok as a macro reference, so
            simply append it to dest.  */
         append_tokens_without_splicing (dest, &tok);
@@ -1425,16 +1418,14 @@ scan (struct macro_buffer *dest,
 
 
 gdb::unique_xmalloc_ptr<char>
-macro_expand (const char *source,
-              macro_lookup_ftype *lookup_func,
-              void *lookup_func_baton)
+macro_expand (const char *source, const macro_scope &scope)
 {
   struct macro_buffer src (source, strlen (source));
 
   struct macro_buffer dest (0);
   dest.last_token = 0;
 
-  scan (&dest, &src, 0, lookup_func, lookup_func_baton);
+  scan (&dest, &src, 0, scope);
 
   dest.appendc ('\0');
 
@@ -1443,18 +1434,14 @@ macro_expand (const char *source,
 
 
 gdb::unique_xmalloc_ptr<char>
-macro_expand_once (const char *source,
-                   macro_lookup_ftype *lookup_func,
-                   void *lookup_func_baton)
+macro_expand_once (const char *source, const macro_scope &scope)
 {
   error (_("Expand-once not implemented yet."));
 }
 
 
 char *
-macro_expand_next (const char **lexptr,
-                   macro_lookup_ftype *lookup_func,
-                   void *lookup_baton)
+macro_expand_next (const char **lexptr, const macro_scope &scope)
 {
   struct macro_buffer tok;
 
@@ -1470,7 +1457,7 @@ macro_expand_next (const char **lexptr,
     return 0;
 
   /* If it's a macro invocation, expand it.  */
-  if (maybe_expand (&dest, &tok, &src, 0, lookup_func, lookup_baton))
+  if (maybe_expand (&dest, &tok, &src, 0, scope))
     {
       /* It was a macro invocation!  Package up the expansion as a
          null-terminated string and return it.  Set *lexptr to the
diff --git a/gdb/macroexp.h b/gdb/macroexp.h
index bbb0ecd109bf..ec992f22796d 100644
--- a/gdb/macroexp.h
+++ b/gdb/macroexp.h
@@ -21,38 +21,26 @@
 #ifndef MACROEXP_H
 #define MACROEXP_H
 
-/* A function for looking up preprocessor macro definitions.  Return
-   the preprocessor definition of NAME in scope according to BATON, or
-   zero if NAME is not defined as a preprocessor macro.
-
-   The caller must not free or modify the definition returned.  It is
-   probably unwise for the caller to hold pointers to it for very
-   long; it probably lives in some objfile's obstacks.  */
-typedef struct macro_definition *(macro_lookup_ftype) (const char *name,
-                                                       void *baton);
-
-
-/* Expand any preprocessor macros in SOURCE, and return the expanded
-   text.  Use LOOKUP_FUNC and LOOKUP_FUNC_BATON to find identifiers'
-   preprocessor definitions.  SOURCE is a null-terminated string.  The
-   result is a null-terminated string, allocated using xmalloc; it is
-   the caller's responsibility to free it.  */
+struct macro_scope;
+
+/* Expand any preprocessor macros in SOURCE (a null-terminated string), and
+   return the expanded text.
+
+   Use SCOPE to find identifiers' preprocessor definitions.
+
+   The result is a null-terminated string.  */
 gdb::unique_xmalloc_ptr<char> macro_expand (const char *source,
-					    macro_lookup_ftype *lookup_func,
-					    void *lookup_func_baton);
+					    const macro_scope &scope);
 
+/* Expand all preprocessor macro references that appear explicitly in SOURCE
+   (a null-terminated string), but do not expand any new macro references
+   introduced by that first level of expansion.
 
-/* Expand all preprocessor macro references that appear explicitly in
-   SOURCE, but do not expand any new macro references introduced by
-   that first level of expansion.  Use LOOKUP_FUNC and
-   LOOKUP_FUNC_BATON to find identifiers' preprocessor definitions.
-   SOURCE is a null-terminated string.  The result is a
-   null-terminated string, allocated using xmalloc; it is the caller's
-   responsibility to free it.  */
-gdb::unique_xmalloc_ptr<char> macro_expand_once (const char *source,
-						 macro_lookup_ftype *lookup_func,
-						 void *lookup_func_baton);
+   Use SCOPE to find identifiers' preprocessor definitions.
 
+   The result is a null-terminated string.  */
+gdb::unique_xmalloc_ptr<char> macro_expand_once (const char *source,
+						 const macro_scope &scope);
 
 /* If the null-terminated string pointed to by *LEXPTR begins with a
    macro invocation, return the result of expanding that invocation as
@@ -61,9 +49,9 @@ gdb::unique_xmalloc_ptr<char> macro_expand_once (const char *source,
    contains no further macro invocations.
 
    Otherwise, if *LEXPTR does not start with a macro invocation,
-   return zero, and leave *LEXPTR unchanged.
+   return nullptr, and leave *LEXPTR unchanged.
 
-   Use LOOKUP_FUNC and LOOKUP_BATON to find macro definitions.
+   Use SCOPE to find macro definitions.
 
    If this function returns a string, the caller is responsible for
    freeing it, using xfree.
@@ -80,9 +68,7 @@ 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,
-                         macro_lookup_ftype *lookup_func,
-                         void *lookup_baton);
+char *macro_expand_next (const char **lexptr, const macro_scope &scope);
 
 /* Functions to classify characters according to cpp rules.  */
 
diff --git a/gdb/macroscope.c b/gdb/macroscope.c
index 9a1e7fe633be..3b02c97528e1 100644
--- a/gdb/macroscope.c
+++ b/gdb/macroscope.c
@@ -140,15 +140,15 @@ default_macro_scope (void)
    location given by BATON, which must be a pointer to a `struct
    macro_scope' structure.  */
 struct macro_definition *
-standard_macro_lookup (const char *name, void *baton)
+standard_macro_lookup (const char *name, const macro_scope &ms)
 {
-  struct macro_scope *ms = (struct macro_scope *) baton;
-  struct macro_definition *result;
-
   /* Give user-defined macros priority over all others.  */
-  result = macro_lookup_definition (macro_main (macro_user_macros), -1, name);
-  if (! result)
-    result = macro_lookup_definition (ms->file, ms->line, name);
+  macro_definition *result
+    = macro_lookup_definition (macro_main (macro_user_macros), -1, name);
+
+  if (result == nullptr)
+    result = macro_lookup_definition (ms.file, ms.line, name);
+
   return result;
 }
 
diff --git a/gdb/macroscope.h b/gdb/macroscope.h
index a105094a9d6a..6ff3579d6b10 100644
--- a/gdb/macroscope.h
+++ b/gdb/macroscope.h
@@ -56,12 +56,9 @@ gdb::unique_xmalloc_ptr<struct macro_scope> user_macro_scope (void);
    the user macro scope.  */
 gdb::unique_xmalloc_ptr<struct macro_scope> default_macro_scope (void);
 
-
 /* Look up the definition of the macro named NAME in scope at the source
-   location given by BATON, which must be a pointer to a `struct
-   macro_scope' structure.  This function is suitable for use as
-   a macro_lookup_ftype function.  */
-struct macro_definition *standard_macro_lookup (const char *name, void *baton);
-
+   location given by MS.  */
+macro_definition *standard_macro_lookup (const char *name,
+					 const macro_scope &ms);
 
 #endif /* MACROSCOPE_H */
-- 
2.27.0

Re: [PATCH 1/3] gdb: remove callback in macro expand functions

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> I started to look into changing the callbacks in macroexp.h to use
Simon> gdb::function_view.  However, I noticed that the passed lookup function
Simon> was always `standard_macro_lookup`, which looks up a macro in a
Simon> `macro_scope` object.  Since that doesn't look like a very useful
Simon> abstraction, it would be simpler to just pass the scope around and have
Simon> the various functions call standard_macro_lookup themselves.  This is
Simon> what this patch does.

This seems fine to me.

I don't remember, but maybe this lookup function used to vary at some
point in the past.  That would explain the abstraction anyway.  Or maybe
it was just planned somehow.

Tom

Re: [PATCH 1/3] gdb: remove callback in macro expand functions

[Matt Rice]

On Tue, Jun 30, 2020 at 8:47 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> I started to look into changing the callbacks in macroexp.h to use
> Simon> gdb::function_view.  However, I noticed that the passed lookup function
> Simon> was always `standard_macro_lookup`, which looks up a macro in a
> Simon> `macro_scope` object.  Since that doesn't look like a very useful
> Simon> abstraction, it would be simpler to just pass the scope around and have
> Simon> the various functions call standard_macro_lookup themselves.  This is
> Simon> what this patch does.
>
> This seems fine to me.
>
> I don't remember, but maybe this lookup function used to vary at some
> point in the past.  That would explain the abstraction anyway.  Or maybe
> it was just planned somehow.

It looks like it never varied, I think that perhaps varying it could
have been in anticipation of the
macro_expand_once, for storing partial expansion tables in the baton
perhaps, it looks like
all the other unimplemented commands at the time of initial commit
have been implemented without it
(though I didn't do an exhaustive look) anyhow that is just a guess *shrug*.

Re: [PATCH 1/3] gdb: remove callback in macro expand functions

[Simon Marchi]

On 2020-06-30 6:14 p.m., Matt Rice via Gdb-patches wrote:
> On Tue, Jun 30, 2020 at 8:47 PM Tom Tromey <tom@tromey.com> wrote:
>>
>>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>> Simon> From: Simon Marchi <simon.marchi@efficios.com>
>> Simon> I started to look into changing the callbacks in macroexp.h to use
>> Simon> gdb::function_view.  However, I noticed that the passed lookup function
>> Simon> was always `standard_macro_lookup`, which looks up a macro in a
>> Simon> `macro_scope` object.  Since that doesn't look like a very useful
>> Simon> abstraction, it would be simpler to just pass the scope around and have
>> Simon> the various functions call standard_macro_lookup themselves.  This is
>> Simon> what this patch does.
>>
>> This seems fine to me.
>>
>> I don't remember, but maybe this lookup function used to vary at some
>> point in the past.  That would explain the abstraction anyway.  Or maybe
>> it was just planned somehow.
> 
> It looks like it never varied, I think that perhaps varying it could
> have been in anticipation of the
> macro_expand_once, for storing partial expansion tables in the baton
> perhaps, it looks like
> all the other unimplemented commands at the time of initial commit
> have been implemented without it
> (though I didn't do an exhaustive look) anyhow that is just a guess *shrug*.
> 

Ok, thanks for checking.  In any case, if this abstraction is ever actually
needed, it won't be really hard to add it back.  And if not, then we can just
enjoy the simpler code.

Simon

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

[Simon Marchi]

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

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 ();
 }
 
 \f
@@ -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.  */
 
-- 
2.27.0

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

[Tom Tromey]

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

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

[Simon Marchi]

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 3/3] gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

The change to macro_stringify is straightforward.  This allows removing
the manual memory management in fixup_definition.

gdb/ChangeLog:

	* macroexp.h (macro_stringify): Return
	gdb::unique_xmalloc_ptr<char>.
	* macroexp.c (macro_stringify): Likewise.
	* macrotab.c (fixup_definition): Update.

Change-Id: Id7db8988bdbd569dd51c4f4655b00eb26db277cb
---
 gdb/macroexp.c |  4 ++--
 gdb/macroexp.h |  6 ++----
 gdb/macrotab.c | 14 ++++----------
 3 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index e1d185d30c8e..5f749ffe8928 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -698,7 +698,7 @@ stringify (struct macro_buffer *dest, const char *arg, int len)
 
 /* See macroexp.h.  */
 
-char *
+gdb::unique_xmalloc_ptr<char>
 macro_stringify (const char *str)
 {
   int len = strlen (str);
@@ -707,7 +707,7 @@ macro_stringify (const char *str)
   stringify (&buffer, str, len);
   buffer.appendc ('\0');
 
-  return buffer.release ().release ();
+  return buffer.release ();
 }
 
 \f
diff --git a/gdb/macroexp.h b/gdb/macroexp.h
index 511991cacd20..2e29d02d34bd 100644
--- a/gdb/macroexp.h
+++ b/gdb/macroexp.h
@@ -78,9 +78,7 @@ int macro_is_identifier_nondigit (int c);
 int macro_is_digit (int c);
 
 
-/* Stringify STR according to C rules and return an xmalloc'd pointer
-   to the result.  */
-
-char *macro_stringify (const char *str);
+/* Stringify STR according to C rules and return a null-terminated string.  */
+gdb::unique_xmalloc_ptr<char> macro_stringify (const char *str);
 
 #endif /* MACROEXP_H */
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 63cd30148ac2..0fefe8faaacf 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -882,25 +882,19 @@ macro_undef (struct macro_source_file *source, int line,
 static struct macro_definition *
 fixup_definition (const char *filename, int line, struct macro_definition *def)
 {
-  static char *saved_expansion;
-
-  if (saved_expansion)
-    {
-      xfree (saved_expansion);
-      saved_expansion = NULL;
-    }
+  gdb::unique_xmalloc_ptr<char> saved_expansion;
 
   if (def->kind == macro_object_like)
     {
       if (def->argc == macro_FILE)
 	{
 	  saved_expansion = macro_stringify (filename);
-	  def->replacement = saved_expansion;
+	  def->replacement = saved_expansion.get ();
 	}
       else if (def->argc == macro_LINE)
 	{
-	  saved_expansion = xstrprintf ("%d", line);
-	  def->replacement = saved_expansion;
+	  saved_expansion.reset (xstrprintf ("%d", line));
+	  def->replacement = saved_expansion.get ();
 	}
     }
 
-- 
2.27.0

Re: [PATCH 3/3] gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> The change to macro_stringify is straightforward.  This allows removing
Simon> the manual memory management in fixup_definition.

Simon> @@ -882,25 +882,19 @@ macro_undef (struct macro_source_file *source, int line,
Simon>  static struct macro_definition *
Simon>  fixup_definition (const char *filename, int line, struct macro_definition *def)
Simon>  {
Simon> -  static char *saved_expansion;
[...]
Simon> +  gdb::unique_xmalloc_ptr<char> saved_expansion;
 
This loses the "static", but that's necessary because this function
returns "def", which has a pointer to the contents.

Tom

Re: [PATCH 3/3] gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

On 2020-06-30 4:55 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> The change to macro_stringify is straightforward.  This allows removing
> Simon> the manual memory management in fixup_definition.
> 
> Simon> @@ -882,25 +882,19 @@ macro_undef (struct macro_source_file *source, int line,
> Simon>  static struct macro_definition *
> Simon>  fixup_definition (const char *filename, int line, struct macro_definition *def)
> Simon>  {
> Simon> -  static char *saved_expansion;
> [...]
> Simon> +  gdb::unique_xmalloc_ptr<char> saved_expansion;
>  
> This loses the "static", but that's necessary because this function
> returns "def", which has a pointer to the contents.

Oh damn, I fixed it last minute (it fails some tests) but probably forgot to git-add.

Here's the fixed version:


From e0ce95d17288009301534846fcb8c278f5d58fd7 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sat, 27 Jun 2020 16:12:20 -0400
Subject: [PATCH] gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

The change to macro_stringify is straightforward.  This allows removing
the manual memory management in fixup_definition.

gdb/ChangeLog:

	* macroexp.h (macro_stringify): Return
	gdb::unique_xmalloc_ptr<char>.
	* macroexp.c (macro_stringify): Likewise.
	* macrotab.c (fixup_definition): Update.

Change-Id: Id7db8988bdbd569dd51c4f4655b00eb26db277cb
---

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index e1d185d..5f749ff 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -698,7 +698,7 @@

 /* See macroexp.h.  */

-char *
+gdb::unique_xmalloc_ptr<char>
 macro_stringify (const char *str)
 {
   int len = strlen (str);
@@ -707,7 +707,7 @@
   stringify (&buffer, str, len);
   buffer.appendc ('\0');

-  return buffer.release ().release ();
+  return buffer.release ();
 }

 \f
diff --git a/gdb/macroexp.h b/gdb/macroexp.h
index 511991c..2e29d02 100644
--- a/gdb/macroexp.h
+++ b/gdb/macroexp.h
@@ -78,9 +78,7 @@
 int macro_is_digit (int c);


-/* Stringify STR according to C rules and return an xmalloc'd pointer
-   to the result.  */
-
-char *macro_stringify (const char *str);
+/* Stringify STR according to C rules and return a null-terminated string.  */
+gdb::unique_xmalloc_ptr<char> macro_stringify (const char *str);

 #endif /* MACROEXP_H */
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 63cd301..9ada436 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -882,25 +882,19 @@
 static struct macro_definition *
 fixup_definition (const char *filename, int line, struct macro_definition *def)
 {
-  static char *saved_expansion;
-
-  if (saved_expansion)
-    {
-      xfree (saved_expansion);
-      saved_expansion = NULL;
-    }
+  static gdb::unique_xmalloc_ptr<char> saved_expansion;

   if (def->kind == macro_object_like)
     {
       if (def->argc == macro_FILE)
 	{
 	  saved_expansion = macro_stringify (filename);
-	  def->replacement = saved_expansion;
+	  def->replacement = saved_expansion.get ();
 	}
       else if (def->argc == macro_LINE)
 	{
-	  saved_expansion = xstrprintf ("%d", line);
-	  def->replacement = saved_expansion;
+	  saved_expansion.reset (xstrprintf ("%d", line));
+	  def->replacement = saved_expansion.get ();
 	}
     }

Re: [PATCH 3/3] gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

[Tom Tromey]

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

Simon> Oh damn, I fixed it last minute (it fails some tests) but probably forgot to git-add.

Simon> Here's the fixed version:

Thanks, this one looks good.

Tom

Re: [PATCH 3/3] gdb: make macro_stringify return a gdb::unique_xmalloc_ptr<char>

[Simon Marchi]

On 2020-07-01 11:36 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Oh damn, I fixed it last minute (it fails some tests) but probably forgot to git-add.
> 
> Simon> Here's the fixed version:
> 
> Thanks, this one looks good.
> 
> Tom
> 

Thanks, I'll push the series.

Simon