[patch, libgfortran] Bug 91593 - Implicit enum conversions in libgfortran/io/transfer.c

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

[patch, libgfortran] Bug 91593 - Implicit enum conversions in libgfortran/io/transfer.c

jerry DeLisle-3
Hi all,

The attached patch eliminates several warnings by adjusting which enumerator is
used in the subject offending code. I fixed this by adding an enumerator at the
end of the file_mode definition.  This then triggered a warning in several other
places for an unhandled case in the switch statements. I cleared those by
throwing in an assert (false) since it cant happen unless something really goes
wrong somehow.

Regardless, regression tested on x86_64-pc-linux-gnu.

OK for trunk? No applicable test case.

Jerry

2019-09-22  Jerry DeLisle  <[hidden email]>

        PR libfortran/91593
        * io/transfer.c (file_mode, current_mode,
        formatted_transfer_scalar_read, formatted_transfer_scalar_write,
        pre_position, next_record_r, next_record_w): Add and use
        FORMATTED_UNSPECIFIED to enumeration.

PS

While I was at it, I 'touched' all files in libgfortran/io to see what other
warnings are left,  There is another odd warning I have not sorted out yet.

../../../trunk/libgfortran/io/read.c: In function ‘read_decimal’:
../../../trunk/libgfortran/io/read.c:641:9: warning: comparison of integer
expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and
‘int’ [-Wsign-compare]
   641 |   if (w == DEFAULT_WIDTH)
       |         ^~
In function ‘btoa_big’,
     inlined from ‘write_b’ at ../../../trunk/libgfortran/io/write.c:1212:11:
../../../trunk/libgfortran/io/write.c:1051:6: warning: writing 1 byte into a
region of size 0 [-Wstringop-overflow=]
  1051 |   *q = '\0';
       |   ~~~^~~~~~

The first of these two I understand. The second one about region of size 0
puzzles me.

warnings.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, libgfortran] Bug 91593 - Implicit enum conversions in libgfortran/io/transfer.c

Tobias Burnus-5
Hi Jerry,

On 9/22/19 10:51 PM, Jerry DeLisle wrote:
> The attached patch eliminates several warnings by adjusting which
> enumerator is used in the subject offending code. I fixed this by
> adding an enumerator at the end of the file_mode definition.  This
> then triggered a warning in several other places for an unhandled case
> in the switch statements. I cleared those by throwing in an assert
> (false) since it cant happen unless something really goes wrong somehow.
>
> Regardless, regression tested on x86_64-pc-linux-gnu.
> OK for trunk? No applicable test case.

LGTM – thanks for eliminating warnings.


> PS
>
> While I was at it, I 'touched' all files in libgfortran/io to see what
> other warnings are left,  There is another odd warning I have not
> sorted out yet.
> […]
> In function ‘btoa_big’,
>     inlined from ‘write_b’ at
> ../../../trunk/libgfortran/io/write.c:1212:11:
> ../../../trunk/libgfortran/io/write.c:1051:6: warning: writing 1 byte
> into a region of size 0 [-Wstringop-overflow=]
>  1051 |   *q = '\0';
>       |   ~~~^~~~~~
>
> The first of these two I understand. The second one about region of
> size 0 puzzles me.


btoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)


   q = buffer;

       for (i = 0; i < len; i++)
           for (j = 0; j < 8; j++)
             {
               *q++ = (c & 128) ? '1' : '0';

   *q = '\0';

I think the compiler assumes that if you call "q++" (alias buffer)
"8*len" times, that the
    *q = '\0';
will write at buffer[len] – which could be one byte beyond the buffer
size. I don't quickly see whether that's the case or not, but I think
you should check whether this can happen – and if not, you may need to
add a comment, e.g. stating that the buffer is 8*len+1 bytes long or
something along that line. And if it can happen, you need to ensure that
in the future it cannot :-)

Now looking at the code,
   char itoa_buf[GFC_BTOA_BUF_SIZE];
with
    #define GFC_BTOA_BUF_SIZE (GFC_LARGEST_BUF * 8 + 1)
and GFC_LARGEST_BUF  is sizeof(real-16 real or integer-16) if available
or sizeof(real-10) or is not sizeof(largest integer).

