[PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Manish Goregaokar]

strsep doesn't exist on windows

2016-06-29  Manish Goregaokar  <manish@mozilla.com>

gdb/ChangeLog:
    * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep
---
 gdb/rust-lang.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 23ddd5a..961fdca 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
   if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
            strlen (RUST_ENUM_PREFIX)) == 0)
     {
-      char *name, *tail, *token;
+      char *tail, *token, *name, *last = NULL;
       unsigned long fieldno;
       struct type *member_type;
       LONGEST value;
@@ -138,6 +138,8 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
       cleanup = make_cleanup (xfree, name);
       tail = name + strlen (RUST_ENUM_PREFIX);

+      token = strtok_r (tail, "$", &last);
+
       /* The location of the value that doubles as a discriminant is
          stored in the name of the field, as
          RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
@@ -145,7 +147,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
          traversed in order to find the field (which may be several
fields deep)
          and the variantname is the name of the variant of the case when the
          field is zero.  */
-      while ((token = strsep (&tail, "$")) != NULL)
+      do
         {
       if (sscanf (token, "%lu", &fieldno) != 1)
         {
@@ -159,10 +161,8 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,

           embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
           member_type = TYPE_FIELD_TYPE (member_type, fieldno);
-        }
+        } while ((token = strtok_r (NULL, "$", &last)) != NULL);

-      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
-    error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
       value = unpack_long (member_type, valaddr + embedded_offset);

       if (value == 0)
-- 
2.8.3

Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Pedro Alves]

(stepping in since this fixes a build breakage.)

On 06/29/2016 11:13 AM, Manish Goregaokar wrote:
> strsep doesn't exist on windows

"Windows", uppercase.

> 
> 2016-06-29  Manish Goregaokar  <manish@mozilla.com>
> 
> gdb/ChangeLog:
>     * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep

Period at end of sentences.  Likewise on the commit log,
while at it.

