[Sema][ObjC] Invalidate BlockDecl with invalid return expr & its parent BlockExpr

[danix800]

Summary

Invalidate BlockDecl with implicit return type, in case any of the return value exprs is invalid.
Propagating the error info up by replacing BlockExpr with a RecoveryExpr.

(The idea of this fix is given by @hokein(Haojian Wu))

Fix https://github.com/llvm/llvm-project/issues/63863.

[aaron.ballman]

clang/include/clang/AST/ComputeDependence.h

134 ↗

(On Diff #540769)

This doesn't seem like the correct fix, to me -- I would have expected the expression itself to track whether it contains errors, so you wouldn't need to pass in extra information about that.

clang/lib/Sema/SemaExpr.cpp

16749–16753

Hmmm -- is the block *expression* what contains the errors in this case or is it the block declaration? I would have expected this to be an issue for the block declaration created for the block expression. CC @rjmccall

[hokein]

clang/lib/Sema/SemaExpr.cpp

16749–16753

I think a reasonable model is to follow how we handle FunctionDecl, as BlockDecl and FunctionDecl are similar function-decl concepts.

For the crash case like int (^a)() = ^() { return undefined; }, we should:

• invalidate the BlockDecl as its returned type can not be deduced because of the error return stmt (similar to FunctionDecl, we invalidate it for auto func() { return undefined; })
• for an invalid BlockDecl, we should not build a BlockExpr that refers to it (we don't build DeclRefExpr for invalid FunctionDecl). For error recovery, we should use RecoveryExpr.

So I think the reasonable fix is to invalidate the BlockDecl (calling Decl->setInvalidDecl()) if its body has any error stmt, and return ExprError() if the BlockDecl is invalid.

[danix800]

clang/lib/Sema/SemaExpr.cpp

16749–16753

Thanks for sharing and pointing out the right direction.

[danix800]

Invalidate BlockDecl with implicit return type, in case any of the return value exprs is invalid.
Propagating the error info up by replacing BlockExpr with a RecoveryExpr.

[aaron.ballman]

This looks pretty good to me. Can you also add a release note for the fix to clang/docs/ReleaseNotes.rst?

clang/lib/Sema/SemaStmt.cpp

3733–3734

if (auto *CurBlock = dyn_cast<BlockScopeInfo>(CurCap); CurBlock && CurCap->HasImplicitReturnType) {

[danix800]

Update clang/docs/ReleaseNotes.rst to reflect this fix.

[hokein]

thanks!

clang/lib/Sema/SemaStmt.cpp

3736

nit: the isInvalidDecl check is not need, we can inline the RetValExpr to the above if as well.

if (auto *CurBlock = dyn_cast<BlockScopeInfo>(CurCap); CurBlock && CurCap->HasImplicitReturnType && RetValExp && RetValExp->containsErrors()) 
  CurBlock->TheDecl->setInvalidDecl();

clang/test/SemaObjC/crash-on-val-dep-block-expr.m

2

can you move these tests to llvm-project/clang/test/AST/ast-dump-recovery.c?

[danix800]

clang/lib/Sema/SemaStmt.cpp

3736

Nice, look more cleaner!

clang/test/SemaObjC/crash-on-val-dep-block-expr.m

2

No problem. But I noticed llvm-project/clang/test/AST/ast-dump-recovery.m also exists,
should I put them into this .m file instead?

[danix800]

clang/test/SemaObjC/crash-on-val-dep-block-expr.m

2

I'd prefer ast-dump-recovery.m since this file is much shorter. Either file needs appending -fblocks.

[danix800]

1. Remove unnecessary condition & cleanup code
2. Move testcase into correct place. Extraneous parenthesized blockexpr testcase is removed since errors can propagate between exprs normally.

[danix800]

Fix testcase tag with CHECK-NEXT.

[rjmccall]

Dropping the entire block from the AST on error would be unfortunate for some kinds of tooling, but as long as we maintain it with a RecoveryExpr, that seems like a fine approach to me.

As a general rule, I think we should be tracking basically everything on the BlockDecl and let BlockExpr just have a reference to that on top of the baseline Expr storage.

[hokein]

thanks, looks good to me.

clang/test/AST/ast-dump-recovery.m

24 ↗

(On Diff #543010)

nit: it'd be nice to encode the github issue number to the testcase here, for example (renaming the a to gh63863 etc).

25 ↗

(On Diff #543010)

nit: use a more descriptive name return undef;

[danix800]

clang/test/AST/ast-dump-recovery.m

24 ↗

(On Diff #543010)

Will be fixed in the final commit!

clang/lib/Sema/SemaExpr.cpp

       CXXBasePath &Path = Paths.front();
       for (const CXXBasePathElement &B : Path)
         Comps.push_back(OffsetOfNode(B.Base));
     }
 
     if (IndirectMemberDecl) {
       for (auto *FI : IndirectMemberDecl->chain()) {
         assert(isa<FieldDecl>(FI));
         Comps.push_back(OffsetOfNode(OC.LocStart,
                                      cast<FieldDecl>(FI), OC.LocEnd));
       }
     } else
       Comps.push_back(OffsetOfNode(OC.LocStart, MemberDecl, OC.LocEnd));
 
     CurrentType = MemberDecl->getType().getNonReferenceType();
   }
 
   return OffsetOfExpr::Create(Context, Context.getSizeType(), BuiltinLoc, TInfo,
                               Comps, Exprs, RParenLoc);
 }
 

         break;
       }
     }
   }
 
   if (getCurFunction())
     getCurFunction()->addBlock(BD);
 
+  if (BD->isInvalidDecl())
+    return CreateRecoveryExpr(Result->getBeginLoc(), Result->getEndLoc(),
+        {Result}, Result->getType());
   return Result;
 }
 
 ExprResult Sema::ActOnVAArg(SourceLocation BuiltinLoc, Expr *E, ParsedType Ty,
                             SourceLocation RPLoc) {
   TypeSourceInfo *TInfo;
   GetTypeFromParser(Ty, &TInfo);
   return BuildVAArgExpr(BuiltinLoc, E, TInfo, RPLoc);

clang/lib/Sema/SemaStmt.cpp

   // or if we need to infer the return type,
   // save the return statement in our scope for later processing.
   if (CurCap->HasImplicitReturnType || NRVOCandidate)
     FunctionScopes.back()->Returns.push_back(Result);
 
   if (FunctionScopes.back()->FirstReturnLoc.isInvalid())
     FunctionScopes.back()->FirstReturnLoc = ReturnLoc;
 
+  BlockScopeInfo *CurBlock = dyn_cast<BlockScopeInfo>(CurCap);
+  if (CurBlock && CurCap->HasImplicitReturnType) {
+    BlockDecl *BD = CurBlock->TheDecl;
+    if (!BD->isInvalidDecl() && RetValExp && RetValExp->containsErrors())
+      BD->setInvalidDecl();
+  }
+
   return Result;
 }
 
 namespace {
 /// Marks all typedefs in all local classes in a type referenced.
 ///
 /// In a function like
 /// auto f() {

clang/test/SemaObjC/crash-on-val-dep-block-expr.m

+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify %s
+// no crash
+
+int (^a)() = ^() {
+  return c; // expected-error {{use of undeclared identifier 'c'}}
+};
+
+int (^b)() = (^() {
+  return c; // expected-error {{use of undeclared identifier 'c'}}
+});