Sim hangs on new target at dup_arg_p() in infinite loop.

Sim hangs on new target at dup_arg_p() in infinite loop.

[Alexey Makhalov]

[-- Attachment #1: Type: text/plain, Size: 225 bytes --]

I've added checking for hash table overflow. It's better to have
internal error message instead of looping.

2013-09-09 Alexey Makhalov <makhaloff@gmail.com>
        * sim-options.c (dup_arg_p) Check for hash table overflow.

[-- Attachment #2: hash_table_overflow_checking.patch --]
[-- Type: application/octet-stream, Size: 985 bytes --]

Index: sim/common/sim-options.c
===================================================================
RCS file: /cvs/src/src/sim/common/sim-options.c,v
retrieving revision 1.31
diff -u -p -r1.31 sim-options.c
--- sim/common/sim-options.c	3 Sep 2013 20:45:08 -0000	1.31
+++ sim/common/sim-options.c	9 Sep 2013 20:12:25 -0000
@@ -495,6 +495,7 @@ static int
 dup_arg_p (const char *arg)
 {
   int hash;
+  int table_is_full = 0;
   static const char **arg_table = NULL;
 
   if (arg == NULL)
@@ -510,10 +511,15 @@ dup_arg_p (const char *arg)
     {
       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;
+	if (table_is_full++) {
+          fprintf (stderr, "Internal error - number of options more than %d\n",
+			  ARG_HASH_SIZE);
+          abort ();
+	}	
+      }
     }
   arg_table[hash] = arg;
   return 0;

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 828 bytes --]

On Monday 09 September 2013 16:13:38 Alexey Makhalov wrote:
> I've added checking for hash table overflow. It's better to have
> internal error message instead of looping.

please fix your mailer to send patches inline rather than base64 encoded.  it 
makes reviewing a lot easier when you do.

> --- sim/common/sim-options.c	3 Sep 2013 20:45:08 -0000	1.31
> +++ sim/common/sim-options.c	9 Sep 2013 20:12:25 -0000
>
> +	if (table_is_full++) {

GNU style says the brace goes on the following line.

> +          fprintf (stderr, "Internal error - number of options more than 
%d\n",

use sim_io_eprintf instead of fprintf

> +			  ARG_HASH_SIZE);
> +          abort ();
> +	}	
> +      }

also, i'm pretty sure the indentation style in your change is incorrect.  
please adjust to the GNU style.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Tom Tromey]

>>>>> "Alexey" == Alexey Makhalov <makhaloff@gmail.com> writes:

Alexey> I've added checking for hash table overflow. It's better to have
Alexey> internal error message instead of looping.

Alexey> 2013-09-09 Alexey Makhalov <makhaloff@gmail.com>
Alexey>         * sim-options.c (dup_arg_p) Check for hash table overflow.

Is it possible to just use the libiberty hashtab code and not have it
ever overflow?

Tom

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Alexey Makhalov]

[-- Attachment #1: Type: text/plain, Size: 2709 bytes --]

Tom, thanks for advice.
Now it works fine.


Index: sim/common/sim-options.c
===================================================================
RCS file: /cvs/src/src/sim/common/sim-options.c,v
retrieving revision 1.31
diff -u -p -r1.31 sim-options.c
--- sim/common/sim-options.c 3 Sep 2013 20:45:08 -0000 1.31
+++ sim/common/sim-options.c 20 Sep 2013 04:57:09 -0000
@@ -35,6 +35,7 @@ along with this program.  If not, see <h
 #include "sim-assert.h"

 #include "bfd.h"
+#include "hashtab.h"

 /* Add a set of options to the simulator.
    TABLE is an array of OPTIONS terminated by a NULL `opt.name' entry.
@@ -485,37 +486,40 @@ standard_install (SIM_DESC sd)
   return SIM_RC_OK;
 }

+/* A simple comparison function with opposite semantics to strcmp.  */
+
+static int
+streq (const char *lhs, const char *rhs)
+{
+  return !strcmp (lhs, rhs);
+}
+
 /* Return non-zero if arg is a duplicate argument.
    If ARG is NULL, initialize.  */

-#define ARG_HASH_SIZE 97
-#define ARG_HASH(a) ((256 * (unsigned char) a[0] + (unsigned char)
a[1]) % ARG_HASH_SIZE)
+#define ARG_HASH_SIZE 256

 static int
 dup_arg_p (const char *arg)
 {
-  int hash;
-  static const char **arg_table = NULL;
+  void **slot;
+  static htab_t arg_table = NULL;

   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 (ARG_HASH_SIZE, htab_hash_string,
+               (int (*)(const void *, const void *)) streq, free);
+      else
+        htab_empty (arg_table);
       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 = strdup(arg);
   return 0;
 }

