Fortran: fix crash with bounds check writing array section [PR117791]

  Dear all,

the attached patch fixes a wrong-code issue with bounds-checking
enabled when doing I/O of an array section and an index is either
an expression or a function result.  The problem does not occur
without bounds-checking.

When looking at the original testcase, the function occuring in
the affected index was evaluated twice, once with wrong arguments.

The most simple solution appears to fall back to scalarization
with bounds-checking enabled.  If someone has a quick idea to
handle this better, please speak up!

Regtested on x86_64-pc-linux-gnu.  OK for mainline?

This seems to be a 14/15 regression, so a backport is advisable.

Thanks,
Harald
  

On 11/27/24 12:31 PM, Harald Anlauf wrote:
> Dear all,
> 
> the attached patch fixes a wrong-code issue with bounds-checking
> enabled when doing I/O of an array section and an index is either
> an expression or a function result.  The problem does not occur
> without bounds-checking.
> 
> When looking at the original testcase, the function occuring in
> the affected index was evaluated twice, once with wrong arguments.
> 
> The most simple solution appears to fall back to scalarization
> with bounds-checking enabled.  If someone has a quick idea to
> handle this better, please speak up!
> 
> Regtested on x86_64-pc-linux-gnu.  OK for mainline?
> 
> This seems to be a 14/15 regression, so a backport is advisable.
> 
> Thanks,
> Harald
> 

The patch looks OK to me.

I wonder if this fall back to the scalarizer should be done everywhere 
if a a user has specified bounds checking, what is the point of 
optimizing array references?

If the code works in 13 maybe we need to isolate to what broke it and 
intervene at that place.

Also go ahead with back porting if no other ideas pop up.  I just fear 
we are covering up something else.

Jerry
  

Am 27.11.24 um 21:56 schrieb Jerry D:
> On 11/27/24 12:31 PM, Harald Anlauf wrote:
>> Dear all,
>>
>> the attached patch fixes a wrong-code issue with bounds-checking
>> enabled when doing I/O of an array section and an index is either
>> an expression or a function result.  The problem does not occur
>> without bounds-checking.
>>
>> When looking at the original testcase, the function occuring in
>> the affected index was evaluated twice, once with wrong arguments.
>>
>> The most simple solution appears to fall back to scalarization
>> with bounds-checking enabled.  If someone has a quick idea to
>> handle this better, please speak up!
>>
>> Regtested on x86_64-pc-linux-gnu.  OK for mainline?
>>
>> This seems to be a 14/15 regression, so a backport is advisable.
>>
>> Thanks,
>> Harald
>>
> 
> The patch looks OK to me.
> 
> I wonder if this fall back to the scalarizer should be done everywhere 
> if a a user has specified bounds checking, what is the point of 
> optimizing array references?

