[flang] Regression with Fujitsu test 0727_0008

[eugeneepshteyn]

Regression happens with -O0 (which is default):

$ flang 0727_0008.f90 -O0 && ./a.out
Segmentation fault (core dumped)

Passes with -O1 and higher.

Compiling with -g and running valgrind:

$ flang 0727_0008.f90 -O0 -g && ./a.out
Segmentation fault (core dumped)

$ valgrind ./a.out
==3413688== Memcheck, a memory error detector
==3413688== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==3413688== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==3413688== Command: ./a.out
==3413688== 
==3413688== Invalid read of size 2
==3413688==    at 0x10ABAC: test01_ (0727_0008.f90:30)
==3413688==    by 0x10A54C: _QQmain (0727_0008.f90:1)
==3413688==    by 0x10CC11: main (0727_0008.f90:94)
==3413688==  Address 0x1 is not stack'd, malloc'd or (recently) free'd
==3413688== 
==3413688== 
==3413688== Process terminating with default action of signal 11 (SIGSEGV)
==3413688==  Access not within mapped region at address 0x1
==3413688==    at 0x10ABAC: test01_ (0727_0008.f90:30)
==3413688==    by 0x10A54C: _QQmain (0727_0008.f90:1)
==3413688==    by 0x10CC11: main (0727_0008.f90:94)
==3413688==  If you believe this happened as a result of a stack
==3413688==  overflow in your program's main thread (unlikely but
==3413688==  possible), you can try to increase the size of the
==3413688==  main thread stack using the --main-stacksize= flag.
==3413688==  The main thread stack size used in this run was 8388608.
==3413688== 
==3413688== HEAP SUMMARY:
==3413688==     in use at exit: 0 bytes in 0 blocks
==3413688==   total heap usage: 2 allocs, 2 frees, 48 bytes allocated
==3413688== 
==3413688== All heap blocks were freed -- no leaks are possible
==3413688== 
==3413688== For lists of detected and suppressed errors, rerun with: -s
==3413688== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

From valgrind report, line 30 does this:
if (any(ib.ne.(/'12345678','12345678','12345678','12345678'/))) write(6,*) "NG"
From my experiments, this seems to be related to creating temporary array of strings to be used with any.

Commit range:
Last good commit: 727e9f5
First known bad commit: bf51a4d

In my workspace I eventually reverted a103b9b and regression went away.

@jeanPerier , since this was your commit, assigning it to you.

[llvmbot]

@llvm/issue-subscribers-flang-ir

Author: Eugene Epshteyn (eugeneepshteyn)

Regression happens with `-O0` (which is default): ``` $ flang 0727_0008.f90 -O0 && ./a.out Segmentation fault (core dumped) ``` Passes with `-O1` and higher.

Compiling with -g and running valgrind:

$ flang 0727_0008.f90 -O0 -g && ./a.out
Segmentation fault (core dumped)

$ valgrind ./a.out
==3413688== Memcheck, a memory error detector
==3413688== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==3413688== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==3413688== Command: ./a.out
==3413688== 
==3413688== Invalid read of size 2
==3413688==    at 0x10ABAC: test01_ (0727_0008.f90:30)
==3413688==    by 0x10A54C: _QQmain (0727_0008.f90:1)
==3413688==    by 0x10CC11: main (0727_0008.f90:94)
==3413688==  Address 0x1 is not stack'd, malloc'd or (recently) free'd
==3413688== 
==3413688== 
==3413688== Process terminating with default action of signal 11 (SIGSEGV)
==3413688==  Access not within mapped region at address 0x1
==3413688==    at 0x10ABAC: test01_ (0727_0008.f90:30)
==3413688==    by 0x10A54C: _QQmain (0727_0008.f90:1)
==3413688==    by 0x10CC11: main (0727_0008.f90:94)
==3413688==  If you believe this happened as a result of a stack
==3413688==  overflow in your program's main thread (unlikely but
==3413688==  possible), you can try to increase the size of the
==3413688==  main thread stack using the --main-stacksize= flag.
==3413688==  The main thread stack size used in this run was 8388608.
==3413688== 
==3413688== HEAP SUMMARY:
==3413688==     in use at exit: 0 bytes in 0 blocks
==3413688==   total heap usage: 2 allocs, 2 frees, 48 bytes allocated
==3413688== 
==3413688== All heap blocks were freed -- no leaks are possible
==3413688== 
==3413688== For lists of detected and suppressed errors, rerun with: -s
==3413688== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

