Page MenuHomePhabricator

[flang] Fix runtime ICE with maxloc and scalar result

Authored by mleair on Jul 26 2021, 12:31 PM.



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

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.

Diff Detail

Event Timeline

mleair created this revision.Jul 26 2021, 12:31 PM
mleair requested review of this revision.Jul 26 2021, 12:31 PM
mleair edited the summary of this revision. (Show Details)Jul 26 2021, 12:33 PM
mleair edited the summary of this revision. (Show Details)Jul 26 2021, 12:36 PM
klausler added inline comments.Jul 26 2021, 12:41 PM

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

PeteSteinfeld added inline comments.Jul 26 2021, 1:13 PM

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.

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

mleair added inline comments.Jul 26 2021, 4:16 PM

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)) {

  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 added inline comments.Jul 26 2021, 4:24 PM

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

klausler added inline comments.Jul 28 2021, 9:14 AM

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 added inline comments.Jul 28 2021, 10:12 AM

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 updated this revision to Diff 362488.Jul 28 2021, 11:55 AM

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

klausler accepted this revision.Jul 29 2021, 9:08 AM
This revision is now accepted and ready to land.Jul 29 2021, 9:08 AM
PeteSteinfeld accepted this revision.Jul 29 2021, 10:45 AM

Thanks, Mark. All looks good.

This revision was landed with ongoing or failed builds.Jul 29 2021, 1:10 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 1:10 PM

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).

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