If an array reference is of the type A(:,f()), there is no need to
do bounds-checking for the first array index (we don't, so OK),
and we also could pass the array slice to a library function that
handles the section in one go, without generating a loop with calls.
Scalarization is then sort of a missed-optimization.

The problem is that the second argument is somehow evaluated twice
with bounds-checking, but only with the I/O optimization.  I did not
see such an issue when assigning A(:,f()) to a temporary rank-1 array
and passing that array to the write().  It did create the right bounds
check, and called f() correctly just once.

Instead of creating a temporary, just passing to the scalarizer was
the simpler solution.  Maybe Paul has an idea to solve this in a
better way.

> If the code works in 13 maybe we need to isolate to what broke it and 
> intervene at that place.

Looking at the tree-dump, no bounds-check was generated in 13.
I did some work to extend bounds-checking during 14-development,
and the testcase may have just uncovered a latent issue?

(And we sometimes evaluate functions way too often, see e.g. pr114021,
so there's no lack of possibly related issues...)

> Also go ahead with back porting if no other ideas pop up.  I just fear 
> we are covering up something else.

I'll wait until tomorrow to see if Paul intervenes.  Otherwise I will
proceed and push.

Thanks for the review and discussion!

Harald

> Jerry
> 
>
  

Hi Harald and Jerry,

I cannot see why the segfault is occurring of course:
          _gfortran_transfer_character_write (&dt_parm.9, &"line 4:"[1]{lb:
1 sz: 1}, 7);
          {
            struct array01_integer(kind=4) parm.10;
            integer(kind=8) D.4841;
            struct array01_integer(kind=4) parm.11;
            integer(kind=8) D.4848;
            struct array01_integer(kind=4) parm.12;

            D.4841 = (integer(kind=8)) sort_2 ((integer(kind=4)[0:] *)
parm.10.data);  // parm10 not set.

I am going to see if stopping the call to
'add_check_section_in_array_bounds' when an inner loop is evaluating an
argument for a function call in the outer loop does the trick.

That said, while your patch seems to be a bit hacky, it does fix the
problem in a sensible way. I just worry about potential corner cases since
it is the call to gfc_conv_expr_descriptor that causes the problem.

Cheers

Paul


On Wed, 27 Nov 2024 at 21:35, Harald Anlauf <anlauf@gmx.de> wrote:

> Am 27.11.24 um 21:56 schrieb Jerry D:
> > On 11/27/24 12:31 PM, Harald Anlauf wrote:
> >> Dear all,
> >>
> >> the attached patch fixes a wrong-code issue with bounds-checking
> >> enabled when doing I/O of an array section and an index is either
> >> an expression or a function result.  The problem does not occur
> >> without bounds-checking.
> >>
> >> When looking at the original testcase, the function occuring in
> >> the affected index was evaluated twice, once with wrong arguments.
> >>
> >> The most simple solution appears to fall back to scalarization
> >> with bounds-checking enabled.  If someone has a quick idea to
> >> handle this better, please speak up!
> >>
> >> Regtested on x86_64-pc-linux-gnu.  OK for mainline?
> >>
> >> This seems to be a 14/15 regression, so a backport is advisable.
> >>
> >> Thanks,
> >> Harald
> >>
> >
> > The patch looks OK to me.
> >
> > I wonder if this fall back to the scalarizer should be done everywhere
> > if a a user has specified bounds checking, what is the point of
> > optimizing array references?
>
> If an array reference is of the type A(:,f()), there is no need to
> do bounds-checking for the first array index (we don't, so OK),
> and we also could pass the array slice to a library function that
> handles the section in one go, without generating a loop with calls.
> Scalarization is then sort of a missed-optimization.
>
> The problem is that the second argument is somehow evaluated twice
> with bounds-checking, but only with the I/O optimization.  I did not
> see such an issue when assigning A(:,f()) to a temporary rank-1 array
> and passing that array to the write().  It did create the right bounds
> check, and called f() correctly just once.
>
> Instead of creating a temporary, just passing to the scalarizer was
> the simpler solution.  Maybe Paul has an idea to solve this in a
> better way.
>
> > If the code works in 13 maybe we need to isolate to what broke it and
> > intervene at that place.
>
> Looking at the tree-dump, no bounds-check was generated in 13.
> I did some work to extend bounds-checking during 14-development,
> and the testcase may have just uncovered a latent issue?
>
> (And we sometimes evaluate functions way too often, see e.g. pr114021,
> so there's no lack of possibly related issues...)
>
> > Also go ahead with back porting if no other ideas pop up.  I just fear
> > we are covering up something else.
>
> I'll wait until tomorrow to see if Paul intervenes.  Otherwise I will
> proceed and push.
>
> Thanks for the review and discussion!
>
> Harald
>
> > Jerry
> >
> >
>
>
  

Hi Harald,


>
>> I'll wait until tomorrow to see if Paul intervenes.  Otherwise I will
>> proceed and push.
>>
>
I succeeded in breaking things even more! Please proceed and push.

Thanks

Paul
  

Hi Paul,

Am 28.11.24 um 13:55 schrieb Paul Richard Thomas:
> Hi Harald,
> 
> 
>>
>>> I'll wait until tomorrow to see if Paul intervenes.  Otherwise I will
>>> proceed and push.
>>>
>>
> I succeeded in breaking things even more! Please proceed and push.

I'm sort of glad you failed, too!  ;-)

Pushed as r15-5766 .

Note that the patch does not touch the following, probably more common
cases, where one index is just an (ordinary or implied-do) variable:

   write(*,*) 'line 4:',(array(:,j), j=sort_2(i(1:2)),sort_2(i(1:2)))
   write(*,*) 'line 5:',(array(:,j), j=1,int (2-sort_2(i(1:2))))

These still get optimized and use _gfortran_transfer_array_write .

Thanks,
Harald

> Thanks
> 
> Paul
>
  

../../gcc/fortran/trans-io.cc: In function 'tree_node* gfc_trans_transfer(gfc_code*)':
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_UNKNOWN' not handled in switch [-Werror=switch]
 2662 |                 switch (ref->u.ar.start[n]->expr_type)
      |                        ^
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_CONSTANT' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_VARIABLE' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_SUBSTRING' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_STRUCTURE' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_ARRAY' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_NULL' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_COMPCALL' not handled in switch [-Werror=switch]
../../gcc/fortran/trans-io.cc:2662:24: error: enumeration value 'EXPR_PPC' not handled in switch [-Werror=switch]
cc1plus: all warnings being treated as errors
make[3]: *** [Makefile:1203: fortran/trans-io.o] Error 1
  

H Harald, hi Paul,

Harald Anlauf wrote:
> Pushed as r15-5766 .

This caused a build fail; see also: https://gcc.gnu.org/PR117843

It looks as if a 'default: break;' is missing.

…/gcc/fortran/trans-io.cc: In function 'tree_node* 
gfc_trans_transfer(gfc_code*)':
…/gcc/fortran/trans-io.cc:2662:24: error: enumeration value 
'EXPR_UNKNOWN' not handled in switch [-Werror=switch]
  2662 |                 switch (ref->u.ar.start[n]->expr_type)
       |                        ^

Tobias