[PATCH/committed] sim: options: increase max option count

[PATCH/committed] sim: options: increase max option count

[Mike Frysinger]

As we turn on more modules by default for all ports, the number of
options has been increasing.  The sim-options module has a limit on
the number of options it can support, and if it's exceeded, it likes
to go into an infinite loop.  Increase the ceiling and add an assert
so we abort right away instead of hanging.

This will be needed to turn on hw support for v850 as it will then
exceed the current limit.
---
 sim/common/ChangeLog     | 5 +++++
 sim/common/sim-options.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
index 61149e21213f..ed4d9f7da65b 100644
--- a/sim/common/ChangeLog
+++ b/sim/common/ChangeLog
@@ -1,3 +1,8 @@
+2021-04-24  Mike Frysinger  <vapier@gentoo.org>
+
+	* sim-options.c (ARG_HASH_SIZE): Increase to 256.
+	(sim_parse_args): Call SIM_ASSERT.
+
 2021-04-22  Tom Tromey  <tom@tromey.com>
 
 	* sim-utils.c: Update includes.
diff --git a/sim/common/sim-options.c b/sim/common/sim-options.c
index d4434cfd7385..ab95984e833f 100644
--- a/sim/common/sim-options.c
+++ b/sim/common/sim-options.c
@@ -416,7 +416,7 @@ standard_install (SIM_DESC sd)
 /* Return non-zero if arg is a duplicate argument.
    If ARG is NULL, initialize.  */
 
-#define ARG_HASH_SIZE 97
+#define ARG_HASH_SIZE 256
 #define ARG_HASH(a) ((256 * (unsigned char) a[0] + (unsigned char) a[1]) % ARG_HASH_SIZE)
 
 static int
@@ -478,6 +478,9 @@ sim_parse_args (SIM_DESC sd, char * const *argv)
       for (opt = ol->options; OPTION_VALID_P (opt); ++opt)
 	++num_opts;
 
+  /* We build a hash table of all options, so make sure they all fit.  */
+  SIM_ASSERT (num_opts <= ARG_HASH_SIZE);
+
   /* Initialize duplicate argument checker.  */
   (void) dup_arg_p (NULL);
 
-- 
2.30.2

Re: [PATCH/committed] sim: options: increase max option count

[Tom Tromey]

>>>>> "Mike" == Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org> writes:

Mike> This will be needed to turn on hw support for v850 as it will then
Mike> exceed the current limit.

The libiberty hash table is easy to use and doesn't have this limitation.
WDYT of the appended?

Tom

diff --git a/sim/common/sim-options.c b/sim/common/sim-options.c
index ab95984e833..a6da8eccdef 100644
--- a/sim/common/sim-options.c
+++ b/sim/common/sim-options.c
@@ -28,6 +28,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "sim-io.h"
 #include "sim-assert.h"
 #include "version.h"
+#include "hashtab.h"
 
 #include "bfd.h"
 
@@ -413,37 +414,36 @@ standard_install (SIM_DESC sd)
   return SIM_RC_OK;
 }
 
+/* Equality function for arguments.  */
+
+static int
+compare_strings (const void *a, const void *b)
+{
+  return strcmp (a, b) == 0;
+}
+
 /* Return non-zero if arg is a duplicate argument.
    If ARG is NULL, initialize.  */
 
-#define ARG_HASH_SIZE 256
-#define ARG_HASH(a) ((256 * (unsigned char) a[0] + (unsigned char) a[1]) % ARG_HASH_SIZE)
-
 static int
 dup_arg_p (const char *arg)
 {
-  int hash;
-  static const char **arg_table = NULL;
+  static htab_t arg_table = NULL;
+  void **slot;
 
   if (arg == NULL)
     {
       if (arg_table == NULL)
-	arg_table = (const char **) xmalloc (ARG_HASH_SIZE * sizeof (char *));
-      memset (arg_table, 0, ARG_HASH_SIZE * sizeof (char *));
+	arg_table = htab_create_alloc (10, htab_hash_string,
+				       compare_strings, NULL,
+				       xcalloc, free);
       return 0;
     }
 
-  hash = ARG_HASH (arg);
-  while (arg_table[hash] != NULL)
-    {
-      if (strcmp (arg, arg_table[hash]) == 0)
-	return 1;
-      /* We assume there won't be more than ARG_HASH_SIZE arguments so we
-	 don't check if the table is full.  */
-      if (++hash == ARG_HASH_SIZE)
-	hash = 0;
-    }
-  arg_table[hash] = arg;
+  slot = htab_find_slot (arg_table, arg, INSERT);
+  if (*slot != NULL)
+    return 1;
+  *slot = (void *) arg;
   return 0;
 }
 
@@ -478,9 +478,6 @@ sim_parse_args (SIM_DESC sd, char * const *argv)
       for (opt = ol->options; OPTION_VALID_P (opt); ++opt)
 	++num_opts;
 
-  /* We build a hash table of all options, so make sure they all fit.  */
-  SIM_ASSERT (num_opts <= ARG_HASH_SIZE);
-
   /* Initialize duplicate argument checker.  */
   (void) dup_arg_p (NULL);
 

Re: [PATCH/committed] sim: options: increase max option count

[Mike Frysinger]

On 24 Apr 2021 15:00, Tom Tromey wrote:
> >>>>> "Mike" == Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Mike> This will be needed to turn on hw support for v850 as it will then
> Mike> exceed the current limit.
> 
> The libiberty hash table is easy to use and doesn't have this limitation.
> WDYT of the appended?

nice, lgtm
-mike

[PATCH/committed] sim: options: fix --help output

[Mike Frysinger]

The hash table rewrite broke --help output due to subtle behavior:
calling dup_arg_p(NULL) will create & clear the table, not just
create it.  The --help output relies on this to clear the table
before it shows things.
---
 sim/common/ChangeLog     | 4 ++++
 sim/common/sim-options.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
index 55d684bfeab8..7abaeb3a8166 100644
--- a/sim/common/ChangeLog
+++ b/sim/common/ChangeLog
@@ -1,3 +1,7 @@
+2021-05-01  Mike Frysinger  <vapier@gentoo.org>
+
+	* sim-options.c (dup_arg_p): Call htab_empty.
+
 2021-05-01  Mike Frysinger  <vapier@gentoo.org>
 
 	* dv-sockser.c (dv_sockser_install): Rename to ...
diff --git a/sim/common/sim-options.c b/sim/common/sim-options.c
index a6da8eccdefa..1522cac9379f 100644
--- a/sim/common/sim-options.c
+++ b/sim/common/sim-options.c
@@ -437,6 +437,7 @@ dup_arg_p (const char *arg)
 	arg_table = htab_create_alloc (10, htab_hash_string,
 				       compare_strings, NULL,
 				       xcalloc, free);
+      htab_empty (arg_table);
       return 0;
     }
 
-- 
2.31.1

Re: [PATCH/committed] sim: options: fix --help output

[Tom Tromey]

Mike> The hash table rewrite broke --help output due to subtle behavior:
Mike> calling dup_arg_p(NULL) will create & clear the table, not just
Mike> create it.  The --help output relies on this to clear the table
Mike> before it shows things.

Oops, I'm sorry about that.  Thank you for fixing it.

Tom