| Message ID | c4d05663-57a2-40be-3fba-270239b52ee0@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628] | expand |
Hi, Please CC fortran@gcc for Fortran patches. Fortraners: Please see my add-on suggestion/.diff for 'do_simply' below and the original email at https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613019.html On 01.03.23 08:09, HAO CHEN GUI via Gcc-patches wrote: > The patch escalates the failure when Hollerith constant to real conversion > fails in native_interpret_expr. It finally reports an "Unclassifiable > statement" error. I think we should do better than this (cf. below). > The patch of pr95450 added a verification for decoding/encoding checking > in native_interpret_expr. native_interpret_expr may fail on real type > conversion and returns a NULL tree then. But upper layer calls don't handle > the failure so that an ICE is reported when the verification fails. > > IBM long double is an example. It doesn't have a unique memory presentation > for some real values. So it may not pass the verification. The new test > case shows the problem. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. I think the patch OK except for two items: > ChangeLog > 2023-03-01 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > PR target/103628 > * fortran/target-memory.cc (gfc_interpret_float): Return FAIL when > native_interpret_expr gets a NULL tree. > * fortran/arith.cc (gfc_hollerith2real): Return NULL when > gfc_interpret_float fails. > > gcc/testsuite/ > PR target/103628 > * gfortran.dg/pr103628.f90: New. ... > --- a/gcc/fortran/target-memory.cc > +++ b/gcc/fortran/target-memory.cc > @@ -417,10 +417,13 @@ gfc_interpret_float (int kind, unsigned char *buffer, size_t buffer_size, > { > gfc_set_model_kind (kind); > mpfr_init (real); > - gfc_conv_tree_to_mpfr (real, > - native_interpret_expr (gfc_get_real_type (kind), > - buffer, buffer_size)); > > + tree source = native_interpret_expr (gfc_get_real_type (kind), buffer, > + buffer_size); > + if (!source) > + return 0; > + > + gfc_conv_tree_to_mpfr (real, source); Can you move the mpfr_init before the 'gfc_conv...mfpr' to avoid pointless allocation/leaks by mpfr_init. > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/pr103628.f90 > @@ -0,0 +1,14 @@ > +! { dg-do compile { target powerpc*-*-* } } > +! { dg-options "-O2 -mabi=ibmlongdouble" } > + > +! Test to ensure that it reports an "Unclassifiable statement" error > +! instead of throwing an ICE when the memory represent of the HOLLERITH > +! string is not unique with ibm long double encoding. > ... > + real(kind = k):: b = 4h1234 > ... > +! { dg-warning "Conversion from HOLLERITH" "warning" { target powerpc*-*-* } 10 } > +! { dg-error "Unclassifiable statement" "error" { target powerpc*-*-* } 10 } I find a generic error pointing to the beginning of the line not that helpful. I did wonder about something specific, but at the end for a rather special case, a generic catch-all for 'do_simplify' seems to make sense. The callers of 'do_simplify' clearly assume that this function will always (indirectly) generate an error message if an error occurs. With my change, the result is: 10 | real(kind = k):: b = 4h1234 | 1 Error: Cannot simplify expression at (1) That's with: diff --git a/gcc/fortran/intrinsic.cc b/gcc/fortran/intrinsic.cc index 64821c84543..b60d92a0665 100644 --- a/gcc/fortran/intrinsic.cc +++ b/gcc/fortran/intrinsic.cc @@ -27,2 +27,3 @@ along with GCC; see the file COPYING3. If not see #include "intrinsic.h" +#include "diagnostic.h" /* For errorcount. */ @@ -4622,2 +4623,3 @@ do_simplify (gfc_intrinsic_sym *specific, gfc_expr *e) gfc_actual_arglist *arg; + int old_errorcount = errorcount; @@ -4710,3 +4712,7 @@ finish: if (result == &gfc_bad_expr) - return false; + { + if (errorcount == old_errorcount) + gfc_error ("Cannot simplify expression at %L", &e->where); + return false; + } Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
On Wed, Mar 01, 2023 at 10:40:15AM +0100, Tobias Burnus wrote: > > --- /dev/null > > +++ b/gcc/testsuite/gfortran.dg/pr103628.f90 > > @@ -0,0 +1,14 @@ > > +! { dg-do compile { target powerpc*-*-* } } > > +! { dg-options "-O2 -mabi=ibmlongdouble" } > > + > > +! Test to ensure that it reports an "Unclassifiable statement" error > > +! instead of throwing an ICE when the memory represent of the HOLLERITH > > +! string is not unique with ibm long double encoding. > > ... > > + real(kind = k):: b = 4h1234 This should be rejected with an error message. Hollerith was deleted from Fortran in Appendix C-1 of the Fortran 77 standard. The appearance of a Hollerith entity in an initialization expression has never be conforming. In fact, Hollerith should be hidden behind a -fallow-hollerith option and added to -std=legacy. > diff --git a/gcc/fortran/intrinsic.cc b/gcc/fortran/intrinsic.cc > index 64821c84543..b60d92a0665 100644 > --- a/gcc/fortran/intrinsic.cc > +++ b/gcc/fortran/intrinsic.cc > @@ -27,2 +27,3 @@ along with GCC; see the file COPYING3. If not see > #include "intrinsic.h" > +#include "diagnostic.h" /* For errorcount. */ > > @@ -4622,2 +4623,3 @@ do_simplify (gfc_intrinsic_sym *specific, gfc_expr *e) > gfc_actual_arglist *arg; > + int old_errorcount = errorcount; > > @@ -4710,3 +4712,7 @@ finish: > if (result == &gfc_bad_expr) > - return false; > + { > + if (errorcount == old_errorcount) > + gfc_error ("Cannot simplify expression at %L", &e->where); > + return false; > + } I'm okay with this suggestion.
On Wed, 1 Mar 2023 10:40:15 +0100 Tobias Burnus <tobias@codesourcery.com> wrote: > Hi, > > Please CC fortran@gcc for Fortran patches. > > --- a/gcc/fortran/target-memory.cc > > +++ b/gcc/fortran/target-memory.cc > > @@ -417,10 +417,13 @@ gfc_interpret_float (int kind, unsigned char *buffer, size_t buffer_size, > > { > > gfc_set_model_kind (kind); > > mpfr_init (real); > > - gfc_conv_tree_to_mpfr (real, > > - native_interpret_expr (gfc_get_real_type (kind), > > - buffer, buffer_size)); > > > > + tree source = native_interpret_expr (gfc_get_real_type (kind), buffer, s/source = native/source = native/ additionally to moving mpfr_init around and the other comments. Please send an updated, regtested patch? thanks && cheers,
On Wed, 1 Mar 2023 07:39:40 -0800 Steve Kargl via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > In fact, Hollerith should be hidden behind a -fallow-hollerith > option and added to -std=legacy. While i'd be all for that, in my mind this will block off literally all consultants and quite some scientists unless we error out with a specific hint to an option that re-enable this. So yea, but please forgivingly for the decades to come? HTH,
On Thu, Mar 02, 2023 at 01:07:32AM +0100, Bernhard Reutner-Fischer wrote: > On Wed, 1 Mar 2023 07:39:40 -0800 > Steve Kargl via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > In fact, Hollerith should be hidden behind a -fallow-hollerith > > option and added to -std=legacy. > > While i'd be all for that, in my mind this will block off literally all > consultants and quite some scientists unless we error out > with a specific hint to an option that re-enable this. > > So yea, but please forgivingly for the decades to come? > It has already been decades. It seems to me that only way to motivate people to fix their code is to nag. Hollerith is pre-F77 era code. gfortran already provide a warning. The warning should be made into an error, and yes, it can point to -fallow-hollerith. The option would downgrade the error to a warning. The only way to suppress the warning is to suppress all warnings with -w. See -fallow-invalid-boz. % cat > a.f90 program foo ! free-form source code is post-f77. real :: b = 4H1234 ! No code, in particular, legacy has print *, b ! initialization expressions. end program foo % gfcx -o z a.f90 a.f90:2:14: 2 | real :: b = 4H1234 | 1 Warning: Extension: Conversion from HOLLERITH to REAL(4) at (1) a.f90:2:18: 2 | real :: b = 4H1234 | 1 Warning: Legacy Extension: Hollerith constant at (1)
Hi Haochen, on 2023/3/1 15:09, HAO CHEN GUI wrote: > Hi, > The patch escalates the failure when Hollerith constant to real conversion > fails in native_interpret_expr. It finally reports an "Unclassifiable > statement" error. > > The patch of pr95450 added a verification for decoding/encoding checking > in native_interpret_expr. native_interpret_expr may fail on real type > conversion and returns a NULL tree then. But upper layer calls don't handle > the failure so that an ICE is reported when the verification fails. > > IBM long double is an example. It doesn't have a unique memory presentation > for some real values. So it may not pass the verification. The new test > case shows the problem. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. > > Thanks > Gui Haochen > > ChangeLog > 2023-03-01 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > PR target/103628 > * fortran/target-memory.cc (gfc_interpret_float): Return FAIL when > native_interpret_expr gets a NULL tree. > * fortran/arith.cc (gfc_hollerith2real): Return NULL when > gfc_interpret_float fails. > > gcc/testsuite/ > PR target/103628 > * gfortran.dg/pr103628.f90: New. > > > patch.diff > diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc > index c0d12cfad9d..d3d38c7eb6a 100644 > --- a/gcc/fortran/arith.cc > +++ b/gcc/fortran/arith.cc > @@ -2752,10 +2752,12 @@ gfc_hollerith2real (gfc_expr *src, int kind) > result = gfc_get_constant_expr (BT_REAL, kind, &src->where); > > hollerith2representation (result, src); > - gfc_interpret_float (kind, (unsigned char *) result->representation.string, > - result->representation.length, result->value.real); > - > - return result; > + if (gfc_interpret_float (kind, > + (unsigned char *) result->representation.string, > + result->representation.length, result->value.real)) > + return result; > + else > + return NULL; > } > > /* Convert character to real. The constant will be padded or truncated. */ > diff --git a/gcc/fortran/target-memory.cc b/gcc/fortran/target-memory.cc > index 7ce7d736629..04afc357e3c 100644 > --- a/gcc/fortran/target-memory.cc > +++ b/gcc/fortran/target-memory.cc > @@ -417,10 +417,13 @@ gfc_interpret_float (int kind, unsigned char *buffer, size_t buffer_size, > { > gfc_set_model_kind (kind); > mpfr_init (real); > - gfc_conv_tree_to_mpfr (real, > - native_interpret_expr (gfc_get_real_type (kind), > - buffer, buffer_size)); > > + tree source = native_interpret_expr (gfc_get_real_type (kind), buffer, > + buffer_size); > + if (!source) > + return 0; > + > + gfc_conv_tree_to_mpfr (real, source); > return size_float (kind); > } > > diff --git a/gcc/testsuite/gfortran.dg/pr103628.f90 b/gcc/testsuite/gfortran.dg/pr103628.f90 > new file mode 100644 > index 00000000000..e49aefc18fd > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/pr103628.f90 > @@ -0,0 +1,14 @@ > +! { dg-do compile { target powerpc*-*-* } } > +! { dg-options "-O2 -mabi=ibmlongdouble" } Since this test case is powerpc only, I think it can be moved to gcc/testsuite/gcc.target/powerpc/ppc-fortran. BR, Kewen
diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc index c0d12cfad9d..d3d38c7eb6a 100644 --- a/gcc/fortran/arith.cc +++ b/gcc/fortran/arith.cc @@ -2752,10 +2752,12 @@ gfc_hollerith2real (gfc_expr *src, int kind) result = gfc_get_constant_expr (BT_REAL, kind, &src->where); hollerith2representation (result, src); - gfc_interpret_float (kind, (unsigned char *) result->representation.string, - result->representation.length, result->value.real); - - return result; + if (gfc_interpret_float (kind, + (unsigned char *) result->representation.string, + result->representation.length, result->value.real)) + return result; + else + return NULL; } /* Convert character to real. The constant will be padded or truncated. */ diff --git a/gcc/fortran/target-memory.cc b/gcc/fortran/target-memory.cc index 7ce7d736629..04afc357e3c 100644 --- a/gcc/fortran/target-memory.cc +++ b/gcc/fortran/target-memory.cc @@ -417,10 +417,13 @@ gfc_interpret_float (int kind, unsigned char *buffer, size_t buffer_size, { gfc_set_model_kind (kind); mpfr_init (real); - gfc_conv_tree_to_mpfr (real, - native_interpret_expr (gfc_get_real_type (kind), - buffer, buffer_size)); + tree source = native_interpret_expr (gfc_get_real_type (kind), buffer, + buffer_size); + if (!source) + return 0; + + gfc_conv_tree_to_mpfr (real, source); return size_float (kind); } diff --git a/gcc/testsuite/gfortran.dg/pr103628.f90 b/gcc/testsuite/gfortran.dg/pr103628.f90 new file mode 100644 index 00000000000..e49aefc18fd --- /dev/null +++ b/gcc/testsuite/gfortran.dg/pr103628.f90 @@ -0,0 +1,14 @@ +! { dg-do compile { target powerpc*-*-* } } +! { dg-options "-O2 -mabi=ibmlongdouble" } + +! Test to ensure that it reports an "Unclassifiable statement" error +! instead of throwing an ICE when the memory represent of the HOLLERITH +! string is not unique with ibm long double encoding. + +program main + integer, parameter :: k = 16 + real(kind = k):: b = 4h1234 +end program main + +! { dg-warning "Conversion from HOLLERITH" "warning" { target powerpc*-*-* } 10 } +! { dg-error "Unclassifiable statement" "error" { target powerpc*-*-* } 10 }