Page MenuHomePhabricator

[flang] Fix runtime crash on TRIM('')
ClosedPublic

Authored by klausler on Mar 24 2021, 1:37 PM.

Details

Summary

The implementation of CFI_establish() incorrectly rejects an
element length of zero, causing a crash when a runtime intrinsic
function tries to return an empty CHARACTER value. Fix.

Diff Detail

Event Timeline

klausler created this revision.Mar 24 2021, 1:37 PM
klausler requested review of this revision.Mar 24 2021, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 1:37 PM
PeteSteinfeld requested changes to this revision.Mar 24 2021, 7:21 PM

This builds OK, but when I run tests, I see failures in the ISO-Fortran-binding test:

FAIL: flang-OldUnit :: Evaluate/ISO-Fortran-binding.test (608 of 738)
******************** TEST 'flang-OldUnit :: Evaluate/ISO-Fortran-binding.test' FAILED ********************
/local/home/psteinfeld/main/trim/flang/unittests/Evaluate/ISO-Fortran-binding.cpp:151: FAIL: expectedRetCode == 0x3, not 0x0
/local/home/psteinfeld/main/trim/flang/unittests/Evaluate/ISO-Fortran-binding.cpp:151: FAIL: expectedRetCode == 0x3, not 0x0
/local/home/psteinfeld/main/trim/flang/unittests/Evaluate/ISO-Fortran-binding.cpp:151: FAIL: expectedRetCode == 0x3, not 0x0
/local/home/psteinfeld/main/trim/flang/unittests/Evaluate/ISO-Fortran-binding.cpp:151: FAIL: expectedRetCode == 0x3, not 0x0
This revision now requires changes to proceed.Mar 24 2021, 7:21 PM

Regarding CFI_establish, the Fortran standard says in 18.5.5.5 about elem_len :

elem_len: If type is equal to CFI_type_struct, CFI_type_other, or a Fortran character type code, elem_len shall be greater than zero and equal to the storage size in bytes of an element of the object. Otherwise, elem_len will be ignored.

So it look like the implementation/unittests are following the standard. But it is weird that it should be forbidden to establish zero length characters in the C interoperability context. I see no harm in that, so I am OK with your change.

However, do zero sized struct/ CFI_type_other make sense ? I guess in the C interoperability context, empty structs are not thing since they are technically illegal in C, but outside of the C interoperability context, I do not think empty derived types are forbidden.

Regarding CFI_establish, the Fortran standard says in 18.5.5.5 about elem_len :

elem_len: If type is equal to CFI_type_struct, CFI_type_other, or a Fortran character type code, elem_len shall be greater than zero and equal to the storage size in bytes of an element of the object. Otherwise, elem_len will be ignored.

So it look like the implementation/unittests are following the standard. But it is weird that it should be forbidden to establish zero length characters in the C interoperability context. I see no harm in that, so I am OK with your change.

However, do zero sized struct/ CFI_type_other make sense ? I guess in the C interoperability context, empty structs are not thing since they are technically illegal in C, but outside of the C interoperability context, I do not think empty derived types are forbidden.

I'm going to rework things so that CFI_establish will adhere to the standard and then change the failing CHECK that's calling it to allow for a bad element length.

klausler updated this revision to Diff 333359.Mar 25 2021, 11:07 AM

Rework the fix.

klausler updated this revision to Diff 333361.Mar 25 2021, 11:10 AM

One more tweak for clarity.

PeteSteinfeld accepted this revision.Mar 25 2021, 12:35 PM

All builds, tests, and looks good.

This revision is now accepted and ready to land.Mar 25 2021, 12:35 PM
This revision was automatically updated to reflect the committed changes.