[flang] Fix runtime ICE with maxloc and scalar result

[mleair]

Summary

The following legal Fortran causes a runtime ICE in certain cases. Also, valgrind reports use of an unitialized memory reference (UMR). Steve's list of F95 compliance tests exposed this problem.

  real :: a(5)
  a = 1.1
  a(3) = 2.2
  i = maxloc(a,dim=1)
  print*, i
end

The resulting ICE:

fatal Fortran runtime error(/local/home/mleair/fir-dev/f18-llvm-project/flang/runtime/reduction-templates.h:203): Internal error: RUNTIME_CHECK(at[0] == 1) failed at /local/home/mleair/fir-dev/f18-llvm-project/flang/runtime/reduction-templates.h(203)

The ICE occurs because a scalar result descriptor has 1 element and a rank == 0. The runtime calls GetLowerBounds() which does nothing with rank == 0. This can lead to an ICE and/or UMR.

Since this occurs with only a handful of intrinsics, I propose adding a check for this case where result.GetLowerBounds(at) is called instead of changing GetLowerBounds(). We may not want to make an assumption with rank == 0 in other cases. But I'll let the code reviewers weigh in on whether we should just modify GetLowerBounds() to work with rank == 0 and number of elements == 1.

This change is done for the maxloc, maxval, minloc, minval, findloc, count, parity, all, any intrinsics. I also added scalar cases to the unit tests for each of these intrinsics as well. Let me know if there are other intrinsics to consider with this change.

[klausler]

flang/runtime/reduction-templates.h

209

INTERNAL_CHECK(result.rank() == 0 || at[0] == 1);

[PeteSteinfeld]

flang/runtime/reduction.cpp

272

It would be good to clean up these clang-tidy warnings about not having braces around single statement "if-then" blocks here and at line 327 below. It looks like everywhere else in the file such statements have braces around them.

[PeteSteinfeld]

Other than my previous comments, all builds, tests, and looks good.

[mleair]

flang/runtime/reduction-templates.h

209

Hi Peter,

I thought about that change, but I think "at" is still referenced after this check. For example:

// No MASK= or scalar MASK=.TRUE.
for (auto n{result.Elements()}; n-- > 0; result.IncrementSubscripts(at)) {

  accumulator.Reinitialize();
  ReduceDimToScalar<CppType, ACCUMULATOR>(
      x, dim - 1, at, result.Element<CppType>(at), accumulator);
}

It looks like ReduceDimToScalar() still references "at". If so, then "at" should be initialized, correct? I believe this is where the UMR comes from. When at[0] is initialized to 1, the UMR goes away.

[mleair]

flang/runtime/reduction-templates.h

209

To clarify, I think there might be a second UMR with with ReduceDimToScalar(); not just with the INTERNAL_CHECK macro.

[klausler]

flang/runtime/reduction-templates.h

209

I doubt it. When rank == 0, the subscript values should not be used by Descriptor::SubscriptsToByteOffset() (and therefore not by Descriptor::Element) or by ReduceDimToScalar. How do you know that ReduceDimToScalar() is referencing uninitialized memory?

[mleair]

flang/runtime/reduction-templates.h

209

Sorry. I was mistaken. I thought I still saw a valgrind bug when I tried the rank guard with the INTERNAL_CHECK macro, But when I tried it just now, it does not occur. I'll check the other cases too.

[mleair]

Simplify the bug fix by changing the assert condition in the applicable INTERNAL_CHECK usages.

[PeteSteinfeld]

Thanks, Mark. All looks good.

[awarzynski]

Mark, could you check flang-x86_64-windows? The failure seems to be caused by this change (I only took a brief look - can check again tomorrow).

[mleair]

Yes. I saw the failure and I think I have a fix for this. It's out for review now.

-Mark

