[PATCH] D81101: [flang] Fix crash on erroneous expressions

[Pete Steinfeld via Phabricator via llvm-commits]

PeteSteinfeld marked an inline comment as done.
PeteSteinfeld added inline comments.


================
Comment at: flang/lib/Semantics/check-do-forall.cpp:1049
+  if (parsedExpr.typedExpr) { // typedExpr is empty on expressions with errors
+    if (const SomeExpr * expr{GetExpr(parsedExpr)}) {
+      ActualArgumentSet argSet{CollectActualArguments(*expr)};
----------------
tskeith wrote:
> `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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81101/new/

https://reviews.llvm.org/D81101