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

Message ID 7fb106f7-31f3-9d4c-3610-270c3162164d@charter.net
State New
Headers show
Series
  • [libgfortran] Bug 91593 - Implicit enum conversions in libgfortran/io/transfer.c
Related show

Commit Message

Jerry Sept. 22, 2019, 8:51 p.m.
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  <jvdelisle@gcc.gnu.org>

	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.

Comments

Tobias Burnus Sept. 23, 2019, 9:04 a.m. | #1
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
Bernhard Reutner-Fischer Sept. 23, 2019, 3:52 p.m. | #2
On 22 September 2019 22:51:46 CEST, Jerry DeLisle <jvdelisle@charter.net> 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,
Jerry Sept. 24, 2019, 3:12 a.m. | #3
On 9/23/19 8:52 AM, Bernhard Reutner-Fischer wrote:
> On 22 September 2019 22:51:46 CEST, Jerry DeLisle <jvdelisle@charter.net> 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
Jerry Sept. 27, 2019, 5:14 p.m. | #4
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 <jvdelisle@charter.net> 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
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index f5e63797ba1..bcd6dde9a5b 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -32,6 +32,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #include <gthr.h>
 
+#define gcc_unreachable() __builtin_unreachable ()
 
 /* POSIX 2008 specifies that the extended locale stuff is found in
    locale.h, but some systems have them in xlocale.h.  */
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index c43360f6332..4c5e210ce5a 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -193,7 +193,8 @@ static const st_option async_opt[] = {
 
 typedef enum
 { FORMATTED_SEQUENTIAL, UNFORMATTED_SEQUENTIAL,
-  FORMATTED_DIRECT, UNFORMATTED_DIRECT, FORMATTED_STREAM, UNFORMATTED_STREAM
+  FORMATTED_DIRECT, UNFORMATTED_DIRECT, FORMATTED_STREAM,
+  UNFORMATTED_STREAM, FORMATTED_UNSPECIFIED
 }
 file_mode;
 
@@ -203,7 +204,7 @@ current_mode (st_parameter_dt *dtp)
 {
   file_mode m;
 
-  m = FORM_UNSPECIFIED;
+  m = FORMATTED_UNSPECIFIED;
 
   if (dtp->u.p.current_unit->flags.access == ACCESS_DIRECT)
     {
@@ -1727,17 +1728,17 @@ formatted_transfer_scalar_read (st_parameter_dt *dtp, bt type, void *p, int kind
 
 	case FMT_S:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_S;
+	  dtp->u.p.sign_status = SIGN_PROCDEFINED;
 	  break;
 
 	case FMT_SS:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SS;
+	  dtp->u.p.sign_status = SIGN_SUPPRESS;
 	  break;
 
 	case FMT_SP:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SP;
+	  dtp->u.p.sign_status = SIGN_PLUS;
 	  break;
 
 	case FMT_BN:
@@ -2186,17 +2187,17 @@ formatted_transfer_scalar_write (st_parameter_dt *dtp, bt type, void *p, int kin
 
 	case FMT_S:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_S;
+	  dtp->u.p.sign_status = SIGN_PROCDEFINED;
 	  break;
 
 	case FMT_SS:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SS;
+	  dtp->u.p.sign_status = SIGN_SUPPRESS;
 	  break;
 
 	case FMT_SP:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SP;
+	  dtp->u.p.sign_status = SIGN_PLUS;
 	  break;
 
 	case FMT_BN:
@@ -2766,6 +2767,8 @@ pre_position (st_parameter_dt *dtp)
     case UNFORMATTED_DIRECT:
       dtp->u.p.current_unit->bytes_left = dtp->u.p.current_unit->recl;
       break;
+    case FORMATTED_UNSPECIFIED:
+      gcc_unreachable ();
     }
 
   dtp->u.p.current_unit->current_record = 1;
@@ -3637,6 +3640,8 @@ next_record_r (st_parameter_dt *dtp, int done)
 	  while (p != '\n');
 	}
       break;
+    case FORMATTED_UNSPECIFIED:
+      gcc_unreachable ();
     }
 }
 
@@ -4002,6 +4007,8 @@ next_record_w (st_parameter_dt *dtp, int done)
 	}
 
       break;
