[PATCH v2 2/2] ld: pe: Apply review suggestions on the existing exports/imports arrays

["Martin Storsjö" <martin@martin.st>]

Use a separate explicit max_exports/imports field, instead of
deducing it from the number of allocated elements. Use a named
constant for the incremental growth of the array.

Use bool instead of int for boolean values.

Remove an unnecessary if statement/scope in the def_file_free
function.

Add more verbose comments about parameters, and about insertion
into an array of structs.

Generally use unsigned integers for all array indices and sizes.
The num_exports/imports fields are kept as is as signed integers,
since changing them to unsigned would require a disproportionate
amount of changes ti pe-dll.c to avoid comparisons between signed
and unsigned.

Simply use xrealloc instead of a check and xmalloc/xrealloc;
xrealloc can take NULL as the first parameter (and does a similar
check internally). (This wasn't requested in review though,
but noticed while working on the code.)
---
 ld/deffile.h  |   6 +-
 ld/deffilep.y | 157 ++++++++++++++++++++++++--------------------------
 ld/pe-dll.c   |   6 +-
 3 files changed, 83 insertions(+), 86 deletions(-)

diff --git a/ld/deffile.h b/ld/deffile.h
index 89ffe9f972e..9eb08ad83fb 100644
--- a/ld/deffile.h
+++ b/ld/deffile.h
@@ -84,6 +84,7 @@ typedef struct def_file {
 
   /* From the EXPORTS commands.  */
   int num_exports;
+  unsigned int max_exports;
   def_file_export *exports;
 
   /* Used by imports for module names.  */
@@ -91,6 +92,7 @@ typedef struct def_file {
 
   /* From the IMPORTS commands.  */
   int num_imports;
+  unsigned int max_imports;
   def_file_import *imports;
 
   /* From the VERSION command, -1 if not specified.  */
@@ -112,10 +114,10 @@ extern def_file *def_file_parse (const char *, def_file *);
 extern void def_file_free (def_file *);
 extern def_file_export *def_file_add_export (def_file *, const char *,
 					     const char *, int,
-					     const char *, int *);
+					     const char *, bool *);
 extern def_file_import *def_file_add_import (def_file *, const char *,
 					     const char *, int, const char *,
-					     const char *, int *);
+					     const char *, bool *);
 extern int def_file_add_import_from (def_file *fdef,
 				     int num_imports,
 				     const char *name,
diff --git a/ld/deffilep.y b/ld/deffilep.y
index ed8f0d6719a..d7052a63beb 100644
--- a/ld/deffilep.y
+++ b/ld/deffilep.y
@@ -459,29 +459,23 @@ def_file_free (def_file *fdef)
       free (fdef->section_defs);
     }
 
-  if (fdef->exports)
+  for (i = 0; i < fdef->num_exports; i++)
     {
-      for (i = 0; i < fdef->num_exports; i++)
-	{
-	  if (fdef->exports[i].internal_name != fdef->exports[i].name)
-	    free (fdef->exports[i].internal_name);
-	  free (fdef->exports[i].name);
-	  free (fdef->exports[i].its_name);
-	}
-      free (fdef->exports);
+      if (fdef->exports[i].internal_name != fdef->exports[i].name)
+        free (fdef->exports[i].internal_name);
+      free (fdef->exports[i].name);
+      free (fdef->exports[i].its_name);
     }
+  free (fdef->exports);
 
-  if (fdef->imports)
+  for (i = 0; i < fdef->num_imports; i++)
     {
-      for (i = 0; i < fdef->num_imports; i++)
-	{
-	  if (fdef->imports[i].internal_name != fdef->imports[i].name)
-	    free (fdef->imports[i].internal_name);
-	  free (fdef->imports[i].name);
-	  free (fdef->imports[i].its_name);
-	}
-      free (fdef->imports);
+      if (fdef->imports[i].internal_name != fdef->imports[i].name)
+        free (fdef->imports[i].internal_name);
+      free (fdef->imports[i].name);
+      free (fdef->imports[i].its_name);
     }
