[1/6] Remove some manual memory management from compile interface

Message ID 20200809135258.8207-2-tom@tromey.com
State New
Headers show
Series
  • Avoid manual memory management in gdb/compile/
Related show

Commit Message

Tom Tromey Aug. 9, 2020, 1:52 p.m.
This changes gdb's compile code to use std::vector in a couple of
places, rather than manual memory management.

gdb/ChangeLog
2020-08-08  Tom Tromey  <tom@tromey.com>

	* compile/compile-cplus-types.c
	(compile_cplus_convert_struct_or_union): Use std::vector.
	(compile_cplus_convert_func): Likewise.
	* compile/compile-c-types.c (convert_func): Use std::vector.
---
 gdb/ChangeLog                     |  7 ++++++
 gdb/compile/compile-c-types.c     |  4 ++--
 gdb/compile/compile-cplus-types.c | 39 +++++++++++++++----------------
 3 files changed, 28 insertions(+), 22 deletions(-)

-- 
2.17.2

Comments

Simon Marchi Aug. 9, 2020, 10:34 p.m. | #1
> diff --git a/gdb/compile/compile-cplus-types.c b/gdb/compile/compile-cplus-types.c

> index 02df7ab90e6..00eaaf3fa68 100644

> --- a/gdb/compile/compile-cplus-types.c

> +++ b/gdb/compile/compile-cplus-types.c

