Fortran: use name of array component in runtime error message [PR30802]
Checks
Commit Message
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
Comments
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
@@ -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));
new file mode 100644
@@ -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