[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

1 ↗

(On Diff #542735)

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

1 ↗

(On Diff #542735)

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

1 ↗

(On Diff #542735)

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

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

25

nit: use a more descriptive name return undef;

[danix800]

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

24

Will be fixed in the final commit!

clang/docs/ReleaseNotes.rst

 - Fix crash when casting an object to an array type.
   (`#63758 <https://github.com/llvm/llvm-project/issues/63758>_`)
 - Fixed false positive error diagnostic observed from mixing ``asm goto`` with
   ``__attribute__((cleanup()))`` variables falsely warning that jumps to
   non-targets would skip cleanup.
 - Correcly diagnose jumps into statement expressions.
   This ensures the behavior of Clang is consistent with GCC.
   (`#63682 <https://github.com/llvm/llvm-project/issues/63682>`_)
+- 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. This fixes:
+  (`#63863 <https://github.com/llvm/llvm-project/issues/63863>_`)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaExpr.cpp

if (IsDerivedFrom(OC.LocStart, CurrentType, Context.getTypeDeclType(Parent),
       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);
 }
 
for (const auto &CI : Result->getBlockDecl()->captures()) {
         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

StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
   // 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;
 
+  if (auto *CurBlock = dyn_cast<BlockScopeInfo>(CurCap);
+      CurBlock && CurCap->HasImplicitReturnType && RetValExp &&
+      RetValExp->containsErrors())
+    CurBlock->TheDecl->setInvalidDecl();
+
   return Result;
 }
 
 namespace {
 /// Marks all typedefs in all local classes in a type referenced.
 ///
 /// In a function like
 /// auto f() {

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

-// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -frecovery-ast -frecovery-ast-type -ast-dump %s | FileCheck -strict-whitespace %s
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -frecovery-ast -frecovery-ast-type -fblocks -ast-dump %s | FileCheck -strict-whitespace %s
 
 @interface Foo
 - (void)method:(int)n;
 @end
 
 void k(Foo *foo) {
   // CHECK:       ObjCMessageExpr {{.*}} 'void' contains-errors
   // CHECK-CHECK:  |-ImplicitCastExpr {{.*}} 'Foo *' <LValueToRValue>
   // CHECK-CHECK:  | `-DeclRefExpr {{.*}} 'foo'
   // CHECK-CHECK:  `-RecoveryExpr {{.*}}
   [foo method:undef];
 
   // CHECK:      ImplicitCastExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>' contains-errors
   // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'foo'
   foo.undef;
 }
+
+// CHECK:      |-VarDecl {{.*}} 'int (^)()' cinit
+// CHECK-NEXT: | `-RecoveryExpr {{.*}} '<dependent type> (^)(void)' contains-errors lvalue
+// CHECK-NEXT: |   `-BlockExpr {{.*}} '<dependent type> (^)(void)'
+// CHECK-NEXT: |     `-BlockDecl {{.*}} invalid
+int (^gh63863)() = ^() {
+  return undef;
+};