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

[klausler]

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.

[PeteSteinfeld]

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

[jeanPerier]

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.

[klausler]

In D99296#2649900, @jeanPerier wrote:

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]

Rework the fix.

[klausler]

One more tweak for clarity.

[PeteSteinfeld]

All builds, tests, and looks good.

flang/runtime/descriptor.cpp

if (raw_.attribute != CFI_attribute_pointer) {
     Deallocate();
   }
 }
 
 void Descriptor::Establish(TypeCode t, std::size_t elementBytes, void *p,
     int rank, const SubscriptValue *extent, ISO::CFI_attribute_t attribute,
     bool addendum) {
   Terminator terminator{__FILE__, __LINE__};
+  // Subtle: the standard CFI_establish() function doesn't allow a zero
+  // elem_len argument in cases where elem_len is not ignored; and when it
+  // returns an error code (CFI_INVALID_ELEM_LEN in this case), it must not
+  // modify the descriptor.  That design makes sense, maybe, for actual
+  // C interoperability, but we need to work around it here.  A zero
+  // incoming element length is replaced by 4 so that it will be valid
+  // for all CHARACTER kinds.
+  std::size_t workaroundElemLen{elementBytes ? elementBytes : 4};
   RUNTIME_CHECK(terminator,
-      ISO::CFI_establish(&raw_, p, attribute, t.raw(), elementBytes, rank,
+      ISO::CFI_establish(&raw_, p, attribute, t.raw(), workaroundElemLen, rank,
           extent) == CFI_SUCCESS);
+  if (elementBytes == 0) {
+    raw_.elem_len = 0;
+    for (int j{0}; j < rank; ++j) {
+      GetDimension(j).SetByteStride(0);
+    }
+  }
   raw_.f18Addendum = addendum;
   DescriptorAddendum *a{Addendum()};
   RUNTIME_CHECK(terminator, addendum == (a != nullptr));
   if (a) {
     new (a) DescriptorAddendum{};
   }
 }