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

[Tim Keith via Phabricator via llvm-commits]

tskeith 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)};
----------------
PeteSteinfeld wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81101