RFC: Followup to PR91593

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: Followup to PR91593

jerry DeLisle-3
Hi all,

The following patch clears up the last two warnings in libgfortran/io. The chunk
in read.c is obvious. DEFAULT_WIDTH is defined as -1 and is used here as a flag.

The chunk in write.c is not so obvious.

I "think" it is correct, however, we need to find a machine to test this on and
I would like other eyes and thinking on this since it is subtle. It has to do
with endianess and my dyslexic brain could have it all wrong.

What machine in gcc compile farm is little endian so I can test?

Does anyone else see this better than I? Please comment.

Cheers,

Jerry

diff --git a/libgfortran/io/read.c b/libgfortran/io/read.c
index be9f6cb6f76..4a77e4384b7 100644
--- a/libgfortran/io/read.c
+++ b/libgfortran/io/read.c
@@ -638,7 +638,7 @@ read_decimal (st_parameter_dt *dtp, const fnode *f, char
*dest, int length)
    /* This is a legacy extension, and the frontend will only allow such cases
     * through when -fdec-format-defaults is passed.
     */
-  if (w == DEFAULT_WIDTH)
+  if (w == (size_t) DEFAULT_WIDTH)
      w = default_width_for_integer (length);

    p = read_block_form (dtp, &w);
diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
index 4ef35561fdd..fc046efbe34 100644
--- a/libgfortran/io/write.c
+++ b/libgfortran/io/write.c
@@ -1031,7 +1031,7 @@ btoa_big (const char *s, char *buffer, int len,
GFC_UINTEGER_LARGEST *n)
    else
      {
        const char *p = s + len - 1;
-      for (i = 0; i < len; i++)
+      for (i = 0; i < len - 1; i++)
  {
   char c = *p;

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

Steve Kargl
On Sun, Sep 29, 2019 at 09:02:07AM -0700, Jerry DeLisle wrote:

> Hi all,
>
> The following patch clears up the last two warnings in libgfortran/io.
> The chunk in read.c is obvious. DEFAULT_WIDTH is defined as -1 and
> is used here as a flag.
>
> The chunk in write.c is not so obvious.
>
> I "think" it is correct, however, we need to find a machine to test
> this on and I would like other eyes and thinking on this since it
> is subtle. It has to do with endianess and my dyslexic brain could
> have it all wrong.
>
> What machine in gcc compile farm is little endian so I can test?

Did you mean big endian?  Isn't 32-bit and 64-bit x86 little endian?
If you're looking for big endian, you probably want a powerpc system.

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

jerry DeLisle-3
On 9/29/19 10:57 AM, Steve Kargl wrote:

> On Sun, Sep 29, 2019 at 09:02:07AM -0700, Jerry DeLisle wrote:
>> Hi all,
>>
>> The following patch clears up the last two warnings in libgfortran/io.
>> The chunk in read.c is obvious. DEFAULT_WIDTH is defined as -1 and
>> is used here as a flag.
>>
>> The chunk in write.c is not so obvious.
>>
>> I "think" it is correct, however, we need to find a machine to test
>> this on and I would like other eyes and thinking on this since it
>> is subtle. It has to do with endianess and my dyslexic brain could
>> have it all wrong.
>>
>> What machine in gcc compile farm is little endian so I can test?
>
> Did you mean big endian?  Isn't 32-bit and 64-bit x86 little endian?
> If you're looking for big endian, you probably want a powerpc system.
>

My error, the possible fix is on the little endian part of the code.  Anyway, it
regression tests fine here without any problems. I have some more testing to do.
Let me know if you think the loop limit is right or not.

Jerry
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

Steve Kargl
In reply to this post by jerry DeLisle-3
On Mon, Sep 30, 2019 at 08:40:29PM -0700, Jerry DeLisle wrote:

> Copying gcc list for additional thoughts on a possible bogus warning.
>
> On 9/29/19 9:02 AM, Jerry DeLisle wrote:
> > Hi all,
> >
> --- snip ---
>
> > diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
> > index 4ef35561fdd..fc046efbe34 100644
> > --- a/libgfortran/io/write.c
> > +++ b/libgfortran/io/write.c
> > @@ -1031,7 +1031,7 @@ btoa_big (const char *s, char *buffer, int len,
> > GFC_UINTEGER_LARGEST *n)
> >     else
> >       {
> >         const char *p = s + len - 1;
> > -      for (i = 0; i < len; i++)
> > +      for (i = 0; i < len - 1; i++)
> >       {
> >         char c = *p;
> >
>
> --- snip ---
>
> The first attempt to fix (above) is completely off.  I have tried various
> combinations of code changes and I am beginning to think the warning is bogus:
>
> In function ‘btoa_big’,
>      inlined from ‘write_b’ at ../../../trunk/libgfortran/io/write.c:1217:11:
> ../../../trunk/libgfortran/io/write.c:1052:6: warning: writing 1 byte into a
> region of size 0 [-Wstringop-overflow=]
>   1052 |   *q = '\0';
>        |   ~~~^~~~~~
>
> Using gdb I have watched the pointer address stored in q and the setting of the
> string of bytes doing the binary to ascii conversion. I have also checked the
> length of the buffer being used and its is what I would expect with length of 129.
>
> However, the warning only goes away if I add an additional 8 bytes to the buffer
> (suspicious).
>
> So doing the following eliminates the warning:
>
> diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
> index 4ef35561fdd..fd0e46851e4 100644
> --- a/libgfortran/io/write.c
> +++ b/libgfortran/io/write.c
> @@ -1204,7 +1204,7 @@ void
>   write_b (st_parameter_dt *dtp, const fnode *f, const char *source, int len)
>   {
>     const char *p;
> -  char itoa_buf[GFC_BTOA_BUF_SIZE];
> +  char itoa_buf[GFC_BTOA_BUF_SIZE + 8];
>     GFC_UINTEGER_LARGEST n = 0;
>
>     if (len > (int) sizeof (GFC_UINTEGER_LARGEST))
>
> Any suggestions? I am certainly not seeing it.
>

Can you just zero memory and remove the explicit setting
of the terminating '\0'?

  q = buffer;
  memset(q, 0, len);

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

Steve Kargl
On Mon, Sep 30, 2019 at 08:51:51PM -0700, Steve Kargl wrote:
>
> Can you just zero memory and remove the explicit setting
> of the terminating '\0'?
>
>   q = buffer;
>   memset(q, 0, len);
>

(may be dup)

Upon further reading of the code, does buffer have
8*len+1 length?  You would need to do memset(q,0,8*len+1).

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

Andreas Schwab-2
In reply to this post by jerry DeLisle-3
On Sep 30 2019, Jerry DeLisle <[hidden email]> wrote:

> The first attempt to fix (above) is completely off.  I have tried various
> combinations of code changes and I am beginning to think the warning is
> bogus:
>
> In function ‘btoa_big’,
>     inlined from ‘write_b’ at ../../../trunk/libgfortran/io/write.c:1217:11:
> ../../../trunk/libgfortran/io/write.c:1052:6: warning: writing 1 byte into
> a region of size 0 [-Wstringop-overflow=]
>  1052 |   *q = '\0';
>       |   ~~~^~~~~~

If len > sizeof (GFC_UINTEGER_LARGEST), then you are writing more than
GFC_BTOA_BUF_SIZE, don't you?

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

Martin Sebor-2
In reply to this post by jerry DeLisle-3
On 9/30/19 9:40 PM, Jerry DeLisle wrote:

> Copying gcc list for additional thoughts on a possible bogus warning.
>
> On 9/29/19 9:02 AM, Jerry DeLisle wrote:
>> Hi all,
>>
> --- snip ---
>
>> diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
>> index 4ef35561fdd..fc046efbe34 100644
>> --- a/libgfortran/io/write.c
>> +++ b/libgfortran/io/write.c
>> @@ -1031,7 +1031,7 @@ btoa_big (const char *s, char *buffer, int len,
>> GFC_UINTEGER_LARGEST *n)
>>     else
>>       {
>>         const char *p = s + len - 1;
>> -      for (i = 0; i < len; i++)
>> +      for (i = 0; i < len - 1; i++)
>>       {
>>         char c = *p;
>>
>
> --- snip ---
>
> The first attempt to fix (above) is completely off.  I have tried
> various combinations of code changes and I am beginning to think the
> warning is bogus:
>
> In function ‘btoa_big’,
>      inlined from ‘write_b’ at
> ../../../trunk/libgfortran/io/write.c:1217:11:
> ../../../trunk/libgfortran/io/write.c:1052:6: warning: writing 1 byte
> into a region of size 0 [-Wstringop-overflow=]
>   1052 |   *q = '\0';
>        |   ~~~^~~~~~

In case it helps, the warning is for the access:

   # .MEM_68 = VDEF <.MEM_71>
   MEM[(char *)_86] = 0;

where _86 is set to

   _86 = &itoa_buf + _43;

and _43 has the range [136, 17179869176].  (The warning needs to
be enhanced a bit to mention the accessed object in this case.)
itoa_buf's DECL_SIZE_UNIT evaluates to 129.

The call to btoa_big in write_b:

       p = btoa_big (source, itoa_buf, len, &n);

is made with len > 16.  If len > sizeof itoa_buf  / 8 then it does
look like btoa_big would write past the end of itoa_buf because it
writes len * 8 bytes into it.  I don't know if the function can be
called with len that large but if not, adding this just above
the call suppresses the warning.

       if (len > sizeof itoa_buf / 8)
        __builtin_unreachable ();

Martin

> Using gdb I have watched the pointer address stored in q and the setting
> of the string of bytes doing the binary to ascii conversion. I have also
> checked the length of the buffer being used and its is what I would
> expect with length of 129.
>
> However, the warning only goes away if I add an additional 8 bytes to
> the buffer (suspicious).
>
> So doing the following eliminates the warning:
>
> diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
> index 4ef35561fdd..fd0e46851e4 100644
> --- a/libgfortran/io/write.c
> +++ b/libgfortran/io/write.c
> @@ -1204,7 +1204,7 @@ void
>   write_b (st_parameter_dt *dtp, const fnode *f, const char *source, int
> len)
>   {
>     const char *p;
> -  char itoa_buf[GFC_BTOA_BUF_SIZE];
> +  char itoa_buf[GFC_BTOA_BUF_SIZE + 8];
>     GFC_UINTEGER_LARGEST n = 0;
>
>     if (len > (int) sizeof (GFC_UINTEGER_LARGEST))
>
> Any suggestions? I am certainly not seeing it.
>
> Regards,
>
> Jerry
>
>
>
>
>
>
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

Martin Sebor-2
In reply to this post by jerry DeLisle-3
On 9/30/19 9:40 PM, Jerry DeLisle wrote:

> Copying gcc list for additional thoughts on a possible bogus warning.
>
> On 9/29/19 9:02 AM, Jerry DeLisle wrote:
>> Hi all,
>>
> --- snip ---
>
>> diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
>> index 4ef35561fdd..fc046efbe34 100644
>> --- a/libgfortran/io/write.c
>> +++ b/libgfortran/io/write.c
>> @@ -1031,7 +1031,7 @@ btoa_big (const char *s, char *buffer, int len,
>> GFC_UINTEGER_LARGEST *n)
>>     else
>>       {
>>         const char *p = s + len - 1;
>> -      for (i = 0; i < len; i++)
>> +      for (i = 0; i < len - 1; i++)
>>       {
>>         char c = *p;
>>
>
> --- snip ---
>
> The first attempt to fix (above) is completely off.  I have tried
> various combinations of code changes and I am beginning to think the
> warning is bogus:
>
> In function ‘btoa_big’,
>      inlined from ‘write_b’ at
> ../../../trunk/libgfortran/io/write.c:1217:11:
> ../../../trunk/libgfortran/io/write.c:1052:6: warning: writing 1 byte
> into a region of size 0 [-Wstringop-overflow=]
>   1052 |   *q = '\0';
>        |   ~~~^~~~~~

In case it helps, the warning is for the access:

   # .MEM_68 = VDEF <.MEM_71>
   MEM[(char *)_86] = 0;

where _86 is set to

   _86 = &itoa_buf + _43;

and _43 has the range [136, 17179869176].  (The warning needs to
be enhanced a bit to mention the accessed object in this case.)
itoa_buf's DECL_SIZE_UNIT evaluates to 129.

The call to btoa_big in write_b:

       p = btoa_big (source, itoa_buf, len, &n);

is made with len > 16.  If len > sizeof itoa_buf  / 8 then it does
look like btoa_big would write past the end of itoa_buf because it
writes len * 8 bytes into it.  I don't know if the function can be
called with len that large but if not, adding this just above
the call suppresses the warning.

       if (len > sizeof itoa_buf / 8)
        __builtin_unreachable ();

Martin

> Using gdb I have watched the pointer address stored in q and the setting
> of the string of bytes doing the binary to ascii conversion. I have also
> checked the length of the buffer being used and its is what I would
> expect with length of 129.
>
> However, the warning only goes away if I add an additional 8 bytes to
> the buffer (suspicious).
>
> So doing the following eliminates the warning:
>
> diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
> index 4ef35561fdd..fd0e46851e4 100644
> --- a/libgfortran/io/write.c
> +++ b/libgfortran/io/write.c
> @@ -1204,7 +1204,7 @@ void
>   write_b (st_parameter_dt *dtp, const fnode *f, const char *source, int
> len)
>   {
>     const char *p;
> -  char itoa_buf[GFC_BTOA_BUF_SIZE];
> +  char itoa_buf[GFC_BTOA_BUF_SIZE + 8];
>     GFC_UINTEGER_LARGEST n = 0;
>
>     if (len > (int) sizeof (GFC_UINTEGER_LARGEST))
>
> Any suggestions? I am certainly not seeing it.
>
> Regards,
>
> Jerry
>
>
>
>
>
>
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

jerry DeLisle-3
In reply to this post by Steve Kargl
On 9/30/19 8:57 PM, Steve Kargl wrote:

> On Mon, Sep 30, 2019 at 08:51:51PM -0700, Steve Kargl wrote:
>>
>> Can you just zero memory and remove the explicit setting
>> of the terminating '\0'?
>>
>>    q = buffer;
>>    memset(q, 0, len);
>>
>
> (may be dup)
>
> Upon further reading of the code, does buffer have
> 8*len+1 length?  You would need to do memset(q,0,8*len+1).
>

Commited as obvious.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91593

diff --git a/libgfortran/io/read.c b/libgfortran/io/read.c
index be9f6cb6f76..4a77e4384b7 100644
--- a/libgfortran/io/read.c
+++ b/libgfortran/io/read.c
@@ -638,7 +638,7 @@ read_decimal (st_parameter_dt *dtp, const fnode *f, char
*dest, int length)
    /* This is a legacy extension, and the frontend will only allow such cases
     * through when -fdec-format-defaults is passed.
     */
-  if (w == DEFAULT_WIDTH)
+  if (w == (size_t) DEFAULT_WIDTH)
      w = default_width_for_integer (length);

    p = read_block_form (dtp, &w);
diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
index 4ef35561fdd..eacd1f79715 100644
--- a/libgfortran/io/write.c
+++ b/libgfortran/io/write.c
@@ -1048,8 +1048,6 @@ btoa_big (const char *s, char *buffer, int len,
GFC_UINTEGER_LARGEST *n)
         }
      }

-  *q = '\0';
-
    if (*n == 0)
      return "0";

@@ -1207,6 +1205,9 @@ write_b (st_parameter_dt *dtp, const fnode *f, const char
*source, int len)
    char itoa_buf[GFC_BTOA_BUF_SIZE];
    GFC_UINTEGER_LARGEST n = 0;

+  /* Ensure we end up with a null terminated string.  */
+  memset(itoa_buf, '\0', GFC_BTOA_BUF_SIZE);
+
    if (len > (int) sizeof (GFC_UINTEGER_LARGEST))
      {
        p = btoa_big (source, itoa_buf, len, &n);
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Followup to PR91593

jerry DeLisle-3
In reply to this post by Martin Sebor-2
On 10/1/19 12:40 PM, Martin Sebor wrote:
> On 9/30/19 9:40 PM, Jerry DeLisle wrote:
>> Copying gcc list for additional thoughts on a possible bogus warning.
>>
>> On 9/29/19 9:02 AM, Jerry DeLisle wrote:

--- snip ---

>
> In case it helps, the warning is for the access:
>
>    # .MEM_68 = VDEF <.MEM_71>
>    MEM[(char *)_86] = 0;
>
> where _86 is set to
>
>    _86 = &itoa_buf + _43;
>
> and _43 has the range [136, 17179869176].  (The warning needs to
> be enhanced a bit to mention the accessed object in this case.)
> itoa_buf's DECL_SIZE_UNIT evaluates to 129.
>
> The call to btoa_big in write_b:
>
>        p = btoa_big (source, itoa_buf, len, &n);
>
> is made with len > 16.  If len > sizeof itoa_buf  / 8 then it does
> look like btoa_big would write past the end of itoa_buf because it
> writes len * 8 bytes into it.  I don't know if the function can be
> called with len that large but if not, adding this just above
> the call suppresses the warning.
>
>        if (len > sizeof itoa_buf / 8)
>      __builtin_unreachable ();
>
> Martin
>

Thanks Martin, this does help me understand why the warning. Yes, len cannot be
greater than 16. I decided to just use memset in write_b where the buffer is
created to avoid the assignment that triggers the warning. Your suggestion may
likewise work. In my comments describing the warning as bogus, by bogus, I mean
"not applicable" to the situation.

I notice many warnings flying by when building gcc/gfortran. My preference would
be to clear as many off as possible so that we can actually notice warnings when
they are applicable and not miss them among a crowd of warnings.

I don't know if this makes sense to anyone and I do appreciate your response.

Jerry