| 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 | expand |
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
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,
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
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
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.
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.)
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);
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.