This is an archive of the discontinued LLVM Phabricator instance.

[flang] Failed call to CHECK() for call to ASSOCIATED(NULL())
ClosedPublic

Authored by PeteSteinfeld on Sep 25 2020, 9:04 AM.

Details

Summary

Calling "ASSOCATED(NULL()) was causing an internal check of the compiler to
fail.

I fixed this by changing the entry for "ASSOCIATED" in the intrinsics table to
accept "AnyPointer" which contains a new "KindCode" of "pointerType". I also
changed the function "FromActual()" which to return a typeless intrinsic when
called on a pointer, which duplicates its behavior for BOZ literals. This
required changing the analysis of procedure arguments. I wrote additional
tests to cover all of the code. This exposed an additional failing call to
CHECK() that I fixed.

I made several other changes:

  • I implemented constant folding for ASSOCIATED().
  • I fixed handling of NULL() in relational operations.
  • I implemented semantic analysis for ASSOCIATED().
  • I noticed that the semantics for ASSOCIATED() are similar to those for pointer assignment. So I extracted the code that pointer assignment uses for procedure pointer compatibility to a place where it could be used by the semantic analysis for ASSOCIATED().
  • I couldn't figure out how to make the general semantic analysis for procedure arguments work with ASSOCIATED()'s second argument, which can be either a pointer or a target. So I stopped using normal semantic analysis for arguments for ASSOCIATED().
  • I added tests for all of this.

Diff Detail

Event Timeline

PeteSteinfeld created this revision.Sep 25 2020, 9:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
PeteSteinfeld requested review of this revision.Sep 25 2020, 9:04 AM
PeteSteinfeld added a project: Restricted Project.Sep 25 2020, 9:24 AM
klausler added inline comments.Sep 25 2020, 11:47 AM
flang/include/flang/Evaluate/characteristics.h
231

Magic flag arguments should be avoided. If you want special-case handling of some input in some situation, consider adding a wrapper function with the magic that defaults to the original function otherwise. But maybe the new special behavior doesn't have to be special -- what would break if you just changed the treatment of NULL for all cases?

flang/include/flang/Evaluate/type.h
205

ASSOCIATED

flang/lib/Evaluate/characteristics.cpp
361

Obsolete comment?

flang/lib/Evaluate/intrinsics.cpp
87

If it's only meant to be used for NULL(), please use a more specific name.

286

Can the optional TARGET= argument be NULL(MOLD=) or even just NULL()?

PeteSteinfeld added inline comments.Sep 25 2020, 12:03 PM
flang/include/flang/Evaluate/characteristics.h
231

Thanks, Peter.

My first impulse was to change the treatment of NULL for both cases, but that caused problems in the treatment of procedure arguments. I'll take another look at this and see if I can make it work. If not, I'll look at adding a wrapper function.

DavidTruby resigned from this revision.Sep 30 2020, 5:34 AM
PeteSteinfeld added inline comments.Sep 30 2020, 8:33 AM
flang/lib/Evaluate/intrinsics.cpp
286

Good question.

My reading of the standard is that NULL() should be allowed as the TARGET
argument of ASSOCIATED(). The note at the bottom of section 16.9.16
explicitly mentions disassociated pointers. Also, if we're going to allow
NULL() as the first argument, it seems like we're implicitly stating that
null pointers match all pointer types, and thus we should allow NULL() as the
second argument to ASSOCIATED().

I wrote some tests and tried them on several compilers. The only one that
seems to agree with my analysis above is IBM. GNU, pgf90, nagfor, and ifort
all give various compile-time errors when using any form of null() as the
second argument to ASSOCIATED().

I reverted the interface of the FromActual() function. This required redoing
the analysis of procedure arguments. To make sure I covered all the code in
the anslysis of procedure arguments, I added some tests that exposed another
bad call to CHECK() that I fixed. I added tests for all of this.

I also added some tests for passing NULL() as the second argument to
ASSOCIATED().

PeteSteinfeld edited the summary of this revision. (Show Details)Sep 30 2020, 10:05 AM
klausler added inline comments.Sep 30 2020, 11:06 AM
flang/lib/Evaluate/characteristics.cpp
431