From valgrind report, line 30 does this:
if (any(ib.ne.(/'12345678','12345678','12345678','12345678'/))) write(6,*) "NG"
From my experiments, this seems to be specific to creating temporary array of strings to be used with any.

Commit range:
Last good commit: 727e9f5
First known bad commit: bf51a4d

I eventually reverted a103b9b and regression went away.

@jeanPerier , since this was your commit, assigning it to you.

[jeanPerier]

Thanks @eugeneepshteyn for identifying the failure. The regression is indeed related to a103b9b, but the root cause is that the test is illegal Fortran.

The specification expression of ifunc02 result lengths violates the Fortran 2023 constraint C15121:

In the specification-expr that specifies a type parameter value of the result of an elemental function, an
object designator with a dummy argument of the function as the base object shall appear only as the
subject of a specification inquiry (10.1.11), and that specification inquiry shall not depend on a property
that is deferred.

While ifunc02 dummy i appears in character(len=len(i//'a')) ifunc02. The valid equivalent would be character(len=len(i)+len('a')) ifunc02.

Lowering is dumb and generate the concatenation code to evaluate len(i//'a')). At O2 and above, optimizations are clever enough to prune that, but not at O0. The difference with before and after my patch is that before my patch, the specification expression evaluation for elemental happened with the first element address, while now it is happening with the address of 0x00000001 because there is not good element address to pick in case of zero sized array.

In this specific case, it is not really required to evaluate i//'a' to compute the length, so the compiler could work around it, but in general, I think the fix is to better enforce C15121 and reject this test.

Side Improvement (which is not the correct fix IMHO, but that we should still do for other contexts):

As a side observation, I think we should slightly improve len() evaluation. Currently it is using the "old" intrinsic lowering which causes materialization of the arguments while this is not needed with HLFIR. That is, currently for len(i//'a')), lowering generates:

   ...
    %59 = hlfir.concat %55#0, %57#0 len %58 : (!fir.boxchar<1>, !fir.ref<!fir.char<1>>, index) -> !hlfir.expr<!fir.char<1,?>>
    %60:3 = hlfir.associate %59 typeparams %58 {adapt.valuebyref} : (!hlfir.expr<!fir.char<1,?>>, index) -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>, i1)

And then retrieve length from the hlfir.associate that materialized the expression in memory. Since hlfir.associate has a memory allocation effect, MLIR dead code elimination that we always run does not remove it and also leaves the useless hlfir.concat.

If we were to intercept len in Fortran::lower::lowerHlfirIntrinsic, we could directly use hlfir::genCharLength to get the length from the argument (hlfir.concat) and skip the hlfir.associate generation. Then, MLIR CSE pass for instance will get rid of the hlfir.concat (and the bad test would still work at O0). This can be tested by removing the hlfir.associate/hlfir.end_associate from the IR obtained with -emit-hlfir on 0727_0008.f90 and running fir-opt -cse.

[eugeneepshteyn]

Assigning to myself to work on Jean's suggestions.

[klausler]

@jeanPerier in the Fujitsu test 0727_0008, the LEN property of the dummy argument I is assumed, not deferred, so the program doesn't seem to violate C15121.

[jeanPerier]

@jeanPerier in the Fujitsu test 0727_0008, the LEN property of the dummy argument I is assumed, not deferred, so the program doesn't seem to violate C15121.

Maybe I overinterpreted the requirement shall appear only as the subject of a specification inquiry . Wouldn't this make it illegal for i to appear as a concatenation expression operand, even when this sub-expression itself is in a specification inquiry?

In this specific context, the concatenation expression can be simplified away, so I agree that the compiler only needs the length in the end, and that lowering needs to be improved in general for len lowering.

I see that C15121 is otherwise enforced for problematic cases like character(len=len(repeat('a', index(i, 'a')))) ifunc02, so I am OK with just going with the lowering change and support this.