+  free (fdef->imports);
 
   while (fdef->modules)
     {
@@ -627,22 +621,25 @@ cmp_export_elem (const def_file_export *e, const char *ex_name,
 
 /* Search the position of the identical element, or returns the position
    of the next higher element. If last valid element is smaller, then MAX
-   is returned.  */
+   is returned. The max parameter indicates the number of elements in the
+   array. On return, *is_ident indicates whether the returned array index
+   points at an element which is identical to the one searched for.  */
 
-static int
-find_export_in_list (def_file_export *b, int max,
+static unsigned int
+find_export_in_list (def_file_export *b, unsigned int max,
 		     const char *ex_name, const char *in_name,
-		     const char *its_name, int ord, int *is_ident)
+		     const char *its_name, int ord, bool *is_ident)
 {
-  int e, l, r, p;
+  int e;
+  unsigned int l, r, p;
 
-  *is_ident = 0;
+  *is_ident = false;
   if (!max)
     return 0;
   if ((e = cmp_export_elem (b, ex_name, in_name, its_name, ord)) <= 0)
     {
       if (!e)
-	*is_ident = 1;
+	*is_ident = true;
       return 0;
     }
   if (max == 1)
@@ -652,7 +649,7 @@ find_export_in_list (def_file_export *b, int max,
   else if (!e || max == 2)
     {
       if (!e)
-	*is_ident = 1;
+	*is_ident = true;
       return max - 1;
     }
   l = 0; r = max - 1;
@@ -662,7 +659,7 @@ find_export_in_list (def_file_export *b, int max,
       e = cmp_export_elem (b + p, ex_name, in_name, its_name, ord);
       if (!e)
 	{
-	  *is_ident = 1;
+	  *is_ident = true;
 	  return p;
 	}
       else if (e < 0)
@@ -673,7 +670,7 @@ find_export_in_list (def_file_export *b, int max,
   if ((e = cmp_export_elem (b + l, ex_name, in_name, its_name, ord)) > 0)
     ++l;
   else if (!e)
-    *is_ident = 1;
+    *is_ident = true;
   return l;
 }
 
@@ -683,11 +680,10 @@ def_file_add_export (def_file *fdef,
 		     const char *internal_name,
 		     int ordinal,
 		     const char *its_name,
-		     int *is_dup)
+		     bool *is_dup)
 {
   def_file_export *e;
-  int pos;
-  int max_exports = ROUND_UP(fdef->num_exports, 32);
+  unsigned int pos;
 
   if (internal_name && !external_name)
     external_name = internal_name;
@@ -695,27 +691,27 @@ def_file_add_export (def_file *fdef,
     internal_name = external_name;
 
   /* We need to avoid duplicates.  */
-  *is_dup = 0;
+  *is_dup = false;
   pos = find_export_in_list (fdef->exports, fdef->num_exports,
 		     external_name, internal_name,
 		     its_name, ordinal, is_dup);
 
-  if (*is_dup != 0)
+  if (*is_dup)
     return (fdef->exports + pos);
 
-  if (fdef->num_exports >= max_exports)
+  if ((unsigned)fdef->num_exports >= fdef->max_exports)
     {
-      max_exports = ROUND_UP(fdef->num_exports + 1, 32);
-      if (fdef->exports)
-	fdef->exports = xrealloc (fdef->exports,
-				 max_exports * sizeof (def_file_export));
-      else
-	fdef->exports = xmalloc (max_exports * sizeof (def_file_export));
+      fdef->max_exports += SYMBOL_LIST_ARRAY_GROW;
+      fdef->exports = xrealloc (fdef->exports,
+				fdef->max_exports * sizeof (def_file_export));
     }
 
   e = fdef->exports + pos;
-  if (pos != fdef->num_exports)
+  /* If we're inserting in the middle of the array, we need to move the
+     following elements forward.  */
+  if (pos != (unsigned)fdef->num_exports)
     memmove (&e[1], e, (sizeof (def_file_export) * (fdef->num_exports - pos)));
+  /* Wipe the element for use as a new entry.  */
   memset (e, 0, sizeof (def_file_export));
   e->name = xstrdup (external_name);
   e->internal_name = xstrdup (internal_name);
@@ -772,22 +768,25 @@ cmp_import_elem (const def_file_import *e, const char *ex_name,
 
 /* Search the position of the identical element, or returns the position
    of the next higher element. If last valid element is smaller, then MAX
-   is returned.  */
+   is returned. The max parameter indicates the number of elements in the
+   array. On return, *is_ident indicates whether the returned array index
+   points at an element which is identical to the one searched for.  */
 
-static int
-find_import_in_list (def_file_import *b, int max,
+static unsigned int
+find_import_in_list (def_file_import *b, unsigned int max,
 		     const char *ex_name, const char *in_name,
-		     const char *module, int ord, int *is_ident)
+		     const char *module, int ord, bool *is_ident)
 {
-  int e, l, r, p;
+  int e;
+  unsigned int l, r, p;
 
-  *is_ident = 0;
+  *is_ident = false;
   if (!max)
     return 0;
   if ((e = cmp_import_elem (b, ex_name, in_name, module, ord)) <= 0)
     {
       if (!e)
-	*is_ident = 1;
+	*is_ident = true;
       return 0;
     }
   if (max == 1)
@@ -797,7 +796,7 @@ find_import_in_list (def_file_import *b, int max,
   else if (!e || max == 2)
     {
       if (!e)
-	*is_ident = 1;
+	*is_ident = true;
       return max - 1;
     }
   l = 0; r = max - 1;
@@ -807,7 +806,7 @@ find_import_in_list (def_file_import *b, int max,
       e = cmp_import_elem (b + p, ex_name, in_name, module, ord);
       if (!e)
 	{
-	  *is_ident = 1;
+	  *is_ident = true;
 	  return p;
 	}
       else if (e < 0)
@@ -818,7 +817,7 @@ find_import_in_list (def_file_import *b, int max,
   if ((e = cmp_import_elem (b + l, ex_name, in_name, module, ord)) > 0)
     ++l;
   else if (!e)
-    *is_ident = 1;
+    *is_ident = true;
   return l;
 }
 
@@ -849,33 +848,30 @@ def_file_add_import (def_file *fdef,
 		     int ordinal,
 		     const char *internal_name,
 		     const char *its_name,
-		     int *is_dup)
+		     bool *is_dup)
 {
   def_file_import *i;
-  int pos;
-  int max_imports = ROUND_UP (fdef->num_imports, 16);
+  unsigned int pos;
 
   /* We need to avoid here duplicates.  */
-  *is_dup = 0;
+  *is_dup = false;
   pos = find_import_in_list (fdef->imports, fdef->num_imports,
 			     name,
 			     (!internal_name ? name : internal_name),
 			     module, ordinal, is_dup);
-  if (*is_dup != 0)
+  if (*is_dup)
     return fdef->imports + pos;
 
-  if (fdef->num_imports >= max_imports)
+  if ((unsigned)fdef->num_imports >= fdef->max_imports)
     {
-      max_imports = ROUND_UP (fdef->num_imports+1, 16);
-
-      if (fdef->imports)
-	fdef->imports = xrealloc (fdef->imports,
-				 max_imports * sizeof (def_file_import));
-      else
-	fdef->imports = xmalloc (max_imports * sizeof (def_file_import));
+      fdef->max_imports += SYMBOL_LIST_ARRAY_GROW;
+      fdef->imports = xrealloc (fdef->imports,
+				fdef->max_imports * sizeof (def_file_import));
     }
   i = fdef->imports + pos;
-  if (pos != fdef->num_imports)
+  /* If we're inserting in the middle of the array, we need to move the
+     following elements forward.  */
+  if (pos != (unsigned)fdef->num_imports)
     memmove (i + 1, i, sizeof (def_file_import) * (fdef->num_imports - pos));
 
   fill_in_import (i, name, def_stash_module (fdef, module), ordinal,
@@ -895,36 +891,35 @@ def_file_add_import_from (def_file *fdef,
 			  const char *its_name ATTRIBUTE_UNUSED)
 {
   def_file_import *i;
-  int is_dup;
-  int pos;
-  int max_imports = ROUND_UP (fdef->num_imports, 16);
+  bool is_dup;
+  unsigned int pos;
 
   /* We need to avoid here duplicates.  */
-  is_dup = 0;
+  is_dup = false;
   pos = find_import_in_list (fdef->imports, fdef->num_imports,
 			     name, internal_name ? internal_name : name,
 			     module, ordinal, &is_dup);
-  if (is_dup != 0)
+  if (is_dup)
     return -1;
-  if (fdef->imports && pos != fdef->num_imports)
+  if (fdef->imports && pos != (unsigned)fdef->num_imports)
     {
       i = fdef->imports + pos;
       if (i->module && strcmp (i->module->name, module) == 0)
 	return -1;
     }
 
-  if (fdef->num_imports + num_imports - 1 >= max_imports)
+  if ((unsigned)fdef->num_imports + num_imports - 1 >= fdef->max_imports)
     {
-      max_imports = ROUND_UP (fdef->num_imports + num_imports, 16);
+      fdef->max_imports = fdef->num_imports + num_imports +
+			  SYMBOL_LIST_ARRAY_GROW;
 
-      if (fdef->imports)
-	fdef->imports = xrealloc (fdef->imports,
-				 max_imports * sizeof (def_file_import));
-      else
-	fdef->imports = xmalloc (max_imports * sizeof (def_file_import));
+      fdef->imports = xrealloc (fdef->imports,
+				fdef->max_imports * sizeof (def_file_import));
     }
   i = fdef->imports + pos;
-  if (pos != fdef->num_imports)
+  /* If we're inserting in the middle of the array, we need to move the
+     following elements forward.  */
+  if (pos != (unsigned)fdef->num_imports)
     memmove (i + num_imports, i,
 	     sizeof (def_file_import) * (fdef->num_imports - pos));
 
@@ -1261,7 +1256,7 @@ def_exports (const char *external_name,
 	     const char *its_name)
 {
   def_file_export *dfe;
-  int is_dup = 0;
+  bool is_dup = false;
 
   if (!internal_name && external_name)
     internal_name = external_name;
@@ -1297,7 +1292,7 @@ def_import (const char *internal_name,
 {
   char *buf = 0;
   const char *ext = dllext ? dllext : "dll";
-  int is_dup = 0;
+  bool is_dup = false;
 
   buf = xmalloc (strlen (module) + strlen (ext) + 2);
   sprintf (buf, "%s.%s", module, ext);
diff --git a/ld/pe-dll.c b/ld/pe-dll.c
index 60584a88571..92c33f528c8 100644
--- a/ld/pe-dll.c
+++ b/ld/pe-dll.c
@@ -791,7 +791,7 @@ process_def_file_and_drectve (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_link_info *
 
 		  if (auto_export (b, pe_def_file, sn))
 		    {
-		      int is_dup = 0;
+		      bool is_dup = false;
 		      def_file_export *p;
 
 		      p = def_file_add_export (pe_def_file, sn, 0, -1,
@@ -857,7 +857,7 @@ process_def_file_and_drectve (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_link_info *
 
 	  if (strchr (pe_def_file->exports[i].name, '@'))
 	    {
-	      int is_dup = 1;
+	      bool is_dup = true;
 	      int lead_at = (*pe_def_file->exports[i].name == '@');
 	      char *tmp = xstrdup (pe_def_file->exports[i].name + lead_at);
 
@@ -3579,7 +3579,7 @@ pe_implied_import_dll (const char *filename)
 	 exported in buggy auto-import releases.  */
       if (! startswith (erva + name_rva, "__nm_"))
 	{
-	  int is_dup = 0;
+	  bool is_dup = false;
 	  /* is_data is true if the address is in the data, rdata or bss
 	     segment.  */
 	  is_data =
-- 
2.25.1