flang/runtime/reduction-templates.h

 template <typename ACCUMULATOR, TypeCategory CAT, int KIND>
 inline void PartialReduction(Descriptor &result, const Descriptor &x, int dim,
     const Descriptor *mask, Terminator &terminator, const char *intrinsic,
     ACCUMULATOR &accumulator) {
   CreatePartialReductionResult(
       result, x, dim, terminator, intrinsic, TypeCode{CAT, KIND});
   SubscriptValue at[maxRank];
   result.GetLowerBounds(at);
-  INTERNAL_CHECK(at[0] == 1);
+  INTERNAL_CHECK(result.rank() == 0 || at[0] == 1);
   using CppType = CppTypeFor<CAT, KIND>;
   if (mask) {
     CheckConformability(x, *mask, terminator, intrinsic, "ARRAY", "MASK");
     SubscriptValue maskAt[maxRank]; // contents unused
     if (mask->rank() > 0) {
       for (auto n{result.Elements()}; n-- > 0; result.IncrementSubscripts(at)) {
         accumulator.Reinitialize();
         ReduceDimMaskToScalar<CppType, ACCUMULATOR>(
             x, dim - 1, at, *mask, result.Element<CppType>(at), accumulator);
       }
       return;
     } else if (!IsLogicalElementTrue(*mask, maskAt)) {
       // scalar MASK=.FALSE.
       accumulator.Reinitialize();
       for (auto n{result.Elements()}; n-- > 0; result.IncrementSubscripts(at)) {
         accumulator.GetResult(result.Element<CppType>(at));
       }

flang/runtime/reduction.cpp

template <LogicalReduction REDUCTION> struct LogicalReduceHelper {
   template <int KIND> struct Functor {
     void operator()(Descriptor &result, const Descriptor &x, int dim,
         Terminator &terminator, const char *intrinsic) const {
       // Standard requires result to have same LOGICAL kind as argument.
       CreatePartialReductionResult(
           result, x, dim, terminator, intrinsic, x.type());
       SubscriptValue at[maxRank];
       result.GetLowerBounds(at);
-      INTERNAL_CHECK(at[0] == 1);
+      INTERNAL_CHECK(result.rank() == 0 || at[0] == 1);
       using CppType = CppTypeFor<TypeCategory::Logical, KIND>;
       for (auto n{result.Elements()}; n-- > 0; result.IncrementSubscripts(at)) {
         *result.Element<CppType>(at) =
             ReduceLogicalDimToScalar<LogicalAccumulator<REDUCTION>>(
                 x, dim - 1, at);
       }
     }
   };
 };
 

 
 template <int KIND> struct CountDimension {
   void operator()(Descriptor &result, const Descriptor &x, int dim,
       Terminator &terminator) const {
     CreatePartialReductionResult(result, x, dim, terminator, "COUNT",
         TypeCode{TypeCategory::Integer, KIND});
     SubscriptValue at[maxRank];
     result.GetLowerBounds(at);
-    INTERNAL_CHECK(at[0] == 1);
+    INTERNAL_CHECK(result.rank() == 0 || at[0] == 1);
     using CppType = CppTypeFor<TypeCategory::Integer, KIND>;
     for (auto n{result.Elements()}; n-- > 0; result.IncrementSubscripts(at)) {
       *result.Element<CppType>(at) =
           ReduceLogicalDimToScalar<CountAccumulator>(x, dim - 1, at);
     }
   }
 };
 

flang/unittests/RuntimeGTest/Reduction.cpp

TEST(Reductions, DoubleMaxMinNorm2) {
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(5), 1); // -5
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(6), 2); // 18
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(7), 1); // -7
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(8), 0);
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(9), 2); // -21
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(10), 2); // 22
   EXPECT_EQ(*loc.ZeroBasedIndexedElement<std::int16_t>(11), 2); // 22
   loc.Destroy();
+  // Test scalar result for MaxlocDim, MinlocDim, MaxvalDim, MinvalDim.
+  // A scalar result occurs when you have a rank 1 array and dim == 1.
+  std::vector<int> shape1{24};
+  auto array1{MakeArray<TypeCategory::Real, 8>(shape1, rawData)};
+  StaticDescriptor<0, true> statDesc0;
+  Descriptor &scalarResult{statDesc0.descriptor()};
+  RTNAME(MaxlocDim)
+  (scalarResult, *array1, /*KIND=*/2, /*DIM=*/1, __FILE__, __LINE__,
+      /*MASK=*/nullptr, /*BACK=*/false);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<std::int16_t>(0), 23);
+  scalarResult.Destroy();
+  RTNAME(MinlocDim)
+  (scalarResult, *array1, /*KIND=*/2, /*DIM=*/1, __FILE__, __LINE__,
+      /*MASK=*/nullptr, /*BACK=*/true);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<std::int16_t>(0), 22);
+  scalarResult.Destroy();
+  RTNAME(MaxvalDim)
+  (scalarResult, *array1, /*DIM=*/1, __FILE__, __LINE__, /*MASK=*/nullptr);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<double>(0), 22.0);
+  scalarResult.Destroy();
+  RTNAME(MinvalDim)
+  (scalarResult, *array1, /*DIM=*/1, __FILE__, __LINE__, /*MASK=*/nullptr);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<double>(0), -21.0);
+  scalarResult.Destroy();
 }
 
 TEST(Reductions, Character) {
   std::vector<int> shape{2, 3};
   auto array{MakeArray<TypeCategory::Character, 1>(shape,
       std::vector<std::string>{"abc", "def", "ghi", "jkl", "mno", "abc"}, 3)};
   StaticDescriptor<1, true> statDesc[2];
   Descriptor &res{statDesc[0].descriptor()};
TEST(Reductions, Logical) {
   RTNAME(AllDim)(res, *array, /*DIM=*/2, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Logical, 4}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(0), 0);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(1), 0);
   res.Destroy();