On Tue, Sep 17, 2013 at 2:52 PM, Alexey Makhalov <makhaloff@gmail.com> wrote:
> I'll try.
>
>
> On Tue, Sep 17, 2013 at 10:29 AM, Tom Tromey <tromey@redhat.com> wrote:
>>
>> >>>>> "Alexey" == Alexey Makhalov <makhaloff@gmail.com> writes:
>>
>> Alexey> I've added checking for hash table overflow. It's better to have
>> Alexey> internal error message instead of looping.
>>
>> Alexey> 2013-09-09 Alexey Makhalov <makhaloff@gmail.com>
>> Alexey>         * sim-options.c (dup_arg_p) Check for hash table overflow.
>>
>> Is it possible to just use the libiberty hashtab code and not have it
>> ever overflow?
>>
>> Tom
>
>

[-- Attachment #2: htab.patch --]
[-- Type: application/octet-stream, Size: 2052 bytes --]

Index: sim/common/sim-options.c
===================================================================
RCS file: /cvs/src/src/sim/common/sim-options.c,v
retrieving revision 1.31
diff -u -p -r1.31 sim-options.c
--- sim/common/sim-options.c	3 Sep 2013 20:45:08 -0000	1.31
+++ sim/common/sim-options.c	20 Sep 2013 04:57:09 -0000
@@ -35,6 +35,7 @@ along with this program.  If not, see <h
 #include "sim-assert.h"
 
 #include "bfd.h"
+#include "hashtab.h"
 
 /* Add a set of options to the simulator.
    TABLE is an array of OPTIONS terminated by a NULL `opt.name' entry.
@@ -485,37 +486,40 @@ standard_install (SIM_DESC sd)
   return SIM_RC_OK;
 }
 
+/* A simple comparison function with opposite semantics to strcmp.  */
+
+static int
+streq (const char *lhs, const char *rhs)
+{
+  return !strcmp (lhs, rhs);
+}
+
 /* Return non-zero if arg is a duplicate argument.
    If ARG is NULL, initialize.  */
 
-#define ARG_HASH_SIZE 97
-#define ARG_HASH(a) ((256 * (unsigned char) a[0] + (unsigned char) a[1]) % ARG_HASH_SIZE)
+#define ARG_HASH_SIZE 256
 
 static int
 dup_arg_p (const char *arg)
 {
-  int hash;
-  static const char **arg_table = NULL;
+  void **slot;
+  static htab_t arg_table = NULL;
 
   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 (ARG_HASH_SIZE, htab_hash_string, 
+               (int (*)(const void *, const void *)) streq, free);
+      else
+        htab_empty (arg_table);
       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 = strdup(arg);
   return 0;
 }

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Tom Tromey]

>>>>> "Alexey" == Alexey Makhalov <makhaloff@gmail.com> writes:

Alexey> +        arg_table = htab_create (ARG_HASH_SIZE, htab_hash_string,
Alexey> +               (int (*)(const void *, const void *)) streq, free);

I think casts like this are better avoided.
Since streq is only used here, it is no trouble to just give it the
proper type.

Alexey> +  *slot = strdup(arg);

The GNU coding style requires a space before the paren here.

Tom

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Alexey Makhalov]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

fixed


Index: sim/common/sim-options.c
===================================================================
RCS file: /cvs/src/src/sim/common/sim-options.c,v
retrieving revision 1.31
diff -u -p -r1.31 sim-options.c
--- sim/common/sim-options.c 3 Sep 2013 20:45:08 -0000 1.31
+++ sim/common/sim-options.c 21 Sep 2013 02:56:18 -0000
@@ -35,6 +35,7 @@ along with this program.  If not, see <h
 #include "sim-assert.h"

 #include "bfd.h"
+#include "hashtab.h"

 /* Add a set of options to the simulator.
    TABLE is an array of OPTIONS terminated by a NULL `opt.name' entry.
@@ -485,37 +486,39 @@ standard_install (SIM_DESC sd)
   return SIM_RC_OK;
 }

+/* A simple comparison function with opposite semantics to strcmp.  */
+
+static int
+streq (const void *lhs, const void *rhs)
+{
+  return !strcmp ((const char *) lhs, (const char *) rhs);
+}
+
 /* Return non-zero if arg is a duplicate argument.
    If ARG is NULL, initialize.  */

-#define ARG_HASH_SIZE 97
-#define ARG_HASH(a) ((256 * (unsigned char) a[0] + (unsigned char)
a[1]) % ARG_HASH_SIZE)
+#define ARG_HASH_SIZE 256

 static int
 dup_arg_p (const char *arg)
 {
-  int hash;
-  static const char **arg_table = NULL;
+  void **slot;
+  static htab_t arg_table = NULL;

   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 (ARG_HASH_SIZE, htab_hash_string, streq, free);
+      else
+ htab_empty (arg_table);
       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 = strdup (arg);
   return 0;
 }


On Fri, Sep 20, 2013 at 10:33 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Alexey" == Alexey Makhalov <makhaloff@gmail.com> writes:
>
> Alexey> +        arg_table = htab_create (ARG_HASH_SIZE, htab_hash_string,
> Alexey> +               (int (*)(const void *, const void *)) streq, free);
>
> I think casts like this are better avoided.
> Since streq is only used here, it is no trouble to just give it the
> proper type.
>
> Alexey> +  *slot = strdup(arg);
>
> The GNU coding style requires a space before the paren here.
>
> Tom