+    case FORMATTED_UNSPECIFIED:
+      gcc_unreachable ();
 
     io_error:
       generate_error (&dtp->common, LIBERROR_OS, NULL);
Steve Kargl Sept. 28, 2019, 4:37 p.m. | #5
On Fri, Sep 27, 2019 at 10:14:34AM -0700, Jerry DeLisle wrote:
> 

> If no objections, I will commit the attached updated patch

> with a new ChangeLog.


No objection.

-- 
Steve
Janne Blomqvist Sept. 28, 2019, 8:21 p.m. | #6
On Fri, Sep 27, 2019 at 8:14 PM Jerry DeLisle <jvdelisle@charter.net> 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 <jvdelisle@charter.net> 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 think for the frontend there's the argument that it might be
compiled with a non-GCC compiler which might not support
__builtin_unreachable. But libgfortran is always compiled with the
corresponding gcc frontend, so this doesn't apply there.)

-- 
Janne Blomqvist

Patch

diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index c43360f6332..3ba72a47d3e 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -32,6 +32,7 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include "format.h"
 #include "unix.h"
 #include "async.h"
+#include <assert.h>
 #include <string.h>
 #include <errno.h>
 
@@ -193,7 +194,8 @@  static const st_option async_opt[] = {
 
 typedef enum
 { FORMATTED_SEQUENTIAL, UNFORMATTED_SEQUENTIAL,
-  FORMATTED_DIRECT, UNFORMATTED_DIRECT, FORMATTED_STREAM, UNFORMATTED_STREAM
+  FORMATTED_DIRECT, UNFORMATTED_DIRECT, FORMATTED_STREAM,
+  UNFORMATTED_STREAM, FORMATTED_UNSPECIFIED
 }
 file_mode;
 
@@ -203,7 +205,7 @@  current_mode (st_parameter_dt *dtp)
 {
   file_mode m;
 
-  m = FORM_UNSPECIFIED;
+  m = FORMATTED_UNSPECIFIED;
 
   if (dtp->u.p.current_unit->flags.access == ACCESS_DIRECT)
     {
@@ -1727,17 +1729,17 @@  formatted_transfer_scalar_read (st_parameter_dt *dtp, bt type, void *p, int kind
 
 	case FMT_S:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_S;
+	  dtp->u.p.sign_status = SIGN_PROCDEFINED;
 	  break;
 
 	case FMT_SS:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SS;
+	  dtp->u.p.sign_status = SIGN_SUPPRESS;
 	  break;
 
 	case FMT_SP:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SP;
+	  dtp->u.p.sign_status = SIGN_PLUS;
 	  break;
 
 	case FMT_BN:
@@ -2186,17 +2188,17 @@  formatted_transfer_scalar_write (st_parameter_dt *dtp, bt type, void *p, int kin
 
 	case FMT_S:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_S;
+	  dtp->u.p.sign_status = SIGN_PROCDEFINED;
 	  break;
 
 	case FMT_SS:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SS;
+	  dtp->u.p.sign_status = SIGN_SUPPRESS;
 	  break;
 
 	case FMT_SP:
 	  consume_data_flag = 0;
-	  dtp->u.p.sign_status = SIGN_SP;
+	  dtp->u.p.sign_status = SIGN_PLUS;
 	  break;
 
 	case FMT_BN:
@@ -2766,6 +2768,8 @@  pre_position (st_parameter_dt *dtp)
     case UNFORMATTED_DIRECT:
       dtp->u.p.current_unit->bytes_left = dtp->u.p.current_unit->recl;
       break;
+    case FORMATTED_UNSPECIFIED:
+      assert (false); /* Should never happen.  */
     }
 
   dtp->u.p.current_unit->current_record = 1;
@@ -3637,6 +3641,8 @@  next_record_r (st_parameter_dt *dtp, int done)
 	  while (p != '\n');
 	}
       break;
+    case FORMATTED_UNSPECIFIED:
+      assert (false); /* Should never happen.  */
     }
 }
 
@@ -4002,6 +4008,8 @@  next_record_w (st_parameter_dt *dtp, int done)
 	}
 
       break;
+    case FORMATTED_UNSPECIFIED:
+      assert (false); /* Should never happen.  */
 
     io_error:
       generate_error (&dtp->common, LIBERROR_OS, NULL);