Please use const auto * here.

flang/lib/Evaluate/intrinsics.cpp
286

Will you allow NULL() as the second (TARGET=) argument here?

1729

associated*

PeteSteinfeld added inline comments.Sep 30 2020, 11:38 AM
flang/lib/Evaluate/intrinsics.cpp
286

Yes. I added a test for this to resolve89.f90. I would think that, if the second argument is NULL() and the first argument evaluates to NULL(), that ASSOCIATED() would evaluate to .TRUE..

Responding to review comments.

klausler added inline comments.Oct 2 2020, 9:01 AM
flang/lib/Evaluate/intrinsics.cpp
286

I could read the standard either way for the case with two NULL()s. What does XLF do?

PeteSteinfeld added inline comments.Oct 2 2020, 10:22 AM
flang/lib/Evaluate/intrinsics.cpp
286

xlf is the only compiler I can find that does not have a fatal error when evaluating associated(null(), null()). It gives the warning:

"test.f90", line 2.7: 1516-303 (W) The pointer and target specified for the ASSOCIATED intrinsic are not type compatible or have mismatching type parameters or rank.  The value of ASSOCIATED will be FALSE.

As the message says, it evaluates the call to associated() to false.

PeteSteinfeld added inline comments.Oct 2 2020, 10:28 AM
flang/lib/Evaluate/intrinsics.cpp
286

nagfor compiles without warnings and also evaluates to false.

klausler added inline comments.Oct 2 2020, 10:43 AM
flang/lib/Evaluate/intrinsics.cpp
286

Ok, so there's good precedent for .false.. Can you fold associated(null(),null()) to .false. with a warning?

PeteSteinfeld added inline comments.Oct 2 2020, 11:03 AM
flang/lib/Evaluate/intrinsics.cpp
286

I'll look into folding. There's also the case of folding associated(null()). Note that nagfor is the only compiler that compiles this without error. As expected, it evaluates it to false.

I made several changes:

  • I implemented constant folding for ASSOCIATED().
  • I fixed handling of NULL() in relational operations.
  • I implemented semantic analysis for ASSOCIATED().
  • I noticed that the semantics for ASSOCIATED() are similar to those for pointer assignment. So I extracted the code that pointer assignment uses for procedure pointer compatibility to a place where it could be used by the semantic analysis for ASSOCIATED().
  • I couldn't figure out how to make the general semantic analysis for procedure arguments work with ASSOCIATED()'s second argument, which can be either a pointer or a target. So I stopped using normal semantic analysis for arguments for ASSOCIATED().
  • I added tests for all of this.

Fixed some formatting.

PeteSteinfeld edited the summary of this revision. (Show Details)Oct 15 2020, 7:18 AM
klausler added inline comments.Oct 15 2020, 10:56 AM
flang/lib/Evaluate/characteristics.cpp
437

Could be

const auto *argObj{...}.
return argObj && argObj->type.type().IsTypelessIntrinsicArgument();
flang/lib/Evaluate/intrinsics.cpp
1874

This block of code has grown to the point that it should be extracted into its own function, I think.

flang/lib/Evaluate/tools.cpp
894 ↗(On Diff #298373)

Debugging code needs scrubbing.

PeteSteinfeld marked 10 inline comments as done.Oct 15 2020, 12:57 PM

Thanks for the comments. New update coming.

flang/lib/Evaluate/characteristics.cpp
437

Thanks. I'll fix this.

flang/lib/Evaluate/intrinsics.cpp
1874

Thanks. I'll do it.

flang/lib/Evaluate/tools.cpp
894 ↗(On Diff #298373)

Ay yi yi.

PeteSteinfeld marked 3 inline comments as done.

Responses to review comments.

I recoded a function in characteristics.cpp, pulled the semantic checking for
ASSOCIATED() to a separate function in intrinsics.cpp, and removed debug code
from tools.cpp.

klausler accepted this revision.Oct 15 2020, 1:19 PM

LGTM and thanks!

This revision is now accepted and ready to land.Oct 15 2020, 1:19 PM
This revision was landed with ongoing or failed builds.Oct 16 2020, 7:17 AM
This revision was automatically updated to reflect the committed changes.