> ---
>  gdb/rust-lang.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
> index 23ddd5a..961fdca 100644
> --- a/gdb/rust-lang.c
> +++ b/gdb/rust-lang.c
> @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>    if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>             strlen (RUST_ENUM_PREFIX)) == 0)
>      {
> -      char *name, *tail, *token;
> +      char *tail, *token, *name, *last = NULL;

(I don't think you need to initialize this.  If you get
a warning about it, it's likely revealing a bug.)

>        unsigned long fieldno;
>        struct type *member_type;
>        LONGEST value;
> @@ -138,6 +138,8 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>        cleanup = make_cleanup (xfree, name);
>        tail = name + strlen (RUST_ENUM_PREFIX);
> 
> +      token = strtok_r (tail, "$", &last);
> +
>        /* The location of the value that doubles as a discriminant is
>           stored in the name of the field, as
>           RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
> @@ -145,7 +147,7 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>           traversed in order to find the field (which may be several
> fields deep)
>           and the variantname is the name of the variant of the case when the
>           field is zero.  */
> -      while ((token = strsep (&tail, "$")) != NULL)
> +      do
>          {
>        if (sscanf (token, "%lu", &fieldno) != 1)

We'll now reach here even if token is NULL the first time.

>          {
> @@ -159,10 +161,8 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
> 
>            embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
>            member_type = TYPE_FIELD_TYPE (member_type, fieldno);
> -        }
> +        } while ((token = strtok_r (NULL, "$", &last)) != NULL);

IMO, this calls for a "for" loop instead of a do/while.  Like:

 -      while ((token = strsep (&tail, "$")) != NULL)
 +      for (token = strtok_r (tail, "$", &saveptr);
 +           token != NULL;
 +           token = strtok_r (NULL, "$", &saveptr))
         {

Thanks,
Pedro Alves

Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Manish Goregaokar]

Fixed.

From 6b577bb78279a453c77b08c5a1090d8dec1eeee7 Mon Sep 17 00:00:00 2001
From: Manish Goregaokar <manish@mozilla.com>
Date: Wed, 29 Jun 2016 15:42:28 +0530
Subject: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

strsep doesn't exist on Windows.

2016-06-29  Manish Goregaokar  <manish@mozilla.com>

gdb/ChangeLog:
    * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep.
---
 gdb/rust-lang.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 23ddd5a..5910d4b 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
   if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
            strlen (RUST_ENUM_PREFIX)) == 0)
     {
-      char *name, *tail, *token;
+      char *tail, *token, *name, *saveptr;
       unsigned long fieldno;
       struct type *member_type;
       LONGEST value;
@@ -145,7 +145,9 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
          traversed in order to find the field (which may be several
fields deep)
          and the variantname is the name of the variant of the case when the
          field is zero.  */
-      while ((token = strsep (&tail, "$")) != NULL)
+      for (token = strtok_r (tail, "$", &saveptr);
+           token != NULL;
+           token = strtok_r (NULL, "$", &saveptr))
         {
       if (sscanf (token, "%lu", &fieldno) != 1)
         {
@@ -161,8 +163,6 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
           member_type = TYPE_FIELD_TYPE (member_type, fieldno);
         }

-      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
-    error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
       value = unpack_long (member_type, valaddr + embedded_offset);

       if (value == 0)
-- 
2.8.3

-Manish


On Wed, Jun 29, 2016 at 3:56 PM, Pedro Alves <palves@redhat.com> wrote:
> (stepping in since this fixes a build breakage.)
>
> On 06/29/2016 11:13 AM, Manish Goregaokar wrote:
>> strsep doesn't exist on windows
>
> "Windows", uppercase.
>
>>
>> 2016-06-29  Manish Goregaokar  <manish@mozilla.com>
>>
>> gdb/ChangeLog:
>>     * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep
>
> Period at end of sentences.  Likewise on the commit log,
> while at it.
>
>> ---
>>  gdb/rust-lang.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
>> index 23ddd5a..961fdca 100644
>> --- a/gdb/rust-lang.c
>> +++ b/gdb/rust-lang.c
>> @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
>> gdb_byte *valaddr,
>>    if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>>             strlen (RUST_ENUM_PREFIX)) == 0)
>>      {
>> -      char *name, *tail, *token;
>> +      char *tail, *token, *name, *last = NULL;
>
> (I don't think you need to initialize this.  If you get
> a warning about it, it's likely revealing a bug.)
>
>>        unsigned long fieldno;
>>        struct type *member_type;
>>        LONGEST value;
>> @@ -138,6 +138,8 @@ rust_get_disr_info (struct type *type, const
>> gdb_byte *valaddr,
>>        cleanup = make_cleanup (xfree, name);
>>        tail = name + strlen (RUST_ENUM_PREFIX);
>>
>> +      token = strtok_r (tail, "$", &last);
>> +
>>        /* The location of the value that doubles as a discriminant is
>>           stored in the name of the field, as
>>           RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
>> @@ -145,7 +147,7 @@ rust_get_disr_info (struct type *type, const
>> gdb_byte *valaddr,
>>           traversed in order to find the field (which may be several
>> fields deep)
>>           and the variantname is the name of the variant of the case when the
>>           field is zero.  */
>> -      while ((token = strsep (&tail, "$")) != NULL)
>> +      do
>>          {
>>        if (sscanf (token, "%lu", &fieldno) != 1)
>
> We'll now reach here even if token is NULL the first time.
>
>>          {
>> @@ -159,10 +161,8 @@ rust_get_disr_info (struct type *type, const
>> gdb_byte *valaddr,
>>
>>            embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
>>            member_type = TYPE_FIELD_TYPE (member_type, fieldno);
>> -        }
>> +        } while ((token = strtok_r (NULL, "$", &last)) != NULL);
>
> IMO, this calls for a "for" loop instead of a do/while.  Like:
>
>  -      while ((token = strsep (&tail, "$")) != NULL)
>  +      for (token = strtok_r (tail, "$", &saveptr);
>  +           token != NULL;
>  +           token = strtok_r (NULL, "$", &saveptr))
>          {
>
> Thanks,
> Pedro Alves
>

Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Manish Goregaokar]

Actually, it occurs to me that token could still be null after the
loop, and we use the value of token to get the name.

New patch:

--------------


strsep doesn't exist on Windows.

2016-06-29  Manish Goregaokar  <manish@mozilla.com>

gdb/ChangeLog:
    * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep.
---
 gdb/rust-lang.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 23ddd5a..c01687a 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
   if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
            strlen (RUST_ENUM_PREFIX)) == 0)
     {
-      char *name, *tail, *token;
+      char *tail, *token, *name, *saveptr;
       unsigned long fieldno;
       struct type *member_type;
       LONGEST value;
@@ -145,7 +145,9 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
          traversed in order to find the field (which may be several
fields deep)
          and the variantname is the name of the variant of the case when the
          field is zero.  */
-      while ((token = strsep (&tail, "$")) != NULL)
+      for (token = strtok_r (tail, "$", &saveptr);
+           token != NULL;
+           token = strtok_r (NULL, "$", &saveptr))
         {
       if (sscanf (token, "%lu", &fieldno) != 1)
         {
@@ -161,7 +163,7 @@ rust_get_disr_info (struct type *type, const
gdb_byte *valaddr,
           member_type = TYPE_FIELD_TYPE (member_type, fieldno);
         }

-      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
+      if (token == NULL)
     error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
       value = unpack_long (member_type, valaddr + embedded_offset);

-- 
2.8.3

-Manish


On Wed, Jun 29, 2016 at 4:01 PM, Manish Goregaokar <manish@mozilla.com> wrote:
> Fixed.
>
> From 6b577bb78279a453c77b08c5a1090d8dec1eeee7 Mon Sep 17 00:00:00 2001
> From: Manish Goregaokar <manish@mozilla.com>
> Date: Wed, 29 Jun 2016 15:42:28 +0530
> Subject: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info
>
> strsep doesn't exist on Windows.
>
> 2016-06-29  Manish Goregaokar  <manish@mozilla.com>
>
> gdb/ChangeLog:
>     * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep.
> ---
>  gdb/rust-lang.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
> index 23ddd5a..5910d4b 100644
> --- a/gdb/rust-lang.c
> +++ b/gdb/rust-lang.c
> @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>    if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>             strlen (RUST_ENUM_PREFIX)) == 0)
>      {
> -      char *name, *tail, *token;
> +      char *tail, *token, *name, *saveptr;
>        unsigned long fieldno;
>        struct type *member_type;
>        LONGEST value;
> @@ -145,7 +145,9 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>           traversed in order to find the field (which may be several
> fields deep)
>           and the variantname is the name of the variant of the case when the
>           field is zero.  */
> -      while ((token = strsep (&tail, "$")) != NULL)
> +      for (token = strtok_r (tail, "$", &saveptr);
> +           token != NULL;
> +           token = strtok_r (NULL, "$", &saveptr))
>          {
>        if (sscanf (token, "%lu", &fieldno) != 1)
>          {
> @@ -161,8 +163,6 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>            member_type = TYPE_FIELD_TYPE (member_type, fieldno);
>          }
>
> -      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
> -    error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
>        value = unpack_long (member_type, valaddr + embedded_offset);
>
>        if (value == 0)
> --
> 2.8.3
>
> -Manish
>
>
> On Wed, Jun 29, 2016 at 3:56 PM, Pedro Alves <palves@redhat.com> wrote:
>> (stepping in since this fixes a build breakage.)
>>
>> On 06/29/2016 11:13 AM, Manish Goregaokar wrote:
>>> strsep doesn't exist on windows
>>
>> "Windows", uppercase.
>>
>>>
>>> 2016-06-29  Manish Goregaokar  <manish@mozilla.com>
>>>
>>> gdb/ChangeLog:
>>>     * rust-lang.c (rust_get_disr_info): Use strtok_r instead of strsep
>>
>> Period at end of sentences.  Likewise on the commit log,
>> while at it.
>>
>>> ---
>>>  gdb/rust-lang.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
>>> index 23ddd5a..961fdca 100644
>>> --- a/gdb/rust-lang.c
>>> +++ b/gdb/rust-lang.c
>>> @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>>    if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>>>             strlen (RUST_ENUM_PREFIX)) == 0)
>>>      {
>>> -      char *name, *tail, *token;
>>> +      char *tail, *token, *name, *last = NULL;
>>
>> (I don't think you need to initialize this.  If you get
>> a warning about it, it's likely revealing a bug.)
>>
>>>        unsigned long fieldno;
>>>        struct type *member_type;
>>>        LONGEST value;
>>> @@ -138,6 +138,8 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>>        cleanup = make_cleanup (xfree, name);
>>>        tail = name + strlen (RUST_ENUM_PREFIX);
>>>
>>> +      token = strtok_r (tail, "$", &last);
>>> +
>>>        /* The location of the value that doubles as a discriminant is
>>>           stored in the name of the field, as
>>>           RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
>>> @@ -145,7 +147,7 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>>           traversed in order to find the field (which may be several
>>> fields deep)
>>>           and the variantname is the name of the variant of the case when the
>>>           field is zero.  */
>>> -      while ((token = strsep (&tail, "$")) != NULL)
>>> +      do
>>>          {
>>>        if (sscanf (token, "%lu", &fieldno) != 1)
>>
>> We'll now reach here even if token is NULL the first time.
>>
>>>          {
>>> @@ -159,10 +161,8 @@ rust_get_disr_info (struct type *type, const
>>> gdb_byte *valaddr,
>>>
>>>            embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
>>>            member_type = TYPE_FIELD_TYPE (member_type, fieldno);
>>> -        }
>>> +        } while ((token = strtok_r (NULL, "$", &last)) != NULL);
>>
>> IMO, this calls for a "for" loop instead of a do/while.  Like:
>>
>>  -      while ((token = strsep (&tail, "$")) != NULL)
>>  +      for (token = strtok_r (tail, "$", &saveptr);
>>  +           token != NULL;
>>  +           token = strtok_r (NULL, "$", &saveptr))
>>          {
>>
>> Thanks,
>> Pedro Alves
>>

Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Pedro Alves]

On 06/29/2016 11:31 AM, Manish Goregaokar wrote:
> @@ -161,8 +163,6 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>            member_type = TYPE_FIELD_TYPE (member_type, fieldno);
>          }
> 
> -      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
> -    error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
>        value = unpack_long (member_type, valaddr + embedded_offset);
> 
>        if (value == 0)

Why was this bit removed?

Thanks,
Pedro Alves

Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Manish Goregaokar]

strsep moves the pointer to the end of the string. We want to ensure
the early break happens, since we need token to contain the name of
the variant at this stage.

strtok doesn't do this. However, strtok does make it NULL, which we
don't want either. I added a check for it in my most recent email,
tests still pass.

Thanks,
-Manish


On Wed, Jun 29, 2016 at 4:06 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/29/2016 11:31 AM, Manish Goregaokar wrote:
>> @@ -161,8 +163,6 @@ rust_get_disr_info (struct type *type, const
>> gdb_byte *valaddr,
>>            member_type = TYPE_FIELD_TYPE (member_type, fieldno);
>>          }
>>
>> -      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
>> -    error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
>>        value = unpack_long (member_type, valaddr + embedded_offset);
>>
>>        if (value == 0)
>
> Why was this bit removed?
>
> Thanks,
> Pedro Alves
>

Re: [PATCH] Use strtok_r instead of strsep in rust_get_disr_info

[Pedro Alves]

On 06/29/2016 11:38 AM, Manish Goregaokar wrote:
> strsep moves the pointer to the end of the string. We want to ensure
> the early break happens, since we need token to contain the name of
> the variant at this stage.
> 
> strtok doesn't do this. However, strtok does make it NULL, which we
> don't want either. I added a check for it in my most recent email,
> tests still pass.

Right, that one is OK.  Please push.

Thanks,
Pedro Alves