+  // Test scalar result for AllDim.
+  // A scalar result occurs when you have a rank 1 array.
+  std::vector<int> shape1{4};
+  auto array1{MakeArray<TypeCategory::Logical, 4>(
+      shape1, std::vector<std::int32_t>{false, false, true, true})};
+  StaticDescriptor<0, true> statDesc0;
+  Descriptor &scalarResult{statDesc0.descriptor()};
+  RTNAME(AllDim)(scalarResult, *array1, /*DIM=*/1, __FILE__, __LINE__);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<std::int64_t>(0), 0);
+  scalarResult.Destroy();
   RTNAME(AnyDim)(res, *array, /*DIM=*/1, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Logical, 4}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(0), 0);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(1), 1);
   res.Destroy();
   RTNAME(AnyDim)(res, *array, /*DIM=*/2, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Logical, 4}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(0), 1);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(1), 1);
   res.Destroy();
+  // Test scalar result for AnyDim.
+  // A scalar result occurs when you have a rank 1 array.
+  RTNAME(AnyDim)(scalarResult, *array1, /*DIM=*/1, __FILE__, __LINE__);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<std::int64_t>(0), 1);
+  scalarResult.Destroy();
   RTNAME(ParityDim)(res, *array, /*DIM=*/1, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Logical, 4}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(0), 0);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(1), 0);
   res.Destroy();
   RTNAME(ParityDim)(res, *array, /*DIM=*/2, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Logical, 4}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(0), 1);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(1), 1);
   res.Destroy();
+  // Test scalar result for ParityDim.
+  // A scalar result occurs when you have a rank 1 array.
+  RTNAME(ParityDim)(scalarResult, *array1, /*DIM=*/1, __FILE__, __LINE__);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<std::int32_t>(0), 0);
+  scalarResult.Destroy();
   RTNAME(CountDim)(res, *array, /*DIM=*/1, /*KIND=*/4, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Integer, 4}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(0), 0);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int32_t>(1), 2);
   res.Destroy();
   RTNAME(CountDim)(res, *array, /*DIM=*/2, /*KIND=*/8, __FILE__, __LINE__);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Integer, 8}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).Extent(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int64_t>(0), 1);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<std::int64_t>(1), 1);
   res.Destroy();
+  // Test scalar result for CountDim.
+  // A scalar result occurs when you have a rank 1 array and dim == 1.
+  RTNAME(CountDim)
+  (scalarResult, *array1, /*DIM=*/1, /*KIND=*/8, __FILE__, __LINE__);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<std::int64_t>(0), 2);
+  scalarResult.Destroy();
   bool boolValue{false};
   Descriptor &target{statDesc[1].descriptor()};
   target.Establish(TypeCategory::Logical, 1, static_cast<void *>(&boolValue), 0,
       nullptr, CFI_attribute_pointer);
   RTNAME(Findloc)
   (res, *array, target, /*KIND=*/4, __FILE__, __LINE__, nullptr,
       /*BACK=*/false);
   EXPECT_EQ(res.rank(), 1);
(res, *realArray, target, 8, /*DIM=*/2, __FILE__, __LINE__, nullptr,
       /*BACK=*/true);
   EXPECT_EQ(res.rank(), 1);
   EXPECT_EQ(res.type().raw(), (TypeCode{TypeCategory::Integer, 8}.raw()));
   EXPECT_EQ(res.GetDimension(0).LowerBound(), 1);
   EXPECT_EQ(res.GetDimension(0).UpperBound(), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<SubscriptValue>(0), 2);
   EXPECT_EQ(*res.ZeroBasedIndexedElement<SubscriptValue>(1), 0);
   res.Destroy();
+  // Test scalar result for FindlocDim.
+  // A scalar result occurs when you have a rank 1 array, value, and dim == 1.
+  std::vector<int> shape1{6};
+  auto realArray1{MakeArray<TypeCategory::Real, 8>(shape1,
+      std::vector<double>{0.0, -0.0, 1.0, 3.14,
+          std::numeric_limits<double>::quiet_NaN(),
+          std::numeric_limits<double>::infinity()})};
+  StaticDescriptor<0, true> statDesc0;
+  Descriptor &scalarResult{statDesc0.descriptor()};
+  RTNAME(FindlocDim)
+  (scalarResult, *realArray1, target, 8, /*DIM=*/1, __FILE__, __LINE__, nullptr,
+      /*BACK=*/false);
+  EXPECT_EQ(scalarResult.rank(), 0);
+  EXPECT_EQ(*scalarResult.ZeroBasedIndexedElement<SubscriptValue>(0), 3);
+  scalarResult.Destroy();
 }
 
 TEST(Reductions, DotProduct) {
   auto realVector{MakeArray<TypeCategory::Real, 8>(
       std::vector<int>{4}, std::vector<double>{0.0, -0.0, 1.0, -2.0})};
   EXPECT_EQ(
       RTNAME(DotProductReal8)(*realVector, *realVector, __FILE__, __LINE__),
       5.0);