| Message ID | trinity-a9064cd8-162e-4fc3-a51d-b5be2c45ced8-1706132367162@3c-app-gmx-bap39 |
|---|---|
| State | New |
| Headers | show |
| Series | Fortran: use name of array component in runtime error message [PR30802] | expand |
Le 24/01/2024 à 22:39, Harald Anlauf a écrit : > Dear all, > > this patch is actually only a followup fix to generate the proper name > of an array reference in derived-type components for the runtime error > message generated for the bounds-checking code. Without the proper > part ref, not only a user may get confused: I was, too... > > The testcase is compile-only, as it is only important to check the > strings used in the error messages. > > Regtested on x86_64-pc-linux-gnu. OK for mainline? > > Thanks, > Harald > Hello, the change proper looks good, and is an improvement. But I'm a little concerned by the production of references like in the test x1%vv%z which could be confusing and is strictly speaking invalid fortran (multiple non-scalar components). Did you consider generating x1%vv(?,?)%zz or x1%vv(...)%z or similar? There is another nit about the test, which has dg-output and dg-shouldfail despite being only a compile-time test. Otherwise looks good. Mikael
Hi Mikael, Am 28.01.24 um 12:39 schrieb Mikael Morin: > Le 24/01/2024 à 22:39, Harald Anlauf a écrit : >> Dear all, >> >> this patch is actually only a followup fix to generate the proper name >> of an array reference in derived-type components for the runtime error >> message generated for the bounds-checking code. Without the proper >> part ref, not only a user may get confused: I was, too... >> >> The testcase is compile-only, as it is only important to check the >> strings used in the error messages. >> >> Regtested on x86_64-pc-linux-gnu. OK for mainline? >> >> Thanks, >> Harald >> > Hello, > > the change proper looks good, and is an improvement. But I'm a little > concerned by the production of references like in the test x1%vv%z which > could be confusing and is strictly speaking invalid fortran (multiple > non-scalar components). Did you consider generating x1%vv(?,?)%zz or > x1%vv(...)%z or similar? yes, that seems very reasonable, given that this is what NAG does. We also have spurious %_data in some error messages that I'll try to get rid off. > There is another nit about the test, which has dg-output and > dg-shouldfail despite being only a compile-time test. I'll remove that. With gcc-13 the runtime check would trigger in the wrong line but the failure of the check is dealt with by another testcase in gcc-14. > Otherwise looks good. > > Mikael I'll come up with a revised patch. Thanks for the feedback so far! Harald
On Sun, Jan 28, 2024 at 08:56:24PM +0100, Harald Anlauf wrote: > > Am 28.01.24 um 12:39 schrieb Mikael Morin: > > Le 24/01/2024 à 22:39, Harald Anlauf a écrit : > > > Dear all, > > > > > > this patch is actually only a followup fix to generate the proper name > > > of an array reference in derived-type components for the runtime error > > > message generated for the bounds-checking code. Without the proper > > > part ref, not only a user may get confused: I was, too... > > > > > > The testcase is compile-only, as it is only important to check the > > > strings used in the error messages. > > > > > > Regtested on x86_64-pc-linux-gnu. OK for mainline? > > > > > > > the change proper looks good, and is an improvement. But I'm a little > > concerned by the production of references like in the test x1%vv%z which > > could be confusing and is strictly speaking invalid fortran (multiple > > non-scalar components). Did you consider generating x1%vv(?,?)%zz or > > x1%vv(...)%z or similar? > > yes, that seems very reasonable, given that this is what NAG does. > > We also have spurious %_data in some error messages that I'll try > to get rid off. > I haven't looked at the patch, but sometimes (if not always) things like _data are marked with attr.artificial. You might see if this will help with suppressing spurious messages.
On 28 January 2024 22:43:37 CET, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: >On Sun, Jan 28, 2024 at 08:56:24PM +0100, Harald Anlauf wrote: >> >> Am 28.01.24 um 12:39 schrieb Mikael Morin: >> > Le 24/01/2024 à 22:39, Harald Anlauf a écrit : >> > > Dear all, >> > > >> > > this patch is actually only a followup fix to generate the proper name >> > > of an array reference in derived-type components for the runtime error >> > > message generated for the bounds-checking code. Without the proper >> > > part ref, not only a user may get confused: I was, too... >> > > >> > > The testcase is compile-only, as it is only important to check the >> > > strings used in the error messages. >> > > >> > > Regtested on x86_64-pc-linux-gnu. OK for mainline? >> > > >> > >> > the change proper looks good, and is an improvement. But I'm a little >> > concerned by the production of references like in the test x1%vv%z which >> > could be confusing and is strictly speaking invalid fortran (multiple >> > non-scalar components). Did you consider generating x1%vv(?,?)%zz or >> > x1%vv(...)%z or similar? >> >> yes, that seems very reasonable, given that this is what NAG does. >> >> We also have spurious %_data in some error messages that I'll try >> to get rid off. >> > >I haven't looked at the patch, but sometimes (if not always) things >like _data are marked with attr.artificial. You might see if this >will help with suppressing spurious messages. > Reminds me of https://inbox.sourceware.org/fortran/20211114231748.376086cd@nbbrfq/ Maybe thats missing, i did not apply that yet, did i? HTH
Am 28.01.24 um 22:43 schrieb Steve Kargl: > On Sun, Jan 28, 2024 at 08:56:24PM +0100, Harald Anlauf wrote: >> >> Am 28.01.24 um 12:39 schrieb Mikael Morin: >>> Le 24/01/2024 à 22:39, Harald Anlauf a écrit : >>>> Dear all, >>>> >>>> this patch is actually only a followup fix to generate the proper name >>>> of an array reference in derived-type components for the runtime error >>>> message generated for the bounds-checking code. Without the proper >>>> part ref, not only a user may get confused: I was, too... >>>> >>>> The testcase is compile-only, as it is only important to check the >>>> strings used in the error messages. >>>> >>>> Regtested on x86_64-pc-linux-gnu. OK for mainline? >>>> >>> >>> the change proper looks good, and is an improvement. But I'm a little >>> concerned by the production of references like in the test x1%vv%z which >>> could be confusing and is strictly speaking invalid fortran (multiple >>> non-scalar components). Did you consider generating x1%vv(?,?)%zz or >>> x1%vv(...)%z or similar? >> >> yes, that seems very reasonable, given that this is what NAG does. >> >> We also have spurious %_data in some error messages that I'll try >> to get rid off. >> > > I haven't looked at the patch, but sometimes (if not always) things > like _data are marked with attr.artificial. You might see if this > will help with suppressing spurious messages. I was talking about the generated format strings of runtime error messages. program p implicit none type t real :: zzz(10) = 42 end type t class(t), allocatable :: xx(:) integer :: j j = 0 allocate (t :: xx(1)) print *, xx(1)% zzz(j) end This is generating the following error at runtime since at least gcc-7: Fortran runtime error: Index '0' of dimension 1 of array 'xx%_data%zzz' below lower bound of 1 I believe you were recalling bogus warnings at compile time. There are no warnings here, and there shouldn't.
Am 29.01.24 um 18:25 schrieb Harald Anlauf: > I was talking about the generated format strings of runtime error > messages. > > program p > implicit none > type t > real :: zzz(10) = 42 > end type t > class(t), allocatable :: xx(:) > integer :: j > j = 0 > allocate (t :: xx(1)) > print *, xx(1)% zzz(j) > end > > This is generating the following error at runtime since at least gcc-7: > > Fortran runtime error: Index '0' of dimension 1 of array 'xx%_data%zzz' > below lower bound of 1 Of course this is easily suppressed by: diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc index 1e0d698a949..fa0e00a28a6 100644 --- a/gcc/fortran/trans-array.cc +++ b/gcc/fortran/trans-array.cc @@ -4054,7 +4054,8 @@ gfc_conv_array_ref (gfc_se * se, gfc_array_ref * ar, gfc_expr *expr, { if (ref->type == REF_ARRAY && &ref->u.ar == ar) break; - if (ref->type == REF_COMPONENT) + if (ref->type == REF_COMPONENT + && strcmp (ref->u.c.component->name, "_data") != 0) { strcat (var_name, "%%"); strcat (var_name, ref->u.c.component->name); I have been contemplating the generation the full chain of references as suggested by Mikael and supported by NAG. The main issue is: how do I easily generate that call? gfc_trans_runtime_check is a vararg function, but what I would rather have is a function that takes either a (chained?) list of trees or an array of trees holding the (co-)indices of the reference. Is there an example, or a recommendation which variant to prefer? Thanks, Harald
Le 29/01/2024 à 21:50, Harald Anlauf a écrit : > Am 29.01.24 um 18:25 schrieb Harald Anlauf: >> I was talking about the generated format strings of runtime error >> messages. >> >> program p >> implicit none >> type t >> real :: zzz(10) = 42 >> end type t >> class(t), allocatable :: xx(:) >> integer :: j >> j = 0 >> allocate (t :: xx(1)) >> print *, xx(1)% zzz(j) >> end >> >> This is generating the following error at runtime since at least gcc-7: >> >> Fortran runtime error: Index '0' of dimension 1 of array 'xx%_data%zzz' >> below lower bound of 1 > > Of course this is easily suppressed by: > > diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc > index 1e0d698a949..fa0e00a28a6 100644 > --- a/gcc/fortran/trans-array.cc > +++ b/gcc/fortran/trans-array.cc > @@ -4054,7 +4054,8 @@ gfc_conv_array_ref (gfc_se * se, gfc_array_ref * > ar, gfc_expr *expr, > { > if (ref->type == REF_ARRAY && &ref->u.ar == ar) > break; > - if (ref->type == REF_COMPONENT) > + if (ref->type == REF_COMPONENT > + && strcmp (ref->u.c.component->name, "_data") != 0) > { > strcat (var_name, "%%"); > strcat (var_name, ref->u.c.component->name); > > > I have been contemplating the generation the full chain of references as > suggested by Mikael and supported by NAG. To be clear, my suggestion was to have the question marks (or dashes, dots, stars, whatever) literally in the array reference, without the actual values of the array indexes. Another (easier) way to clarify the data reference would be rephrasing the message so that the array part is separate from the scalar part, like so (there are too many 'of', but I lack inspiration): Index '0' of dimension 1 of component 'zz' of element from 'x1%vv' below lower bound of 1 > The main issue is: how do I easily generate that call? > gfc_trans_runtime_check is a vararg function, but what I would rather > have is a function that takes either a (chained?) list of trees or > an array of trees holding the (co-)indices of the reference. > > Is there an example, or a recommendation which variant to prefer? > None that I know. For a scalarized expression, the values are present (among others) in the gfc_loopinfo::ss linked list, maybe just use that? In any case, I agree it would be nice to have, but it would probably be a non-negligible amount of new error-prone code; I would rather not attempt this during the stage4 stabilization phase as we are currently.
Le 30/01/2024 à 11:38, Mikael Morin a écrit : > > Another (easier) way to clarify the data reference would be rephrasing > the message so that the array part is separate from the scalar part, > like so (there are too many 'of', but I lack inspiration): > Index '0' of dimension 1 of component 'zz' of element from 'x1%vv' > below lower bound of 1 > This has the same number of 'of' but sounds better maybe: Out of bounds accessing component 'zz' of element from 'x1%yy': index '0' of dimension 1 below lower bound of 1
From 43c0185764ec878576ef2255d9df24fbb1961af4 Mon Sep 17 00:00:00 2001 From: Harald Anlauf <anlauf@gmx.de> Date: Wed, 24 Jan 2024 22:28:31 +0100 Subject: [PATCH] Fortran: use name of array component in runtime error message [PR30802] gcc/fortran/ChangeLog: PR fortran/30802 * trans-array.cc (trans_array_bound_check): Derive name of component for use in runtime error message. gcc/testsuite/ChangeLog: PR fortran/30802 * gfortran.dg/bounds_check_fail_8.f90: New test. --- gcc/fortran/trans-array.cc | 34 ++++++++++++++++++ .../gfortran.dg/bounds_check_fail_8.f90 | 35 +++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 gcc/testsuite/gfortran.dg/bounds_check_fail_8.f90 diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc index 878a92aff18..f6ddce2d023 100644 --- a/gcc/fortran/trans-array.cc +++ b/gcc/fortran/trans-array.cc @@ -3497,6 +3497,10 @@ trans_array_bound_check (gfc_se * se, gfc_ss *ss, tree index, int n, tree descriptor; char *msg; const char * name = NULL; + char *var_name = NULL; + gfc_expr *expr; + gfc_ref *ref; + size_t len; if (!(gfc_option.rtcheck & GFC_RTCHECK_BOUNDS)) return index; @@ -3509,6 +3513,36 @@ trans_array_bound_check (gfc_se * se, gfc_ss *ss, tree index, int n, name = ss->info->expr->symtree->n.sym->name; gcc_assert (name != NULL); + /* When we have a component ref, compile name for the array section. + Note that there can only be one part ref. */ + expr = ss->info->expr; + if (expr->ref && !compname) + { + len = strlen (name) + 1; + + /* Find a safe length. */ + for (ref = expr->ref; ref; ref = ref->next) + if (ref->type == REF_COMPONENT) + len += 2 + strlen (ref->u.c.component->name); + + var_name = XALLOCAVEC (char, len); + strcpy (var_name, name); + + for (ref = expr->ref; ref; ref = ref->next) + { + /* Append component name. */ + if (ref->type == REF_COMPONENT) + { + strcat (var_name, "%%"); + strcat (var_name, ref->u.c.component->name); + continue; + } + if (ref->type == REF_ARRAY && ref->u.ar.type == AR_SECTION) + break; + } + name = var_name; + } + if (VAR_P (descriptor)) name = IDENTIFIER_POINTER (DECL_NAME (descriptor)); diff --git a/gcc/testsuite/gfortran.dg/bounds_check_fail_8.f90 b/gcc/testsuite/gfortran.dg/bounds_check_fail_8.f90 new file mode 100644 index 00000000000..3397e953ba6 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/bounds_check_fail_8.f90 @@ -0,0 +1,35 @@ +! { dg-do compile } +! { dg-additional-options "-fcheck=bounds -g -fdump-tree-original" } +! { dg-output "At line 22 .*" } +! { dg-shouldfail "dimension 3 of array 'uu%z' outside of expected range" } +! +! PR fortran/30802 - improve bounds-checking for array references +! +! Checking the proper component references is the most important part here. + +program test + implicit none + integer :: k = 0 + type t + real, dimension(10,20,30) :: z = 23 + end type t + type u + type(t) :: vv(4,5) + end type u + type(t) :: uu, ww(1) + type(u) :: x1, x2, y1(1), y2(1) + + print *, uu % z(1,k,:) ! runtime check only for dimension 2 of z + print *, ww(1)% z(1,:,k) ! runtime check only for dimension 3 of z + print *, x1 % vv(2,3)% z(1,:,k) ! runtime check only for dimension 3 of z + print *, x2 % vv(k,:)% z(1,2,3) ! runtime check only for dimension 1 of vv + print *, y1(1)% vv(2,3)% z(1,:,k) ! runtime check only for dimension 3 of z + print *, y2(1)% vv(:,k)% z(1,2,3) ! runtime check only for dimension 2 of vv +end program test + +! { dg-final { scan-tree-dump-times "'uu%%z.' outside of expected range" 2 "original" } } +! { dg-final { scan-tree-dump-times "'ww%%z.' outside of expected range" 2 "original" } } +! { dg-final { scan-tree-dump-times "'x1%%vv%%z.' outside of expected range" 2 "original" } } +! { dg-final { scan-tree-dump-times "'x2%%vv.' outside of expected range" 2 "original" } } +! { dg-final { scan-tree-dump-times "'y1%%vv%%z.' outside of expected range" 2 "original" } } +! { dg-final { scan-tree-dump-times "'y2%%vv.' outside of expected range" 2 "original" } } -- 2.35.3