Page MenuHomePhabricator

[flang] Fix crash on erroneous expressions
ClosedPublic

Authored by PeteSteinfeld on Jun 3 2020, 8:08 AM.

Details

Summary

If you create an expression with parse errors, the parser::Expr.typedExpr
will be empty, which causes a compiler crash. The crash is caused by the
check in check-do-forall.cpp that scans all expresssions to see if DO
variables are being modified.

It turned out that the problem was that I was fetching subexpressions of type
parser::Expr, which are not guaranteed to have a non-null typedExpr. I
fixed this by only grabbing the top-level expression from which to gather
arguments as part of the DO loop analysis. This, in turn, exposed a problem
where I wasn't collecting all of the actual arguments in an expression.
Specifically, if an actual argument was a function call, I wasn't recursing
through the arguments of the called function. I fixed this by recursing
through the argument in the member function in
CollectActualArgumentsHelper.

Diff Detail

Event Timeline

PeteSteinfeld created this revision.Jun 3 2020, 8:08 AM
Herald added a project: Restricted Project. · View Herald Transcript
PeteSteinfeld added a project: Restricted Project.Jun 3 2020, 8:09 AM
tskeith added inline comments.Jun 3 2020, 8:54 AM
flang/lib/Semantics/check-do-forall.cpp
1052

GetExpr is supposed to return nullptr if the typedExpr is not present. If it's crashing the bug is inside GetExpr. Otherwise every call would have to be guarded with a check like this.

PeteSteinfeld marked an inline comment as done.Jun 3 2020, 3:18 PM
PeteSteinfeld added inline comments.
flang/lib/Semantics/check-do-forall.cpp
1052

In the code before I made my change, GetExpr(parser::Expr) was explicitly checking for an empty typedExpr by calling CheckMissingAnalysis() and calling common::die() if typedExpr was empty. But the code in check-do-forall.cpp is a little unusual since it's walking all expressions in a program looking for invalid uses of DO variables. That's why I put the check here. I guessed that all other calls to GetExpr() were done in situations where we were sure that typedExpr was not empty.

Note also that when GetExpr is called on a parser::Variable it also calls common::die() when typedExpr is missing. But I'm not sure how to construct a test case that would trigger this.

Should I just get rid of the call to CheckMissingAnalysis() when calling GetExpr(parser::Expr)? What about for the parser::Variable case?

tskeith added inline comments.Jun 3 2020, 3:57 PM
flang/lib/Semantics/check-do-forall.cpp
1052

There are two different kinds of "missing" that can happen in a typedExpr:

  • if the unique_ptr is null it means we haven't tried to analyze it yet
  • if the optional in GenericExprWrapper is absent it means we analyzed it and reported an error

The error in CheckMissingAnalysis is for the first case. It means expression analysis didn't look at that expression, but it should have by that point. So probably the bug is there.

It turned out that the problem was that I was fetching subexpressions of type
parser::Expr, which are not guaranteed to have a non-null typedExpr. I
fixed this by only grabbing the top-level expression from which to gather
arguments as part of the DO loop analysis. This, in turn, exposed a problem
where I wasn't collecting all of the actual arguments in an expression. This
was caused by the fact that I wasn't recursing through the rest of the
expression after finding an argument. I fixed this by recursing through the
argument in the member function in CollectActualArgumentsHelper.

PeteSteinfeld edited the summary of this revision. (Show Details)Jun 4 2020, 1:57 PM
tskeith added inline comments.Jun 4 2020, 3:23 PM
flang/lib/Semantics/check-do-forall.cpp
1048

This should be a member of DoForallChecker. It isn't meaningful outside of that class.

At Tim's suggestion, I moved exprDepth from being a static variable to a data
member.

PeteSteinfeld marked an inline comment as done.Jun 5 2020, 7:11 AM
PeteSteinfeld added inline comments.
flang/lib/Semantics/check-do-forall.cpp
1048

Thanks, Tim. I've made that change.

tskeith accepted this revision.Jun 5 2020, 7:43 AM

Looks good. Be sure to update the description to match the change before committing.

This revision is now accepted and ready to land.Jun 5 2020, 7:43 AM
sscalpone accepted this revision.Jun 5 2020, 7:54 AM
This revision was automatically updated to reflect the committed changes.