[-- Attachment #2: htab.patch --]
[-- Type: application/octet-stream, Size: 2023 bytes --]

Index: sim/common/sim-options.c
===================================================================
RCS file: /cvs/src/src/sim/common/sim-options.c,v
retrieving revision 1.31
diff -u -p -r1.31 sim-options.c
--- sim/common/sim-options.c	3 Sep 2013 20:45:08 -0000	1.31
+++ sim/common/sim-options.c	21 Sep 2013 02:56:18 -0000
@@ -35,6 +35,7 @@ along with this program.  If not, see <h
 #include "sim-assert.h"
 
 #include "bfd.h"
+#include "hashtab.h"
 
 /* Add a set of options to the simulator.
    TABLE is an array of OPTIONS terminated by a NULL `opt.name' entry.
@@ -485,37 +486,39 @@ standard_install (SIM_DESC sd)
   return SIM_RC_OK;
 }
 
+/* A simple comparison function with opposite semantics to strcmp.  */
+
+static int
+streq (const void *lhs, const void *rhs)
+{
+  return !strcmp ((const char *) lhs, (const char *) rhs);
+}
+
 /* Return non-zero if arg is a duplicate argument.
    If ARG is NULL, initialize.  */
 
-#define ARG_HASH_SIZE 97
-#define ARG_HASH(a) ((256 * (unsigned char) a[0] + (unsigned char) a[1]) % ARG_HASH_SIZE)
+#define ARG_HASH_SIZE 256
 
 static int
 dup_arg_p (const char *arg)
 {
-  int hash;
-  static const char **arg_table = NULL;
+  void **slot;
+  static htab_t arg_table = NULL;
 
   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 (ARG_HASH_SIZE, htab_hash_string, streq, free);
+      else
+	htab_empty (arg_table);
       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 = strdup (arg);
   return 0;
 }
 

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Tom Tromey]

>>>>> "Alexey" == Alexey Makhalov <makhaloff@gmail.com> writes:

Alexey> fixed

I never saw a response to this.

The patch looks good.
It needs a ChangeLog entry.
Also, do you have a copyright assignment in place?

thanks,
Tom

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Alexey Makhalov]

Hi,

ChangeLog entry:
2013-11-07 Alexey Makhalov <makhaloff@gmail.com>
        * sim-options.c (dup_arg_p) Using hash table from libiberty to
store sim args.

And about copyright assignment, I have no any special one.
Thanks.
Alexey.

On Thu, Nov 7, 2013 at 5:14 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Alexey" == Alexey Makhalov <makhaloff@gmail.com> writes:
>
> Alexey> fixed
>
> I never saw a response to this.
>
> The patch looks good.
> It needs a ChangeLog entry.
> Also, do you have a copyright assignment in place?
>
> thanks,
> Tom

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 443 bytes --]

On Friday 08 November 2013 01:37:35 Alexey Makhalov wrote:
> And about copyright assignment, I have no any special one.

well, the good news is that the sim/ subdir is a mess and we haven't been 
enforcing copyright assignment.  there is a lot of code in there not owned by 
the FSF.  considering that, i'm not sure requiring future submissions in there 
would be useful if people aren't actively chasing down past contributors.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Joel Brobecker]

> > And about copyright assignment, I have no any special one.
> 
> well, the good news is that the sim/ subdir is a mess and we haven't
> been enforcing copyright assignment.  there is a lot of code in there
> not owned by the FSF.  considering that, i'm not sure requiring future
> submissions in there would be useful if people aren't actively chasing
> down past contributors.

This is unfortunately a real thorn in our paw, and the FSF wants us
to make sure that all new contributions are properly assigned.
I'm sorry if this is a bit of a pain, but it should be a one-time
process. In the meantime, for those who do not have a copyright
assignment in place, we can only accept contributions falling
under the "small" or "obvious" exemption.

-- 
Joel

Re: Sim hangs on new target at dup_arg_p() in infinite loop.

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 983 bytes --]

On Wednesday 27 November 2013 04:49:13 Joel Brobecker wrote:
> > > And about copyright assignment, I have no any special one.
> > 
> > well, the good news is that the sim/ subdir is a mess and we haven't
> > been enforcing copyright assignment.  there is a lot of code in there
> > not owned by the FSF.  considering that, i'm not sure requiring future
> > submissions in there would be useful if people aren't actively chasing
> > down past contributors.
> 
> This is unfortunately a real thorn in our paw, and the FSF wants us
> to make sure that all new contributions are properly assigned.
> I'm sorry if this is a bit of a pain, but it should be a one-time
> process. In the meantime, for those who do not have a copyright
> assignment in place, we can only accept contributions falling
> under the "small" or "obvious" exemption.

np

Alexey: can you follow this:
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/assign.changes.manual
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]