[PATCHv2, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
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. Compared to last version, this version moves the mpfr_init after NULL tree test and fixes the format problem according to Tobias's advice. Thanks a lot. Gui Haochen Thanks ChangeLog 2023-03-01 Haochen Gui 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..0c47aa6b842 100644 --- a/gcc/fortran/target-memory.cc +++ b/gcc/fortran/target-memory.cc @@ -416,11 +416,14 @@ gfc_interpret_float (int kind, unsigned char *buffer, size_t buffer_size, mpfr_t real) { 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; + + mpfr_init (real); + 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 000..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 }
Re: [PATCHv2, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi, The patch passed regression test on Power linux platforms. Sorry for missing the information. Gui Haochen 在 2023/3/3 17:12, HAO CHEN GUI via Gcc-patches 写道: > 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. > > Compared to last version, this version moves the mpfr_init after NULL tree > test and fixes the format problem according to Tobias's advice. Thanks a lot. > > Gui Haochen > Thanks > > ChangeLog > 2023-03-01 Haochen Gui > > 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..0c47aa6b842 100644 > --- a/gcc/fortran/target-memory.cc > +++ b/gcc/fortran/target-memory.cc > @@ -416,11 +416,14 @@ gfc_interpret_float (int kind, unsigned char *buffer, > size_t buffer_size, >mpfr_t real) > { >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; > + > + mpfr_init (real); > + 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 000..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 }
Re: [PATCHv2, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi Haochen, thanks for fixing 'gcc/fortran/target-memory.cc'. But could you also include the 'gcc/fortran/intrinsic.cc' change proposed in https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613030.html (and acknowledged by Steve)? This will replace "Unclassifiable statement" by "Cannot simplify expression" which is a bit more helpful. (After doing so, at least the dg-error in your testcase needs to be updated; I assume that it won't affect other testcases, but that needs to be checked. – And you should update the comment in the testcase as well - and in text you will put in the commit log, if applicable.) Additionally, Kewen suggested: Since this test case is powerpc only, I think it can be moved to gcc/testsuite/gcc.target/powerpc/ppc-fortran. Which sounds reasonable. Otherwise it looks good to me. On 03.03.23 10:12, 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. Compared to last version, this version moves the mpfr_init after NULL tree test and fixes the format problem according to Tobias's advice. Thanks a lot. (The format issue was actually spotted by Bernhard.) Thanks for working on this! Tobias PS: One can also discuss Steve's suggestion about deprecating Holleriths / guarding the support behind some flag. But I think that's unrelated to this bug fix patch and should be discussed/done separately. ChangeLog 2023-03-01 Haochen Gui 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..0c47aa6b842 100644 --- a/gcc/fortran/target-memory.cc +++ b/gcc/fortran/target-memory.cc @@ -416,11 +416,14 @@ gfc_interpret_float (int kind, unsigned char *buffer, size_t buffer_size, mpfr_t real) { 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; + + mpfr_init (real); + 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 000..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 } - 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
Re: [PATCHv2, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi Tobias, 在 2023/3/3 17:29, Tobias Burnus 写道: > But could you also include the 'gcc/fortran/intrinsic.cc' change > proposed in > https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613030.html (and > acknowledged by Steve)? Sure, I will merge it into the patch and do the regression test. Additionally, Kewen suggested: >> Since this test case is powerpc only, I think it can be moved to >> gcc/testsuite/gcc.target/powerpc/ppc-fortran. > > Which sounds reasonable. Test cases under gcc.target are tested by check-gcc-c. It greps "warning" and "error" (C style, lower case) from the output while check-gcc-fortran greps "Warning" and "Error" (upper case). As the test case needs to check the "Warning" and "Error" messages. I have to put it in gfortran.dg directory. What's your opinion? Gui Haochen Thanks
Re: [PATCHv2, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi Haochen, On 03.03.23 10:56, HAO CHEN GUI via Gcc-patches wrote: Sure, I will merge it into the patch and do the regression test. Thanks :-) Additionally, Kewen suggested: Since this test case is powerpc only, I think it can be moved to gcc/testsuite/gcc.target/powerpc/ppc-fortran/ppc-fortran.expgcc.target/powerpc/ppc-fortran. Which sounds reasonable. Test cases under gcc.target are tested by check-gcc-c. It greps "warning" and "error" (C style, lower case) from the output while check-gcc-fortran greps "Warning" and "Error" (upper case). As the test case needs to check the "Warning" and "Error" messages. I have to put it in gfortran.dg directory. What's your opinion? Thanks for digging and giving a reason. Looks as if at some point, adapting gcc/testsuite/gcc.target/powerpc/ppc-fortran/ppc-fortran.exp to handle this as well could make sense. But placing it - as you did - under gcc/testsuite/gfortran.dg is fine and surely the simpler solution. Thus, leave it as it. Thanks, Tobias Gui Haochen Thanks - 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
Re: [PATCH] Fortran: fix CLASS attribute handling [PR106856]
On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote: > - if (attr->class_ok) > -/* Class container has already been built. */ > + /* Class container has already been built with same name. */ > + if (attr->class_ok > + && ts->u.derived->components->attr.dimension >= attr->dimension > + && ts->u.derived->components->attr.codimension >= attr->codimension > + && ts->u.derived->components->attr.class_pointer >= attr->pointer > + && ts->u.derived->components->attr.allocatable >= attr->allocatable) I suppose I'm a bit confused here. dimension, codimension, pointer and allocatable are 1-bit bitfields in the attr struct. These can have the values 0 and 1, so the above conditionals are always true. The rest of the patch looks reasonable. If Tobias has no objections or comments, it's ok to commit once the above is explained. -- Steve
Re: [PATCH] Fortran: fix CLASS attribute handling [PR106856]
Hi Steve, Am 03.03.23 um 20:57 schrieb Steve Kargl via Gcc-patches: On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote: - if (attr->class_ok) -/* Class container has already been built. */ + /* Class container has already been built with same name. */ + if (attr->class_ok + && ts->u.derived->components->attr.dimension >= attr->dimension + && ts->u.derived->components->attr.codimension >= attr->codimension + && ts->u.derived->components->attr.class_pointer >= attr->pointer + && ts->u.derived->components->attr.allocatable >= attr->allocatable) I suppose I'm a bit confused here. dimension, codimension, pointer and allocatable are 1-bit bitfields in the attr struct. These can have the values 0 and 1, so the above conditionals are always true. thanks for looking into it. The above part is from the original draft. I thought I could generate testcases that allow to exercise this part, and found a new case that is not covered by the patch and still ICEs: subroutine bar (x) class(*):: x dimension :: x(:) allocatable :: x end :-( We'll need to revisit the logic... The rest of the patch looks reasonable. If Tobias has no objections or comments, it's ok to commit once the above is explained. Thanks, Harald
Re: [PATCH] Fortran: fix CLASS attribute handling [PR106856]
Hello, Le 03/03/2023 à 20:57, Steve Kargl via Fortran a écrit : On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote: - if (attr->class_ok) -/* Class container has already been built. */ + /* Class container has already been built with same name. */ + if (attr->class_ok + && ts->u.derived->components->attr.dimension >= attr->dimension + && ts->u.derived->components->attr.codimension >= attr->codimension + && ts->u.derived->components->attr.class_pointer >= attr->pointer + && ts->u.derived->components->attr.allocatable >= attr->allocatable) I suppose I'm a bit confused here. dimension, codimension, pointer and allocatable are 1-bit bitfields in the attr struct. These can have the values 0 and 1, so the above conditionals are always true. as I understand it, they aren't if attr has attributes that aren't already set in the class container's first component. a >= b == !(a < b) and if a and b are boolean-valued, a < b == !a && b. Admittedly, I haven't tested the logic like Harald has. The rest of the patch looks reasonable. If Tobias has no objections or comments, it's ok to commit once the above is explained. I have two comments, one about the handling of as and sym->as, which I quite don't understand, but I haven't had time to write something about it. The other is about this: + else if (sym->ts.type == BT_CLASS + && sym->ts.u.derived->attr.is_class + && sym->old_symbol && sym->old_symbol->as == CLASS_DATA (sym)->as) +sym->old_symbol->as = NULL; Can this be avoided? The management of symbol versions should not need any manual change. In principle, either the modified symbols are committed, or (in case of error) the previous symbols are restored, but there shouldn't be any need for restoring a modified previous symbol. I guess it's a matter of memory management, because gfc_build_class_symbol copies the AS pointer to the class descriptor, but I think using gfc_copy_array_spec there or adding the condition above to free_old_symbol would be preferable.
Re: [PATCH] Fortran: fix CLASS attribute handling [PR106856]
On Fri, Mar 03, 2023 at 10:24:07PM +0100, Mikael Morin wrote: > Hello, > > Le 03/03/2023 à 20:57, Steve Kargl via Fortran a écrit : > > On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote: > > > - if (attr->class_ok) > > > -/* Class container has already been built. */ > > > + /* Class container has already been built with same name. */ > > > + if (attr->class_ok > > > + && ts->u.derived->components->attr.dimension >= attr->dimension > > > + && ts->u.derived->components->attr.codimension >= attr->codimension > > > + && ts->u.derived->components->attr.class_pointer >= attr->pointer > > > + && ts->u.derived->components->attr.allocatable >= > > > attr->allocatable) > > > > I suppose I'm a bit confused here. dimension, codimension, > > pointer and allocatable are 1-bit bitfields in the attr > > struct. These can have the values 0 and 1, so the above > > conditionals are always true. > > > as I understand it, they aren't if attr has attributes that aren't already > set in the class container's first component. > a >= b == !(a < b) and if a and b are boolean-valued, a < b == !a && b. > Admittedly, I haven't tested the logic like Harald has. > Mikael, thanks for smacking me with the clue stick. I had to do a quick test to see the trees. % cc -o z a.c && ./z a.i = 0, b.i = 0, a.i >= b.i = 1 a.i = 1, b.i = 0, a.i >= b.i = 1 a.i = 1, b.i = 1, a.i >= b.i = 1 a.i = 0, b.i = 1, a.i >= b.i = 0 I was overlooking the last case. So, the above is an all or nothing test. -- steve
Re: [PATCH][stage1] Remove conditionals around free()
On 2 March 2023 02:23:10 CET, Jerry D wrote: >On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: >> On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via >> Fortran wrote: >>> libgfortran/caf/single.c |6 ++ >>> libgfortran/io/async.c |6 ++ >>> libgfortran/io/format.c |3 +-- >>> libgfortran/io/transfer.c|6 ++ >>> libgfortran/io/unix.c|3 +-- >> >> The Fortran ones are OK. >> > >The only question I have: Is free posix compliant on all platforms? > >For example ming64 or mac? It seems sometimes we run into things like this >once in a while. I think we have the -liberty to cater even for non compliant systems either way, if you please excuse the pun. That's not an excuse on POSIX systems, imho. > >Otherwise I have no issue at all. It is a lot cleaner. > >Jerry
Re: [PATCH][stage1] Remove conditionals around free()
> On 3 Mar 2023, at 23:11, Bernhard Reutner-Fischer via Fortran > wrote: > > On 2 March 2023 02:23:10 CET, Jerry D wrote: >> On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: >>> On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via >>> Fortran wrote: libgfortran/caf/single.c |6 ++ libgfortran/io/async.c |6 ++ libgfortran/io/format.c |3 +-- libgfortran/io/transfer.c|6 ++ libgfortran/io/unix.c|3 +-- >>> >>> The Fortran ones are OK. >>> >> >> The only question I have: Is free posix compliant on all platforms? >> >> For example ming64 or mac? OSX / macOS are [certified] Posix compliant - but to unix03 (and might be missing features declared as optional at that revision, or features from later Posix versions). In the case of free() man says: "The free() function deallocates the memory allocation pointed to by ptr. If ptr is a NULL pointer, no operation is performed.” Iain >> It seems sometimes we run into things like this once in a while. > > I think we have the -liberty to cater even for non compliant systems either > way, if you please excuse the pun. That's not an excuse on POSIX systems, > imho. > >> >> Otherwise I have no issue at all. It is a lot cleaner. >> >> Jerry
Re: [PATCH][stage1] Remove conditionals around free()
On 3/3/23 3:32 PM, Iain Sandoe wrote: On 3 Mar 2023, at 23:11, Bernhard Reutner-Fischer via Fortran wrote: On 2 March 2023 02:23:10 CET, Jerry D wrote: On 3/1/23 4:07 PM, Steve Kargl via Fortran wrote: On Wed, Mar 01, 2023 at 10:28:56PM +0100, Bernhard Reutner-Fischer via Fortran wrote: libgfortran/caf/single.c |6 ++ libgfortran/io/async.c |6 ++ libgfortran/io/format.c |3 +-- libgfortran/io/transfer.c|6 ++ libgfortran/io/unix.c|3 +-- The Fortran ones are OK. The only question I have: Is free posix compliant on all platforms? For example ming64 or mac? OSX / macOS are [certified] Posix compliant - but to unix03 (and might be missing features declared as optional at that revision, or features from later Posix versions). In the case of free() man says: "The free() function deallocates the memory allocation pointed to by ptr. If ptr is a NULL pointer, no operation is performed.” Iain It seems sometimes we run into things like this once in a while. I think we have the -liberty to cater even for non compliant systems either way, if you please excuse the pun. That's not an excuse on POSIX systems, imho. I am certainly not a C++ expert but it seems to me this all begs for automatic finalization where one would not have to invoke free at all. I suspect the gfortran frontend is not designed for such things. Otherwise I have no issue at all. It is a lot cleaner. Jerry