This is an archive of the discontinued LLVM Phabricator instance.

[flang] Detect circularly defined procedures
ClosedPublic

Authored by PeteSteinfeld on Feb 12 2021, 1:29 PM.

Details

Summary

It's possible to define a procedure that has a procedure dummy argument which
names the procedure that contains it. This was causing the compiler to fall
into an infinite loop when characterizing a call to the procedure.

Following a suggestion from Peter, I fixed this be maintaining a set of
procedure symbols that had already been seen while characterizing a procedure.
This required passing a new parameter to the functions that characterized a
Procedure, a DummyArgument, and a DummyProcedure.

I also added several tests that will crash the compiler without this change.

Diff Detail

Event Timeline

PeteSteinfeld requested review of this revision.Feb 12 2021, 1:29 PM
PeteSteinfeld created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 1:29 PM
PeteSteinfeld added a project: Restricted Project.Feb 12 2021, 1:30 PM
klausler accepted this revision.Feb 12 2021, 1:48 PM
klausler added inline comments.
flang/include/flang/Evaluate/characteristics.h
211

As a convenience to external clients, this new argument could be either a pointer that default to nullptr, or there could be an overload without the set argument that declares one and passes it onward (my preference).

flang/lib/Evaluate/characteristics.cpp
655

Please clarify this message so that the user can tell that the circular dependence is in the specification of the procedure's interface or characteristics.

This revision is now accepted and ready to land.Feb 12 2021, 1:48 PM
PeteSteinfeld added inline comments.Feb 12 2021, 2:15 PM
flang/include/flang/Evaluate/characteristics.h
211

I'm not sure if I understand your comment. There are no external clients for the Characterize() functions for a DummyProcedure or a DummyArgument. Rather, all of the calls to them happen in .../Evaluate/characteristics.cpp. There are external clients for Procedure::Characterize(). For this function, I did as you recommend -- the version without the set argument creates one and passes it on to the overload that contains the set argument.

flang/lib/Evaluate/characteristics.cpp
655

I'll take a crack at it.

Responding to Peter's feedback -- an attempt to clarify the error message.

I think there's ways by which the characteristics of a procedure could be self-dependent besides the use of a dummy procedure, so you might want a less specific error. But that's probably the most common way that a user can get themselves into trouble with this sort of thing, so you might be okay. Improve or change the message as you like, or not. Maybe Tim can suggest better wording.

I think there's ways by which the characteristics of a procedure could be self-dependent besides the use of a dummy procedure, so you might want a less specific error. But that's probably the most common way that a user can get themselves into trouble with this sort of thing, so you might be okay. Improve or change the message as you like, or not. Maybe Tim can suggest better wording.

Thanks, Peter. It occurred to me that I can also improve the message by including the names of all of the procedures in the cycle as part of the error message. I'll be producing another update that does that. Let me know what you think when I update the commit.

Trying to improve the error message when a circularity is detected. I added a
list of the procedures in the cycle.

Added a reference to the standard.

tskeith added inline comments.Feb 14 2021, 8:23 PM
flang/include/flang/Evaluate/characteristics.h
57

symbol.h defines SymbolSet which you could use here.

211

The new overloads that include SeenProcs aren't used by external clients so perhaps they can be static non-member functions in characteristics.cpp. Then the external API won't change at all.

flang/lib/Evaluate/characteristics.cpp
648

This could be const SeenProcs &.

651

clang-tidy says this should be const auto &procPtr. But const auto *procPtr would be better.

flang/test/Semantics/resolve100.f90
4

I think the procedure names should be quoted individually, i.e. 'sub', 'p2'.

tskeith added inline comments.Feb 15 2021, 11:03 AM
flang/lib/Evaluate/characteristics.cpp
659

FYI, llvm has utility functions in llvm/ADT/STLExtras.h that simplify this kind of thing. If you take my suggestion of enclosing each procedure name in quotes you could implement this function as:

llvm::interleave(
    seenProcs,
    [&](const Symbol *p) { result += '\'' + p->name().ToString() + '\''; },
    [&]() { result += ", "; });
PeteSteinfeld added inline comments.Feb 15 2021, 7:04 PM
flang/include/flang/Evaluate/characteristics.h
57

Thanks, I'll do that.

211

Good suggestion. And even though the Characterize() methods of DummyArgument and DummyProcedure aren't overloads, It seems to me that they should also be made static in characteristics.cpp.

flang/lib/Evaluate/characteristics.cpp
651

Or const SymbolRef.

659

How do you find this stuff? I'll do it.

flang/test/Semantics/resolve100.f90
4

Will do.

Responding to comments from Tim. I made the functions that characterize
Procedure, DummyArgument, and DummyProcedure that are mutually recursive all be
static and reside completely in characteristics.cpp. I also started using the
type SymbolSet and recoded the function to create a string of comma separated
names to use a utility from LLVM.

tskeith accepted this revision.Feb 16 2021, 12:33 PM

LGTM

flang/test/Semantics/resolve100.f90
4

There are extra quotes around the whole list.

This revision was automatically updated to reflect the committed changes.