> @@ -848,33 +848,33 @@ compile_cplus_convert_struct_or_union (compile_cplus_instance *instance,

>    gcc_type result;

>    if (type->code () == TYPE_CODE_STRUCT)

>      {

> -      struct gcc_vbase_array bases;

>        int num_baseclasses = TYPE_N_BASECLASSES (type);

> +      std::vector<gcc_type> elements (num_baseclasses);

> +      std::vector<enum gcc_cp_symbol_kind> flags (num_baseclasses);

>  

> -      memset (&bases, 0, sizeof (bases));

> +      struct gcc_vbase_array bases {};

> +      bases.elements = elements.data ();

>  

>        if (num_baseclasses > 0)


You could now remove this `if (num_baseclasses > 0)`, there's no reason for it anymore I think.

Otherwise, LGTM.

Simon
Tom Tromey Sept. 19, 2020, 11:45 p.m. | #2
Simon> You could now remove this `if (num_baseclasses > 0)`, there's no
Simon> reason for it anymore I think.

I looked at this more deeply, and based on the compiler library, I think
the rule is that a 0-length array should be passed as nullptr here:

  status
  marshall (connection *conn, const gcc_vbase_array *a)
  {
    size_t len;

    if (a)
      len = a->n_elements;
    else
      len = (size_t)-1;

    if (!marshall_array_start (conn, 'v', len))
      return FAIL;

    if (!a)
      return OK;
   [...]


So, I changed this code to use your suggestion and then also modified
the final call:

      result = instance->plugin ().start_class_type
	(name.get (), resuld, num_baseclasses > 0 ? &bases : nullptr,
	 filename, line);

This didn't result in any test result changes, but I didn't look to see
if this case is not tested or something like that.

Tom
Simon Marchi Sept. 19, 2020, 11:52 p.m. | #3
On 2020-09-19 7:45 p.m., Tom Tromey wrote:
> Simon> You could now remove this `if (num_baseclasses > 0)`, there's no

> Simon> reason for it anymore I think.

>

> I looked at this more deeply, and based on the compiler library, I think

> the rule is that a 0-length array should be passed as nullptr here:

>

>   status

>   marshall (connection *conn, const gcc_vbase_array *a)

>   {

>     size_t len;

>

>     if (a)

>       len = a->n_elements;

>     else

>       len = (size_t)-1;

>

>     if (!marshall_array_start (conn, 'v', len))

>       return FAIL;

>

>     if (!a)

>       return OK;

>    [...]

>

>

> So, I changed this code to use your suggestion and then also modified

> the final call:

>

>       result = instance->plugin ().start_class_type

> 	(name.get (), resuld, num_baseclasses > 0 ? &bases : nullptr,

> 	 filename, line);

>

> This didn't result in any test result changes, but I didn't look to see

> if this case is not tested or something like that.


Oh, I didn't realize that the change I proposed would change how we call
the function.  What you ended up doing makes sense, if that's what's the
compiler library expects.

Simon

Patch

diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 2b25783bb00..7816ececb25 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -176,13 +176,13 @@  convert_func (compile_c_instance *context, struct type *type)
   return_type = context->convert_type (target_type);
 
   array.n_elements = type->num_fields ();
-  array.elements = XNEWVEC (gcc_type, type->num_fields ());
+  std::vector<gcc_type> elements (array.n_elements);
+  array.elements = elements.data ();
   for (i = 0; i < type->num_fields (); ++i)
     array.elements[i] = context->convert_type (type->field (i).type ());
 
   result = context->plugin ().build_function_type (return_type,
 						   &array, is_varargs);
-  xfree (array.elements);
 
   return result;
 }
diff --git a/gdb/compile/compile-cplus-types.c b/gdb/compile/compile-cplus-types.c
index 02df7ab90e6..00eaaf3fa68 100644
--- a/gdb/compile/compile-cplus-types.c
+++ b/gdb/compile/compile-cplus-types.c
@@ -848,33 +848,33 @@  compile_cplus_convert_struct_or_union (compile_cplus_instance *instance,
   gcc_type result;
   if (type->code () == TYPE_CODE_STRUCT)
     {
-      struct gcc_vbase_array bases;
       int num_baseclasses = TYPE_N_BASECLASSES (type);
+      std::vector<gcc_type> elements (num_baseclasses);
+      std::vector<enum gcc_cp_symbol_kind> flags (num_baseclasses);
 
-      memset (&bases, 0, sizeof (bases));
+      struct gcc_vbase_array bases {};
+      bases.elements = elements.data ();
 
       if (num_baseclasses > 0)
 	{
-	  bases.elements = XNEWVEC (gcc_type, num_baseclasses);
-	  bases.flags = XNEWVEC (enum gcc_cp_symbol_kind, num_baseclasses);
+	  bases.flags = flags.data ();
 	  bases.n_elements = num_baseclasses;
-	  for (int i = 0; i < num_baseclasses; ++i)
-	    {
-	      struct type *base_type = TYPE_BASECLASS (type, i);
-
-	      bases.flags[i] = GCC_CP_SYMBOL_BASECLASS
-		| get_field_access_flag (type, i)
-		| (BASETYPE_VIA_VIRTUAL (type, i)
-		   ? GCC_CP_FLAG_BASECLASS_VIRTUAL
-		   : GCC_CP_FLAG_BASECLASS_NOFLAG);
-	      bases.elements[i] = instance->convert_type (base_type);
-	    }
+	}
+
+      for (int i = 0; i < num_baseclasses; ++i)
+	{
+	  struct type *base_type = TYPE_BASECLASS (type, i);
+
+	  bases.flags[i] = (GCC_CP_SYMBOL_BASECLASS
+			    | get_field_access_flag (type, i)
+			    | (BASETYPE_VIA_VIRTUAL (type, i)
+			       ? GCC_CP_FLAG_BASECLASS_VIRTUAL
+			       : GCC_CP_FLAG_BASECLASS_NOFLAG));
+	  bases.elements[i] = instance->convert_type (base_type);
 	}
 
       result = instance->plugin ().start_class_type
 	(name.get (), resuld, &bases, filename, line);
-      xfree (bases.flags);
-      xfree (bases.elements);
     }
   else
     {
@@ -985,8 +985,8 @@  compile_cplus_convert_func (compile_cplus_instance *instance,
      types.  Those are impossible in C, though.  */
   gcc_type return_type = instance->convert_type (target_type);
 
-  struct gcc_type_array array =
-    { type->num_fields (), XNEWVEC (gcc_type, type->num_fields ()) };
+  std::vector<gcc_type> elements (type->num_fields ());
+  struct gcc_type_array array = { type->num_fields (), elements.data () };
   int artificials = 0;
   for (int i = 0; i < type->num_fields (); ++i)
     {
@@ -1006,7 +1006,6 @@  compile_cplus_convert_func (compile_cplus_instance *instance,
      with some minsyms like printf (compile-cplus.exp has examples).  */
   gcc_type result = instance->plugin ().build_function_type
     (return_type, &array, is_varargs);
-  xfree (array.elements);
   return result;
 }