"len" comes in as parameter to write_b/write_o/write_z and looks like
being the byte size or kind. Hence, it seems to be fine. Maybe adding a
comment plus an assert(len < GFC_BTOA_BUF_SIZE) would make sense? With
the assert, the warning would return with "NDEBUG" set (cf. assert man
page) but otherwise, it should be fine.

Down side is that w/o NDEBUG, one adds one additional condition ("if"
branch) to the code - even if it is regarded as unlikely (assert code
moved to the end of the function) - and adds some strings + printf call
as well (via the assert). But at the end, one probably doesn't need to
worry about this too much.

Cheers,

Tobias

Reply | Threaded
Open this post in threaded view
|

Re: [patch, libgfortran] Bug 91593 - Implicit enum conversions in libgfortran/io/transfer.c

jerry DeLisle-3
In reply to this post by jerry DeLisle-3
On 9/23/19 8:12 PM, Jerry DeLisle wrote:

> On 9/23/19 8:52 AM, Bernhard Reutner-Fischer wrote:
>> On 22 September 2019 22:51:46 CEST, Jerry DeLisle <[hidden email]> wrote:
>>> Hi all,
>>>
>>> The attached patch eliminates several warnings by adjusting which
>>> enumerator is
>>> used in the subject offending code. I fixed this by adding an
>>> enumerator at the
>>> end of the file_mode definition.  This then triggered a warning in
>>> several other
>>> places for an unhandled case in the switch statements. I cleared those
>>> by
>>> throwing in an assert (false) since it cant happen unless something
>>> really goes
>>> wrong somehow.
>>>
>> I'm curious why you assert (false) instead of the usual gcc_unreachable ()?
>>
>> Thanks,
>>
>
> Because I forgot all about gcc_unreachable.  I will give it a try.
>
> Jerry
gcc_unreachable is only defined in the gfortran frontend and not the runtime.
Therefore, I added a define to io.h which invokes __builtin_unreachable and does
not use fancy_abort. I don't think we need anything fancy.

If no objections, I will commit the attached updated patch with a new ChangeLog.

Regression tested ok.

Regards,

Jerry

warnings2.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, libgfortran] Bug 91593 - Implicit enum conversions in libgfortran/io/transfer.c

jerry DeLisle-3
On 9/28/19 1:21 PM, Janne Blomqvist wrote:

> On Fri, Sep 27, 2019 at 8:14 PM Jerry DeLisle <[hidden email]> wrote:
>>
>> On 9/23/19 8:12 PM, Jerry DeLisle wrote:
>>> On 9/23/19 8:52 AM, Bernhard Reutner-Fischer wrote:
>>>> On 22 September 2019 22:51:46 CEST, Jerry DeLisle <[hidden email]> wrote:
>>>>> Hi all,
>>>>>
>>>>> The attached patch eliminates several warnings by adjusting which
>>>>> enumerator is
>>>>> used in the subject offending code. I fixed this by adding an
>>>>> enumerator at the
>>>>> end of the file_mode definition.  This then triggered a warning in
>>>>> several other
>>>>> places for an unhandled case in the switch statements. I cleared those
>>>>> by
>>>>> throwing in an assert (false) since it cant happen unless something
>>>>> really goes
>>>>> wrong somehow.
>>>>>
>>>> I'm curious why you assert (false) instead of the usual gcc_unreachable ()?
>>>>
>>>> Thanks,
>>>>
>>>
>>> Because I forgot all about gcc_unreachable.  I will give it a try.
>>>
>>> Jerry
>>
>> gcc_unreachable is only defined in the gfortran frontend and not the runtime.
>> Therefore, I added a define to io.h which invokes __builtin_unreachable and does
>> not use fancy_abort. I don't think we need anything fancy.
>>
>> If no objections, I will commit the attached updated patch with a new ChangeLog.
>
> Just a minor nit, why bother with the #define, why not just use
> __builtin_unreachable directly?
>

I thought to use a define in case later we want to make it fancier similar to
the front end and only have to edit one place in the future. Otherwise no
difference to me.

Jerry