[PATCH 1/6] [gdb/symtab] Fix uninitialized memory in buildsym_compunit::finish_block_internal

[PATCH 1/6] [gdb/symtab] Fix uninitialized memory in buildsym_compunit::finish_block_internal

[Tom de Vries]

When running test-case gdb.dwarf2/per-bfd-sharing.exp with target board stabs,
gdb either segfaults or asserts due to reading uninitialized memory, allocated
here in buildsym_compunit::finish_block_internal:
...
	      ftype->set_fields
		((struct field *)
		 TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
...

Fix this by using TYPE_ZALLOC instead.

Tested on x86_64-linux.

PR symtab/30810
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30810
---
 gdb/buildsym.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 832b689cf03..65ce3a0f5a8 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -267,7 +267,7 @@ buildsym_compunit::finish_block_internal
 	      ftype->set_num_fields (nparams);
 	      ftype->set_fields
 		((struct field *)
-		 TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
+		 TYPE_ZALLOC (ftype, nparams * sizeof (struct field)));
 
 	      iparams = 0;
 	      /* Here we want to directly access the dictionary, because

base-commit: 59487af3c8490bc5961d330bc0ef4d5f05ecdc59
-- 
2.35.3

[PATCH 2/6] [gdb/symtab] Factor out type::{alloc_fields,copy_fields}

[Tom de Vries]

After finding this code in buildsym_compunit::finish_block_internal:
...
              ftype->set_fields
                ((struct field *)
                 TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
...
and fixing PR30810 by using TYPE_ZALLOC, I wondered if there were more
locations that needed fixing.

I decided to make things easier to spot by factoring out a new function
alloc_fields:
...
 /* Allocate the fields array of this type, with NFIELDS elements.  If INIT,
     zero-initialize the allocated memory.  */
  void alloc_fields (unsigned int nfields, bool init = true);
...
where:
- a regular use would be "alloc_fields (nfields)", and
- an exceptional use that needed no initialization would be
  "alloc_fields (nfields, false)".

Pretty soon I discovered that most of the latter cases are due to
initialization by memcpy, so I added two variants of copy_fields as well.

After this rewrite there are 8 uses of set_fields left:
...
gdb/coffread.c:	  type->set_fields (nullptr);
gdb/coffread.c:	  type->set_fields (nullptr);
gdb/coffread.c:	  type->set_fields (nullptr);
gdb/eval.c:  type->set_fields
gdb/gdbtypes.c:  type->set_fields (args);
gdb/gdbtypes.c:  t->set_fields (XRESIZEVEC (struct field, t->fields (),
gdb/dwarf2/read.c:      type->set_fields (new_fields);
gdb/dwarf2/read.c:	      sub_type->set_fields (sub_type->fields () + 1);
...

These fall into the following categories:
- set to nullptr (coffread.c),
- type not owned by objfile or gdbarch (eval.c), and
- modifying an existing fields array, like adding an element at the end or
  dropping an element at the start (the rest).

Tested on x86_64-linux.
---
 gdb/ada-lang.c     |  19 ++------
 gdb/amdgpu-tdep.c  |   5 +--
 gdb/buildsym.c     |   5 +--
 gdb/coffread.c     |  15 ++-----
 gdb/ctfread.c      |   7 +--
 gdb/dwarf2/read.c  |  40 +++--------------
 gdb/gdbtypes.c     |  88 ++++++++++++++++++++++---------------
 gdb/gdbtypes.h     |   8 ++++
 gdb/gnu-v3-abi.c   | 106 ++++++++++++++++++++++++---------------------
 gdb/mdebugread.c   |   9 +---
 gdb/rust-lang.c    |   4 +-
 gdb/stabsread.c    |  21 ++-------
 gdb/windows-tdep.c |   4 +-
 13 files changed, 141 insertions(+), 190 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 575568cffb5..5b856da3189 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -7772,9 +7772,7 @@ ada_template_to_fixed_record_type_1 (struct type *type,
   rtype = type_allocator (type).new_type ();
   rtype->set_code (TYPE_CODE_STRUCT);
   INIT_NONE_SPECIFIC (rtype);
-  rtype->set_num_fields (nfields);
-  rtype->set_fields
-   ((struct field *) TYPE_ZALLOC (rtype, nfields * sizeof (struct field)));
+  rtype->alloc_fields (nfields);
   rtype->set_name (ada_type_name (type));
   rtype->set_is_fixed_instance (true);
 
@@ -8024,14 +8022,8 @@ template_to_static_fixed_type (struct type *type0)
 	      type0->set_target_type (type);
 	      type->set_code (type0->code ());
 	      INIT_NONE_SPECIFIC (type);
-	      type->set_num_fields (nfields);
 
-	      field *fields =
-		((struct field *)
-		 TYPE_ALLOC (type, nfields * sizeof (struct field)));
-	      memcpy (fields, type0->fields (),
-		      sizeof (struct field) * nfields);
-	      type->set_fields (fields);
+	      type->copy_fields (type0);
 
 	      type->set_name (ada_type_name (type0));
 	      type->set_is_fixed_instance (true);
@@ -8077,12 +8069,7 @@ to_record_with_fixed_variant_part (struct type *type, const gdb_byte *valaddr,
   rtype = type_allocator (type).new_type ();
   rtype->set_code (TYPE_CODE_STRUCT);
   INIT_NONE_SPECIFIC (rtype);
-  rtype->set_num_fields (nfields);
-
-  field *fields =
-    (struct field *) TYPE_ALLOC (rtype, nfields * sizeof (struct field));
-  memcpy (fields, type->fields (), sizeof (struct field) * nfields);
-  rtype->set_fields (fields);
+  rtype->copy_fields (type);
 
   rtype->set_name (ada_type_name (type));
   rtype->set_is_fixed_instance (true);
diff --git a/gdb/amdgpu-tdep.c b/gdb/amdgpu-tdep.c
index 21a7a3ae1b7..3a521a43977 100644
--- a/gdb/amdgpu-tdep.c
+++ b/gdb/amdgpu-tdep.c
@@ -757,10 +757,7 @@ amd_dbgapi_register_type_to_gdb_type (const amd_dbgapi_register_type &type,
 	     .new_type (TYPE_CODE_ENUM, enum_type.bit_size (),
 			enum_type.name ().c_str ()));
 
-	gdb_type->set_num_fields (enum_type.size ());
-	gdb_type->set_fields
-	  ((struct field *) TYPE_ZALLOC (gdb_type, (sizeof (struct field)
-						    * enum_type.size ())));
+	gdb_type->alloc_fields (enum_type.size ());
 	gdb_type->set_is_unsigned (true);
 
 	for (size_t i = 0; i < enum_type.size (); ++i)
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 65ce3a0f5a8..3c1e9179411 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -264,10 +264,7 @@ buildsym_compunit::finish_block_internal
 	    }
 	  if (nparams > 0)
 	    {
-	      ftype->set_num_fields (nparams);
-	      ftype->set_fields
-		((struct field *)
-		 TYPE_ZALLOC (ftype, nparams * sizeof (struct field)));
+	      ftype->alloc_fields (nparams);
 
 	      iparams = 0;
 	      /* Here we want to directly access the dictionary, because
diff --git a/gdb/coffread.c b/gdb/coffread.c
index c2fe9fa1761..e132dd1a1f7 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1462,14 +1462,9 @@ patch_type (struct type *type, struct type *real_type)
 {
   struct type *target = type->target_type ();
   struct type *real_target = real_type->target_type ();
-  int field_size = real_target->num_fields () * sizeof (struct field);
 
   target->set_length (real_target->length ());
-  target->set_num_fields (real_target->num_fields ());
-
-  field *fields = (struct field *) TYPE_ALLOC (target, field_size);
-  memcpy (fields, real_target->fields (), field_size);
-  target->set_fields (fields);
+  target->copy_fields (real_target);
 
   if (real_target->name ())
     {
@@ -2037,9 +2032,7 @@ coff_read_struct_type (int index, int length, int lastsym,
     }
   /* Now create the vector of fields, and record how big it is.  */
 
-  type->set_num_fields (nfields);
-  type->set_fields
-    ((struct field *) TYPE_ALLOC (type, sizeof (struct field) * nfields));
+  type->alloc_fields (nfields, false);
 
   /* Copy the saved-up fields into the field vector.  */
 
@@ -2117,9 +2110,7 @@ coff_read_enum_type (int index, int length, int lastsym,
   else /* Assume ints.  */
     type->set_length (gdbarch_int_bit (gdbarch) / TARGET_CHAR_BIT);
   type->set_code (TYPE_CODE_ENUM);
-  type->set_num_fields (nsyms);
-  type->set_fields
-    ((struct field *) TYPE_ALLOC (type, sizeof (struct field) * nsyms));
+  type->alloc_fields (nsyms, false);
 
   /* Find the symbols for the values and put them into the type.
      The symbols can be found in the symlist that we put them on
diff --git a/gdb/ctfread.c b/gdb/ctfread.c
index 16292c74b1a..859908923a4 100644
--- a/gdb/ctfread.c
+++ b/gdb/ctfread.c
@@ -346,9 +346,7 @@ attach_fields_to_type (struct ctf_field_info *fip, struct type *type)
     return;
 
   /* Record the field count, allocate space for the array of fields.  */
-  type->set_num_fields (nfields);
-  type->set_fields
-    ((struct field *) TYPE_ZALLOC (type, sizeof (struct field) * nfields));
+  type->alloc_fields (nfields);
 
   /* Copy the saved-up fields into the field vector.  */
   for (int i = 0; i < nfields; ++i)
@@ -715,8 +713,7 @@ read_func_kind_type (struct ctf_context *ccp, ctf_id_t tid)
       if (ctf_func_type_args (fp, tid, argc, argv.data ()) == CTF_ERR)
 	return nullptr;
 
-      type->set_fields
-	((struct field *) TYPE_ZALLOC (type, argc * sizeof (struct field)));
+      type->alloc_fields (argc);
       struct type *void_type = builtin_type (of)->builtin_void;
       /* If failed to find the argument type, fill it with void_type.  */
       for (int iparam = 0; iparam < argc; iparam++)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 5b32089094d..7fe0e84cdda 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6054,11 +6054,9 @@ quirk_rust_enum (struct type *type, struct objfile *objfile)
       /* Smash this type to be a structure type.  We have to do this
 	 because the type has already been recorded.  */
       type->set_code (TYPE_CODE_STRUCT);
-      type->set_num_fields (3);
       /* Save the field we care about.  */
       struct field saved_field = type->field (0);
-      type->set_fields
-	((struct field *) TYPE_ZALLOC (type, 3 * sizeof (struct field)));
+      type->alloc_fields (3);
 
       /* Put the discriminant at index 0.  */
       type->field (0).set_type (field_type);
@@ -12059,9 +12057,7 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
 
   /* Record the field count, allocate space for the array of fields,
      and create blank accessibility bitfields if necessary.  */
-  type->set_num_fields (nfields);
-  type->set_fields
-    ((struct field *) TYPE_ZALLOC (type, sizeof (struct field) * nfields));
+  type->alloc_fields (nfields);
 
   if (fip->non_public_fields && cu->lang () != language_ada)
     {
@@ -12490,12 +12486,7 @@ rewrite_array_type (struct type *type)
      updated.  Either way we want to copy the type and update
      everything.  */
   struct type *copy = copy_type (type);
-  int nfields = copy->num_fields ();
-  field *new_fields
-    = ((struct field *) TYPE_ZALLOC (copy,
-				     nfields * sizeof (struct field)));
-  memcpy (new_fields, copy->fields (), nfields * sizeof (struct field));
-  copy->set_fields (new_fields);
+  copy->copy_fields (type);
   if (new_target != nullptr)
     copy->set_target_type (new_target);
 
@@ -13260,14 +13251,7 @@ update_enumeration_type_from_children (struct die_info *die,
     }
 
   if (!fields.empty ())
-    {
-      type->set_num_fields (fields.size ());
-      type->set_fields
-	((struct field *)
-	 TYPE_ALLOC (type, sizeof (struct field) * fields.size ()));
-      memcpy (type->fields (), fields.data (),
-	      sizeof (struct field) * fields.size ());
-    }
+    type->copy_fields (fields);
   else
     flag_enum = 0;
 
@@ -13636,12 +13620,7 @@ quirk_ada_thick_pointer (struct die_info *die, struct dwarf2_cu *cu,
   struct type *bounds = alloc.new_type ();
   bounds->set_code (TYPE_CODE_STRUCT);
 
-  bounds->set_num_fields (range_fields.size ());
-  bounds->set_fields
-    ((struct field *) TYPE_ALLOC (bounds, (bounds->num_fields ()
-					   * sizeof (struct field))));
-  memcpy (bounds->fields (), range_fields.data (),
-	  bounds->num_fields () * sizeof (struct field));
+  bounds->copy_fields (range_fields);
 
   int last_fieldno = range_fields.size () - 1;
   int bounds_size = (bounds->field (last_fieldno).loc_bitpos () / 8
@@ -13664,10 +13643,7 @@ quirk_ada_thick_pointer (struct die_info *die, struct dwarf2_cu *cu,
   struct type *result = type_allocator (objfile).new_type ();
   result->set_code (TYPE_CODE_STRUCT);
 
-  result->set_num_fields (2);
-  result->set_fields
-    ((struct field *) TYPE_ZALLOC (result, (result->num_fields ()
-					    * sizeof (struct field))));
+  result->alloc_fields (2);
 
   /* The names are chosen to coincide with what the compiler does with
      -fgnat-encodings=all, which the Ada code in gdb already
@@ -14709,9 +14685,7 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
 	}
 
       /* Allocate storage for parameters and fill them in.  */
-      ftype->set_num_fields (nparams);
-      ftype->set_fields
-	((struct field *) TYPE_ZALLOC (ftype, nparams * sizeof (struct field)));
+      ftype->alloc_fields (nparams);
 
       /* TYPE_FIELD_TYPE must never be NULL.  Pre-fill the array to ensure it
 	 even if we error out during the parameters reading below.  */
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 939fcc23b82..766107a7013 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -597,9 +597,7 @@ lookup_function_type_with_arguments (struct type *type,
 	fn->set_is_prototyped (true);
     }
 
-  fn->set_num_fields (nparams);
-  fn->set_fields
-    ((struct field *) TYPE_ZALLOC (fn, nparams * sizeof (struct field)));
+  fn->alloc_fields (nparams);
   for (i = 0; i < nparams; ++i)
     fn->field (i).set_type (param_types[i]);
 
@@ -1356,9 +1354,7 @@ create_array_type_with_stride (type_allocator &alloc,
   result_type->set_code (TYPE_CODE_ARRAY);
   result_type->set_target_type (element_type);
 
-  result_type->set_num_fields (1);
-  result_type->set_fields
-    ((struct field *) TYPE_ZALLOC (result_type, sizeof (struct field)));
+  result_type->alloc_fields (1);
   result_type->set_index_type (range_type);
   if (byte_stride_prop != NULL)
     result_type->add_dyn_prop (DYN_PROP_BYTE_STRIDE, *byte_stride_prop);
@@ -1442,9 +1438,7 @@ create_set_type (type_allocator &alloc, struct type *domain_type)
   struct type *result_type = alloc.new_type ();
 
   result_type->set_code (TYPE_CODE_SET);
-  result_type->set_num_fields (1);
-  result_type->set_fields
-    ((struct field *) TYPE_ZALLOC (result_type, sizeof (struct field)));
+  result_type->alloc_fields (1);
 
   if (!domain_type->is_stub ())
     {
@@ -2452,13 +2446,7 @@ resolve_dynamic_union (struct type *type,
   gdb_assert (type->code () == TYPE_CODE_UNION);
 
   resolved_type = copy_type (type);
-  resolved_type->set_fields
-    ((struct field *)
-     TYPE_ALLOC (resolved_type,
-		 resolved_type->num_fields () * sizeof (struct field)));
-  memcpy (resolved_type->fields (),
-	  type->fields (),
-	  resolved_type->num_fields () * sizeof (struct field));
+  resolved_type->copy_fields (type);
   for (i = 0; i < resolved_type->num_fields (); ++i)
     {
       struct type *t;
@@ -2618,12 +2606,10 @@ compute_variant_fields (struct type *type,
   for (const auto &part : parts)
     compute_variant_fields_inner (type, addr_stack, part, flags);
 
-  resolved_type->set_num_fields
-    (std::count (flags.begin (), flags.end (), true));
-  resolved_type->set_fields
-    ((struct field *)
-     TYPE_ALLOC (resolved_type,
-		 resolved_type->num_fields () * sizeof (struct field)));
+  unsigned int nfields = std::count (flags.begin (), flags.end (), true);
+  /* No need to zero-initialize the newly allocated fields, they'll be
+     initialized by the copy in the loop below.  */
+  resolved_type->alloc_fields (nfields, false);
 
   int out = 0;
   for (int i = 0; i < type->num_fields (); ++i)
@@ -2664,14 +2650,7 @@ resolve_dynamic_struct (struct type *type,
     }
   else
     {
-      resolved_type->set_fields
-	((struct field *)
-	 TYPE_ALLOC (resolved_type,
-		     resolved_type->num_fields () * sizeof (struct field)));
-      if (type->num_fields () > 0)
-	memcpy (resolved_type->fields (),
-		type->fields (),
-		resolved_type->num_fields () * sizeof (struct field));
+      resolved_type->copy_fields (type);
     }
 
   for (i = 0; i < resolved_type->num_fields (); ++i)
@@ -5556,9 +5535,7 @@ copy_type_recursive (struct type *type, htab_t copied_types)
       int i, nfields;
 
       nfields = type->num_fields ();
-      new_type->set_fields
-	((struct field *)
-	 TYPE_ZALLOC (new_type, nfields * sizeof (struct field)));
+      new_type->alloc_fields (type->num_fields ());
 
       for (i = 0; i < nfields; i++)
 	{
@@ -5703,10 +5680,9 @@ arch_flags_type (struct gdbarch *gdbarch, const char *name, int bit)
 
   type = type_allocator (gdbarch).new_type (TYPE_CODE_FLAGS, bit, name);
   type->set_is_unsigned (true);
-  type->set_num_fields (0);
   /* Pre-allocate enough space assuming every field is one bit.  */
-  type->set_fields
-    ((struct field *) TYPE_ZALLOC (type, bit * sizeof (struct field)));
+  type->alloc_fields (bit);
+  type->set_num_fields (0);
 
   return type;
 }
@@ -5912,6 +5888,46 @@ type::fixed_point_scaling_factor ()
   return type->fixed_point_info ().scaling_factor;
 }
 
+void type::alloc_fields (unsigned int nfields, bool init)
+{
+  this->set_num_fields (nfields);
+
+  if (nfields == 0)
+    {
+      this->main_type->flds_bnds.fields = nullptr;
+      return;
+    }
+
+  size_t size = nfields * sizeof (*this->fields ());
+  struct field *fields
+    = (struct field *) (init
+			? TYPE_ZALLOC (this, size)
+			: TYPE_ALLOC (this, size));
+
+  this->main_type->flds_bnds.fields = fields;
+}
+
+void type::copy_fields (struct type *src)
+{
+  unsigned int nfields = src->num_fields ();
+  alloc_fields (nfields, false);
+  if (nfields == 0)
+    return;
+
+  size_t size = nfields * sizeof (*this->fields ());
+  memcpy (this->fields (), src->fields (), size);
+}
+
+void type::copy_fields (std::vector<struct field> &src)
+{
+  unsigned int nfields = src.size ();
+  alloc_fields (nfields, false);
+  if (nfields == 0)
+    return;
+
+  size_t size = nfields * sizeof (*this->fields ());
+  memcpy (this->fields (), src.data (), size);
+}
 \f
 
 static const registry<gdbarch>::key<struct builtin_type> gdbtypes_data;
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index aedaf53cd5d..8f592dbe1c0 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -992,6 +992,14 @@ struct type
     this->main_type->flds_bnds.fields = fields;
   }
 
+  /* Allocate the fields array of this type, with NFIELDS elements.  If INIT,
+     zero-initialize the allocated memory.  */
+  void alloc_fields (unsigned int nfields, bool init = true);
+
+  /* Allocate the fields array of this type, and copy the fields from SRC.  */
+  void copy_fields (struct type *src);
+  void copy_fields (std::vector<struct field> &src);
+
   type *index_type () const
   {
     return this->field (0).type ();
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 2c7c83e67b1..73b69da21e5 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -113,7 +113,6 @@ static struct type *
 get_gdb_vtable_type (struct gdbarch *arch)
 {
   struct type *t;
-  struct field *field_list, *field;
   int offset;
 
   struct type *result = vtable_type_gdbarch_data.get (arch);
@@ -131,50 +130,56 @@ get_gdb_vtable_type (struct gdbarch *arch)
   struct type *ptrdiff_type
     = init_integer_type (alloc, gdbarch_ptr_bit (arch), 0, "ptrdiff_t");
 
+  t = alloc.new_type (TYPE_CODE_STRUCT, 0, nullptr);
+
   /* We assume no padding is necessary, since GDB doesn't know
      anything about alignment at the moment.  If this assumption bites
      us, we should add a gdbarch method which, given a type, returns
      the alignment that type requires, and then use that here.  */
 
   /* Build the field list.  */
-  field_list = XCNEWVEC (struct field, 4);
-  field = &field_list[0];
+  t->alloc_fields (4);
+
   offset = 0;
 
   /* ptrdiff_t vcall_and_vbase_offsets[0]; */
-  field->set_name ("vcall_and_vbase_offsets");
-  field->set_type (lookup_array_range_type (ptrdiff_type, 0, -1));
-  field->set_loc_bitpos (offset * TARGET_CHAR_BIT);
-  offset += field->type ()->length ();
-  field++;
+  {
+    struct field &field0 = t->field (0);
+    field0.set_name ("vcall_and_vbase_offsets");
+    field0.set_type (lookup_array_range_type (ptrdiff_type, 0, -1));
+    field0.set_loc_bitpos (offset * TARGET_CHAR_BIT);
+    offset += field0.type ()->length ();
+  }
 
   /* ptrdiff_t offset_to_top; */
-  field->set_name ("offset_to_top");
-  field->set_type (ptrdiff_type);
-  field->set_loc_bitpos (offset * TARGET_CHAR_BIT);
-  offset += field->type ()->length ();
-  field++;
+  {
+    struct field &field1 = t->field (1);
+    field1.set_name ("offset_to_top");
+    field1.set_type (ptrdiff_type);
+    field1.set_loc_bitpos (offset * TARGET_CHAR_BIT);
+    offset += field1.type ()->length ();
+  }
 
   /* void *type_info; */
-  field->set_name ("type_info");
-  field->set_type (void_ptr_type);
-  field->set_loc_bitpos (offset * TARGET_CHAR_BIT);
-  offset += field->type ()->length ();
-  field++;
+  {
+    struct field &field2 = t->field (2);
+    field2.set_name ("type_info");
+    field2.set_type (void_ptr_type);
+    field2.set_loc_bitpos (offset * TARGET_CHAR_BIT);
+    offset += field2.type ()->length ();
+  }
 
   /* void (*virtual_functions[0]) (); */
-  field->set_name ("virtual_functions");
-  field->set_type (lookup_array_range_type (ptr_to_void_fn_type, 0, -1));
-  field->set_loc_bitpos (offset * TARGET_CHAR_BIT);
-  offset += field->type ()->length ();
-  field++;
-
-  /* We assumed in the allocation above that there were four fields.  */
-  gdb_assert (field == (field_list + 4));
-
-  t = alloc.new_type (TYPE_CODE_STRUCT, offset * TARGET_CHAR_BIT, NULL);
-  t->set_num_fields (field - field_list);
-  t->set_fields (field_list);
+  {
+    struct field &field3 = t->field (3);
+    field3.set_name ("virtual_functions");
+    field3.set_type (lookup_array_range_type (ptr_to_void_fn_type, 0, -1));
+    field3.set_loc_bitpos (offset * TARGET_CHAR_BIT);
+    offset += field3.type ()->length ();
+  }
+
+  t->set_length (offset);
+
   t->set_name ("gdb_gnu_v3_abi_vtable");
   INIT_CPLUS_SPECIFIC (t);
 
@@ -1026,7 +1031,6 @@ static struct type *
 build_std_type_info_type (struct gdbarch *arch)
 {
   struct type *t;
-  struct field *field_list, *field;
   int offset;
   struct type *void_ptr_type
     = builtin_type (arch)->builtin_data_ptr;
@@ -1035,30 +1039,32 @@ build_std_type_info_type (struct gdbarch *arch)
   struct type *char_ptr_type
     = make_pointer_type (make_cv_type (1, 0, char_type, NULL), NULL);
 
-  field_list = XCNEWVEC (struct field, 2);
-  field = &field_list[0];
+  t = type_allocator (arch).new_type (TYPE_CODE_STRUCT, 0, nullptr);
+
+  t->alloc_fields (2);
+
   offset = 0;
 
   /* The vtable.  */
-  field->set_name ("_vptr.type_info");
-  field->set_type (void_ptr_type);
-  field->set_loc_bitpos (offset * TARGET_CHAR_BIT);
-  offset += field->type ()->length ();
-  field++;
+  {
+    struct field &field0 = t->field (0);
+    field0.set_name ("_vptr.type_info");
+    field0.set_type (void_ptr_type);
+    field0.set_loc_bitpos (offset * TARGET_CHAR_BIT);
+    offset += field0.type ()->length ();
+  }
 
   /* The name.  */
-  field->set_name ("__name");
-  field->set_type (char_ptr_type);
-  field->set_loc_bitpos (offset * TARGET_CHAR_BIT);
-  offset += field->type ()->length ();
-  field++;
-
-  gdb_assert (field == (field_list + 2));
-
-  t = type_allocator (arch).new_type (TYPE_CODE_STRUCT,
-				      offset * TARGET_CHAR_BIT, nullptr);
-  t->set_num_fields (field - field_list);
-  t->set_fields (field_list);
+  {
+    struct field &field1 = t->field (1);
+    field1.set_name ("__name");
+    field1.set_type (char_ptr_type);
+    field1.set_loc_bitpos (offset * TARGET_CHAR_BIT);
+    offset += field1.type ()->length ();
+  }
+
+  t->set_length (offset);
+
   t->set_name ("gdb_gnu_v3_type_info");
   INIT_CPLUS_SPECIFIC (t);
 
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 40ebe6ae9c5..688501e157a 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -1034,9 +1034,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 
 	t->set_code (type_code);
 	t->set_length (sh->value);
-	t->set_num_fields (nfields);
-	f = ((struct field *) TYPE_ALLOC (t, nfields * sizeof (struct field)));
-	t->set_fields (f);
+	t->alloc_fields (nfields, false);
 
 	if (type_code == TYPE_CODE_ENUM)
 	  {
@@ -1197,10 +1195,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 
 	      if (nparams > 0)
 		{
-		  ftype->set_num_fields (nparams);
-		  ftype->set_fields
-		    ((struct field *)
-		     TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
+		  ftype->alloc_fields (nparams, false);
 
 		  iparams = 0;
 		  for (struct symbol *sym : block_iterator_range (cblock))
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 0e2ca090ba8..cacfea86ab1 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -950,9 +950,7 @@ rust_composite_type (struct type *original,
   result->set_code (TYPE_CODE_STRUCT);
   result->set_name (name);
 
-  result->set_num_fields (nfields);
-  result->set_fields
-    ((struct field *) TYPE_ZALLOC (result, nfields * sizeof (struct field)));
+  result->alloc_fields (nfields);
 
   i = 0;
   bitpos = 0;
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index c3d87033f4f..c29d1c0d397 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -982,9 +982,7 @@ define_symbol (CORE_ADDR valu, const char *string, int desc, int type,
 	    }
 
 	  /* Allocate parameter information fields and fill them in.  */
-	  ftype->set_fields
-	    ((struct field *)
-	     TYPE_ALLOC (ftype, nsemi * sizeof (struct field)));
+	  ftype->alloc_fields (nsemi, false);
 	  while (*p++ == ';')
 	    {
 	      struct type *ptype;
@@ -1826,10 +1824,7 @@ read_type (const char **pp, struct objfile *objfile)
 	    && arg_types->type->code () == TYPE_CODE_VOID)
 	  num_args = 0;
 
-	func_type->set_fields
-	  ((struct field *) TYPE_ALLOC (func_type,
-					num_args * sizeof (struct field)));
-	memset (func_type->fields (), 0, num_args * sizeof (struct field));
+	func_type->alloc_fields (num_args);
 	{
 	  int i;
 	  struct type_list *t;
@@ -3295,11 +3290,7 @@ attach_fields_to_type (struct stab_field_info *fip, struct type *type,
      non-public fields.  Record the field count, allocate space for the
      array of fields, and create blank visibility bitfields if necessary.  */
 
-  type->set_num_fields (nfields);
-  type->set_fields
-    ((struct field *)
-     TYPE_ALLOC (type, sizeof (struct field) * nfields));
-  memset (type->fields (), 0, sizeof (struct field) * nfields);
+  type->alloc_fields (nfields);
 
   if (non_public_fields)
     {
@@ -3644,11 +3635,7 @@ read_enum_type (const char **pp, struct type *type,
   type->set_is_stub (false);
   if (unsigned_enum)
     type->set_is_unsigned (true);
-  type->set_num_fields (nsyms);
-  type->set_fields
-    ((struct field *)
-     TYPE_ALLOC (type, sizeof (struct field) * nsyms));
-  memset (type->fields (), 0, sizeof (struct field) * nsyms);
+  type->alloc_fields (nsyms);
 
   /* Find the symbols for the values and put them into the type.
      The symbols can be found in the symlist that we put them on
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index bc1927049d8..0414aea6f1a 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -745,9 +745,7 @@ create_enum (struct gdbarch *gdbarch, int bit, const char *name,
   int i;
 
   type = type_allocator (gdbarch).new_type (TYPE_CODE_ENUM, bit, name);
-  type->set_num_fields (count);
-  type->set_fields
-    ((struct field *) TYPE_ZALLOC (type, sizeof (struct field) * count));
+  type->alloc_fields (count);
   type->set_is_unsigned (true);
 
   for (i = 0; i < count; i++)
-- 
2.35.3

[PATCH 3/6] [gdb/symtab] Do more zero-initialization of type::fields

[Tom de Vries]

Now that we've introduced type::{alloc_fields,copy_fields}, the places where
no zero-initialization of allocated fields is done are easy to spot:
...
$ find gdb* -type f | grep -v ChangeLog | xargs grep alloc_fields | grep false
gdb/coffread.c:  type->alloc_fields (nfields, false);
gdb/coffread.c:  type->alloc_fields (nsyms, false);
gdb/stabsread.c:	  ftype->alloc_fields (nsemi, false);
gdb/gdbtypes.c:  resolved_type->alloc_fields (nfields, false);
gdb/gdbtypes.c:  alloc_fields (nfields, false);
gdb/gdbtypes.c:  alloc_fields (nfields, false);
gdb/mdebugread.c:	t->alloc_fields (nfields, false);
gdb/mdebugread.c:		  ftype->alloc_fields (nparams, false);
...

All hits in gdbtypes.c are ok.  There are two hits in the two variants of
copy_fields, and there's already a comment for the third.

AFAICT, the other ones are not, so fix those by dropping the "false" argument.

Tested on x86_64-linux.
---
 gdb/coffread.c   | 4 ++--
 gdb/mdebugread.c | 4 ++--
 gdb/stabsread.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index e132dd1a1f7..e251f110ff8 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -2032,7 +2032,7 @@ coff_read_struct_type (int index, int length, int lastsym,
     }
   /* Now create the vector of fields, and record how big it is.  */
 
-  type->alloc_fields (nfields, false);
+  type->alloc_fields (nfields);
 
   /* Copy the saved-up fields into the field vector.  */
 
@@ -2110,7 +2110,7 @@ coff_read_enum_type (int index, int length, int lastsym,
   else /* Assume ints.  */
     type->set_length (gdbarch_int_bit (gdbarch) / TARGET_CHAR_BIT);
   type->set_code (TYPE_CODE_ENUM);
-  type->alloc_fields (nsyms, false);
+  type->alloc_fields (nsyms);
 
   /* Find the symbols for the values and put them into the type.
      The symbols can be found in the symlist that we put them on
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 688501e157a..ad9967b75fa 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -1034,7 +1034,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 
 	t->set_code (type_code);
 	t->set_length (sh->value);
-	t->alloc_fields (nfields, false);
+	t->alloc_fields (nfields);
 
 	if (type_code == TYPE_CODE_ENUM)
 	  {
@@ -1195,7 +1195,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 
 	      if (nparams > 0)
 		{
-		  ftype->alloc_fields (nparams, false);
+		  ftype->alloc_fields (nparams);
 
 		  iparams = 0;
 		  for (struct symbol *sym : block_iterator_range (cblock))
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index c29d1c0d397..ad9258a9f20 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -982,7 +982,7 @@ define_symbol (CORE_ADDR valu, const char *string, int desc, int type,
 	    }
 
 	  /* Allocate parameter information fields and fill them in.  */
-	  ftype->alloc_fields (nsemi, false);
+	  ftype->alloc_fields (nsemi);
 	  while (*p++ == ';')
 	    {
 	      struct type *ptype;
-- 
2.35.3

[PATCH 4/6] [gdb/symtab] Replace TYPE_ALLOC + memset with TYPE_ZALLOC

[Tom de Vries]

I noticed a case or TYPE_ALLOC followed by memset.

Replace this with TYPE_ZALLOC.

Tested on x86_64-linux.
---
 gdb/stabsread.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index ad9258a9f20..18fefd6929c 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -2693,9 +2693,7 @@ read_member_functions (struct stab_field_info *fip, const char **pp,
     {
       ALLOCATE_CPLUS_STRUCT_TYPE (type);
       TYPE_FN_FIELDLISTS (type) = (struct fn_fieldlist *)
-	TYPE_ALLOC (type, sizeof (struct fn_fieldlist) * nfn_fields);
-      memset (TYPE_FN_FIELDLISTS (type), 0,
-	      sizeof (struct fn_fieldlist) * nfn_fields);
+	TYPE_ZALLOC (type, sizeof (struct fn_fieldlist) * nfn_fields);
       TYPE_NFN_FIELDS (type) = nfn_fields;
     }
 
-- 
2.35.3

[PATCH 5/6] [gdb/symtab] Replace TYPE_ALLOC + B_CLRALL with TYPE_ZALLOC

[Tom de Vries]

I noticed some cases of TYPE_ALLOC followed by B_CLRALL.

Replace these with TYPE_ZALLOC.

Tested on x86_64-linux.
---
 gdb/dwarf2/read.c | 12 ++++--------
 gdb/stabsread.c   | 15 +++++----------
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 7fe0e84cdda..bc68c290289 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -12064,16 +12064,13 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
       ALLOCATE_CPLUS_STRUCT_TYPE (type);
 
       TYPE_FIELD_PRIVATE_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_PRIVATE_BITS (type), nfields);
+	(B_TYPE *) TYPE_ZALLOC (type, B_BYTES (nfields));
 
       TYPE_FIELD_PROTECTED_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_PROTECTED_BITS (type), nfields);
+	(B_TYPE *) TYPE_ZALLOC (type, B_BYTES (nfields));
 
       TYPE_FIELD_IGNORE_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_IGNORE_BITS (type), nfields);
+	(B_TYPE *) TYPE_ZALLOC (type, B_BYTES (nfields));
     }
 
   /* If the type has baseclasses, allocate and clear a bit vector for
@@ -12084,9 +12081,8 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
       unsigned char *pointer;
 
       ALLOCATE_CPLUS_STRUCT_TYPE (type);
-      pointer = (unsigned char *) TYPE_ALLOC (type, num_bytes);
+      pointer = (unsigned char *) TYPE_ZALLOC (type, num_bytes);
       TYPE_FIELD_VIRTUAL_BITS (type) = pointer;
-      B_CLRALL (TYPE_FIELD_VIRTUAL_BITS (type), fip->baseclasses.size ());
       TYPE_N_BASECLASSES (type) = fip->baseclasses.size ();
     }
 
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index 18fefd6929c..1269fc02d72 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -3067,19 +3067,17 @@ read_baseclasses (struct stab_field_info *fip, const char **pp,
   /* Some stupid compilers have trouble with the following, so break
      it up into simpler expressions.  */
   TYPE_FIELD_VIRTUAL_BITS (type) = (B_TYPE *)
-    TYPE_ALLOC (type, B_BYTES (TYPE_N_BASECLASSES (type)));
+    TYPE_ZALLOC (type, B_BYTES (TYPE_N_BASECLASSES (type)));
 #else
   {
     int num_bytes = B_BYTES (TYPE_N_BASECLASSES (type));
     char *pointer;
 
-    pointer = (char *) TYPE_ALLOC (type, num_bytes);
+    pointer = (char *) TYPE_ZALLOC (type, num_bytes);
     TYPE_FIELD_VIRTUAL_BITS (type) = (B_TYPE *) pointer;
   }
 #endif /* 0 */
 
-  B_CLRALL (TYPE_FIELD_VIRTUAL_BITS (type), TYPE_N_BASECLASSES (type));
-
   for (i = 0; i < TYPE_N_BASECLASSES (type); i++)
     {
       newobj = OBSTACK_ZALLOC (&fip->obstack, struct stabs_nextfield);
@@ -3295,16 +3293,13 @@ attach_fields_to_type (struct stab_field_info *fip, struct type *type,
       ALLOCATE_CPLUS_STRUCT_TYPE (type);
 
       TYPE_FIELD_PRIVATE_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_PRIVATE_BITS (type), nfields);
+	(B_TYPE *) TYPE_ZALLOC (type, B_BYTES (nfields));
 
       TYPE_FIELD_PROTECTED_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_PROTECTED_BITS (type), nfields);
+	(B_TYPE *) TYPE_ZALLOC (type, B_BYTES (nfields));
 
       TYPE_FIELD_IGNORE_BITS (type) =
-	(B_TYPE *) TYPE_ALLOC (type, B_BYTES (nfields));
-      B_CLRALL (TYPE_FIELD_IGNORE_BITS (type), nfields);
+	(B_TYPE *) TYPE_ZALLOC (type, B_BYTES (nfields));
     }
 
   /* Copy the saved-up fields into the field vector.  Start from the
-- 
2.35.3

[PATCH 6/6] [gdb/symtab] Replace TYPE_ALLOC with TYPE_ZALLOC where required

[Tom de Vries]

Handle the remaining uses of TYPE_ALLOC, either by:
- replacing with TYPE_ZALLOC, or
- adding a comment explaining why zero-initialization is not necessary.

Tested on x86_64-linux.
---
 gdb/dwarf2/read.c | 10 ++++++++--
 gdb/gdbtypes.c    |  8 +++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index bc68c290289..527e0770502 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -12376,8 +12376,8 @@ dwarf2_attach_fn_fields_to_type (struct field_info *fip, struct type *type,
 
   ALLOCATE_CPLUS_STRUCT_TYPE (type);
   TYPE_FN_FIELDLISTS (type) = (struct fn_fieldlist *)
-    TYPE_ALLOC (type,
-		sizeof (struct fn_fieldlist) * fip->fnfieldlists.size ());
+    TYPE_ZALLOC (type,
+		 sizeof (struct fn_fieldlist) * fip->fnfieldlists.size ());
 
   for (int i = 0; i < fip->fnfieldlists.size (); i++)
     {
@@ -12386,6 +12386,8 @@ dwarf2_attach_fn_fields_to_type (struct field_info *fip, struct type *type,
 
       TYPE_FN_FIELDLIST_NAME (type, i) = nf.name;
       TYPE_FN_FIELDLIST_LENGTH (type, i) = nf.fnfields.size ();
+      /* No need to zero-initialize, initialization is done by the copy in
+	 the loop below.  */
       fn_flp->fn_fields = (struct fn_field *)
 	TYPE_ALLOC (type, sizeof (struct fn_field) * nf.fnfields.size ());
 
@@ -13088,6 +13090,8 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
 	  int count = fi.typedef_field_list.size ();
 
 	  ALLOCATE_CPLUS_STRUCT_TYPE (type);
+	  /* No zero-initialization is need, the elements are initialized by
+	     the copy in the loop below.  */
 	  TYPE_TYPEDEF_FIELD_ARRAY (type)
 	    = ((struct decl_field *)
 	       TYPE_ALLOC (type,
@@ -13106,6 +13110,8 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
 	  int count = fi.nested_types_list.size ();
 
 	  ALLOCATE_CPLUS_STRUCT_TYPE (type);
+	  /* No zero-initialization is need, the elements are initialized by
+	     the copy in the loop below.  */
 	  TYPE_NESTED_TYPES_ARRAY (type)
 	    = ((struct decl_field *)
 	       TYPE_ALLOC (type, sizeof (struct decl_field) * count));
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 766107a7013..fd67c4bafdb 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3207,7 +3207,7 @@ check_stub_method (struct type *type, int method_id, int signature_id)
   /* We need one extra slot, for the THIS pointer.  */
 
   argtypes = (struct field *)
-    TYPE_ALLOC (type, (argcount + 1) * sizeof (struct field));
+    TYPE_ZALLOC (type, (argcount + 1) * sizeof (struct field));
   p = argtypetext;
 
   /* Add THIS pointer for non-static methods.  */
@@ -3297,7 +3297,7 @@ allocate_cplus_struct_type (struct type *type)
 
   TYPE_SPECIFIC_FIELD (type) = TYPE_SPECIFIC_CPLUS_STUFF;
   TYPE_RAW_CPLUS_SPECIFIC (type) = (struct cplus_struct_type *)
-    TYPE_ALLOC (type, sizeof (struct cplus_struct_type));
+    TYPE_ZALLOC (type, sizeof (struct cplus_struct_type));
   *(TYPE_RAW_CPLUS_SPECIFIC (type)) = cplus_struct_default;
   set_type_vptr_fieldno (type, -1);
 }
@@ -3314,7 +3314,7 @@ allocate_gnat_aux_type (struct type *type)
 {
   TYPE_SPECIFIC_FIELD (type) = TYPE_SPECIFIC_GNAT_STUFF;
   TYPE_GNAT_SPECIFIC (type) = (struct gnat_aux_type *)
-    TYPE_ALLOC (type, sizeof (struct gnat_aux_type));
+    TYPE_ZALLOC (type, sizeof (struct gnat_aux_type));
   *(TYPE_GNAT_SPECIFIC (type)) = gnat_aux_default;
 }
 
@@ -3454,6 +3454,8 @@ init_complex_type (const char *name, struct type *target_type)
     {
       if (name == nullptr && target_type->name () != nullptr)
 	{
+	  /* No zero-initialization required, initialized by strcpy/strcat
+	     below.  */
 	  char *new_name
 	    = (char *) TYPE_ALLOC (target_type,
 				   strlen (target_type->name ())
-- 
2.35.3

Re: [PATCH 1/6] [gdb/symtab] Fix uninitialized memory in buildsym_compunit::finish_block_internal

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> When running test-case gdb.dwarf2/per-bfd-sharing.exp with target board stabs,
Tom> gdb either segfaults or asserts due to reading uninitialized memory, allocated
Tom> here in buildsym_compunit::finish_block_internal:
Tom> ...
Tom> ftype-> set_fields
Tom> 		((struct field *)
Tom> 		 TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
Tom> ...

Tom> Fix this by using TYPE_ZALLOC instead.

At first I was curious about the history of what changed here, but now I
see that dwarf2_attach_fields_to_type is doing the zalloc thing, so I
think it's just the usual thing where nobody uses or tests non-DWARF
stuff (and why would they) so it wasn't caught in some earlier change.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/6] [gdb/symtab] Factor out type::{alloc_fields,copy_fields}

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> After finding this code in buildsym_compunit::finish_block_internal:
Tom> ...
ftype-> set_fields
Tom>                 ((struct field *)
Tom>                  TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
Tom> ...
Tom> and fixing PR30810 by using TYPE_ZALLOC, I wondered if there were more
Tom> locations that needed fixing.

Tom> I decided to make things easier to spot by factoring out a new function
Tom> alloc_fields:

Thanks.  Seems like a good idea to me.

Tom> +void type::alloc_fields (unsigned int nfields, bool init)

gdb style is a newline after the return type.
There's a few cases of this.

Tom

Re: [PATCH 3/6] [gdb/symtab] Do more zero-initialization of type::fields

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Now that we've introduced type::{alloc_fields,copy_fields}, the places where
Tom> no zero-initialization of allocated fields is done are easy to spot:
Tom> ...
Tom> $ find gdb* -type f | grep -v ChangeLog | xargs grep alloc_fields | grep false
Tom> gdb/coffread.c:  type->alloc_fields (nfields, false);
Tom> gdb/coffread.c:  type->alloc_fields (nsyms, false);
Tom> gdb/stabsread.c:	  ftype->alloc_fields (nsemi, false);
Tom> gdb/gdbtypes.c:  resolved_type->alloc_fields (nfields, false);
Tom> gdb/gdbtypes.c:  alloc_fields (nfields, false);
Tom> gdb/gdbtypes.c:  alloc_fields (nfields, false);
Tom> gdb/mdebugread.c:	t->alloc_fields (nfields, false);
Tom> gdb/mdebugread.c:		  ftype->alloc_fields (nparams, false);
Tom> ...

Tom> All hits in gdbtypes.c are ok.  There are two hits in the two variants of
Tom> copy_fields, and there's already a comment for the third.

Tom> AFAICT, the other ones are not, so fix those by dropping the "false" argument.

Looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Honestly it probably wouldn't hurt to just always zero-init these.
If type allocation is ever a performance problem, then I suspect
something's gone pretty wrong.

Tom

Re: [PATCH 4/6] [gdb/symtab] Replace TYPE_ALLOC + memset with TYPE_ZALLOC

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> I noticed a case or TYPE_ALLOC followed by memset.
Tom> Replace this with TYPE_ZALLOC.

Tom> Tested on x86_64-linux.

Obvious IMO.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 5/6] [gdb/symtab] Replace TYPE_ALLOC + B_CLRALL with TYPE_ZALLOC

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> I noticed some cases of TYPE_ALLOC followed by B_CLRALL.
Tom> Replace these with TYPE_ZALLOC.

Looks good.
I wonder if B_CLRALL is still used anywhere after this.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 6/6] [gdb/symtab] Replace TYPE_ALLOC with TYPE_ZALLOC where required

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Handle the remaining uses of TYPE_ALLOC, either by:
Tom> - replacing with TYPE_ZALLOC, or
Tom> - adding a comment explaining why zero-initialization is not necessary.

Seems fine.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/6] [gdb/symtab] Factor out type::{alloc_fields,copy_fields}

[Tom de Vries]

On 8/30/23 22:17, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> After finding this code in buildsym_compunit::finish_block_internal:
> Tom> ...
> ftype-> set_fields
> Tom>                 ((struct field *)
> Tom>                  TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
> Tom> ...
> Tom> and fixing PR30810 by using TYPE_ZALLOC, I wondered if there were more
> Tom> locations that needed fixing.
> 
> Tom> I decided to make things easier to spot by factoring out a new function
> Tom> alloc_fields:
> 
> Thanks.  Seems like a good idea to me.
> 
> Tom> +void type::alloc_fields (unsigned int nfields, bool init)
> 
> gdb style is a newline after the return type.
> There's a few cases of this.

Ack, fixed and pushed the series.

Thanks for the reviews.

- Tom

Re: [PATCH 5/6] [gdb/symtab] Replace TYPE_ALLOC + B_CLRALL with TYPE_ZALLOC

[Tom de Vries]

On 8/30/23 22:21, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> I noticed some cases of TYPE_ALLOC followed by B_CLRALL.
> Tom> Replace these with TYPE_ZALLOC.
> 
> Looks good.
> I wonder if B_CLRALL is still used anywhere after this.

I guess not:
...
$ find gdb* -type f | egrep -v ChangeLog | xargs grep B_CLR
gdb/gdbtypes.h:#define B_CLR(a,x)	((a)[(x)>>3] &= ~(1 << ((x)&7)))
gdb/gdbtypes.h:#define	B_CLRALL(a,x)	memset ((a), 0, B_BYTES(x))
...

Thanks,
- Tom