Fix bad diagnostic when having a constexpr template function call in OmpSs-2 context

mikaoP · mikaoP · commit 91c68ecbd801 · 2020-09-07T12:15:44.000Z
The old method to detect a return instr. inside task body does not work because Scope is used only in parsing, not in instantiation. To make it work properly we push a fake FunctionScopeInfo with a flag to detect we are in task body context. Also, since a lambda tries to propagate captures to outer ones we have to skip our fake FunctionScopeInfo. Closes llvm#96 llvm#97
diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
@@ -124,9 +124,12 @@ class FunctionScopeInfo {
   /// True if current scope is for OpenMP declare reduction combiner.
   bool HasOMPDeclareReductionCombiner : 1;
 
-  /// True if current scope is for OmpSs declare reduction combiner.
+  /// True if current scope is for OmpSs-2 declare reduction combiner.
   bool HasOSSDeclareReductionCombiner : 1;
 
+  /// True if current scope is for OmpSs-2 task/taskloop...
+  bool HasOSSExecutableDirective : 1;
+
   /// Whether there is a fallthrough statement in this function.
   bool HasFallthroughStmt : 1;
 
@@ -371,6 +374,7 @@ class FunctionScopeInfo {
         HasBranchIntoScope(false), HasIndirectGoto(false),
         HasDroppedStmt(false), HasOMPDeclareReductionCombiner(false),
         HasOSSDeclareReductionCombiner(false),
+        HasOSSExecutableDirective(false),
         HasFallthroughStmt(false), HasPotentialAvailabilityViolations(false),
         ObjCShouldCallSuper(false), ObjCIsDesignatedInit(false),
         ObjCWarnForNoDesignatedInitChain(false), ObjCIsSecondaryInit(false),
@@ -422,6 +426,10 @@ class FunctionScopeInfo {
     HasOSSDeclareReductionCombiner = true;
   }
 
+  void setHasOSSExecutableDirective() {
+    HasOSSExecutableDirective = true;
+  }
+
   void setHasFallthroughStmt() {
     HasFallthroughStmt = true;
   }
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -10895,6 +10895,10 @@ class Sema final {
       Scope *S, DeclGroupPtrTy DeclReductions, bool IsValid);
 
 
+  // Used to push a fake FunctionScopeInfo
+  void ActOnOmpSsExecutableDirectiveStart();
+  // Used to pop the fake FunctionScopeInfo
+  void ActOnOmpSsExecutableDirectiveEnd();
   StmtResult ActOnOmpSsExecutableDirective(ArrayRef<OSSClause *> Clauses,
       OmpSsDirectiveKind Kind, Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc);
 
diff --git a/clang/lib/Parse/ParseOmpSs.cpp b/clang/lib/Parse/ParseOmpSs.cpp
@@ -594,7 +594,9 @@ StmtResult Parser::ParseOmpSsDeclarativeOrExecutableDirective(
 
     StmtResult AssociatedStmt;
     if (HasAssociatedStatement) {
+      Actions.ActOnOmpSsExecutableDirectiveStart();
       AssociatedStmt = (Sema::CompoundScopeRAII(Actions), ParseStatement());
+      Actions.ActOnOmpSsExecutableDirectiveEnd();
     }
 
     Clauses = StackClauses.back();
diff --git a/clang/lib/Sema/ScopeInfo.cpp b/clang/lib/Sema/ScopeInfo.cpp
@@ -29,6 +29,7 @@ void FunctionScopeInfo::Clear() {
   HasDroppedStmt = false;
   HasOMPDeclareReductionCombiner = false;
   HasOSSDeclareReductionCombiner = false;
+  HasOSSExecutableDirective = false;
   HasFallthroughStmt = false;
   HasPotentialAvailabilityViolations = false;
   ObjCShouldCallSuper = false;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
@@ -17317,118 +17317,120 @@ bool Sema::tryCaptureVariable(
     }
 
     FunctionScopeInfo  *FSI = FunctionScopes[FunctionScopesIndex];
-    CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FSI);
 
+    if (!(getLangOpts().OmpSs && FSI->HasOSSExecutableDirective)) {
+      CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FSI);
 
-    // Check whether we've already captured it.
-    if (isVariableAlreadyCapturedInScopeInfo(CSI, Var, Nested, CaptureType,
-                                             DeclRefType)) {
-      CSI->getCapture(Var).markUsed(BuildAndDiagnose);
-      break;
-    }
-    // If we are instantiating a generic lambda call operator body,
-    // we do not want to capture new variables.  What was captured
-    // during either a lambdas transformation or initial parsing
-    // should be used.
-    if (isGenericLambdaCallOperatorSpecialization(DC)) {
-      if (BuildAndDiagnose) {
-        LambdaScopeInfo *LSI = cast<LambdaScopeInfo>(CSI);
-        if (LSI->ImpCaptureStyle == CapturingScopeInfo::ImpCap_None) {
-          Diag(ExprLoc, diag::err_lambda_impcap) << Var->getDeclName();
-          Diag(Var->getLocation(), diag::note_previous_decl)
-             << Var->getDeclName();
-          Diag(LSI->Lambda->getBeginLoc(), diag::note_lambda_decl);
-        } else
-          diagnoseUncapturableValueReference(*this, ExprLoc, Var, DC);
+      // Check whether we've already captured it.
+      if (isVariableAlreadyCapturedInScopeInfo(CSI, Var, Nested, CaptureType,
+                                               DeclRefType)) {
+        CSI->getCapture(Var).markUsed(BuildAndDiagnose);
+        break;
+      }
+      // If we are instantiating a generic lambda call operator body,
+      // we do not want to capture new variables.  What was captured
+      // during either a lambdas transformation or initial parsing
+      // should be used.
+      if (isGenericLambdaCallOperatorSpecialization(DC)) {
+        if (BuildAndDiagnose) {
+          LambdaScopeInfo *LSI = cast<LambdaScopeInfo>(CSI);
+          if (LSI->ImpCaptureStyle == CapturingScopeInfo::ImpCap_None) {
+            Diag(ExprLoc, diag::err_lambda_impcap) << Var->getDeclName();
+            Diag(Var->getLocation(), diag::note_previous_decl)
+               << Var->getDeclName();
+            Diag(LSI->Lambda->getBeginLoc(), diag::note_lambda_decl);
+          } else
+            diagnoseUncapturableValueReference(*this, ExprLoc, Var, DC);
+        }
+        return true;
+      }
+
+      // Try to capture variable-length arrays types.
+      if (Var->getType()->isVariablyModifiedType()) {
+        // We're going to walk down into the type and look for VLA
+        // expressions.
+        QualType QTy = Var->getType();
+        if (ParmVarDecl *PVD = dyn_cast_or_null<ParmVarDecl>(Var))
+          QTy = PVD->getOriginalType();
+        captureVariablyModifiedType(Context, QTy, CSI);
       }
-      return true;
-    }
 
-    // Try to capture variable-length arrays types.
-    if (Var->getType()->isVariablyModifiedType()) {
-      // We're going to walk down into the type and look for VLA
-      // expressions.
-      QualType QTy = Var->getType();
-      if (ParmVarDecl *PVD = dyn_cast_or_null<ParmVarDecl>(Var))
-        QTy = PVD->getOriginalType();
-      captureVariablyModifiedType(Context, QTy, CSI);
-    }
-
-    if (getLangOpts().OpenMP) {
-      if (auto *RSI = dyn_cast<CapturedRegionScopeInfo>(CSI)) {
-        // OpenMP private variables should not be captured in outer scope, so
-        // just break here. Similarly, global variables that are captured in a
-        // target region should not be captured outside the scope of the region.
-        if (RSI->CapRegionKind == CR_OpenMP) {
-          OpenMPClauseKind IsOpenMPPrivateDecl = isOpenMPPrivateDecl(
-              Var, RSI->OpenMPLevel, RSI->OpenMPCaptureLevel);
-          // If the variable is private (i.e. not captured) and has variably
-          // modified type, we still need to capture the type for correct
-          // codegen in all regions, associated with the construct. Currently,
-          // it is captured in the innermost captured region only.
-          if (IsOpenMPPrivateDecl != OMPC_unknown &&
-              Var->getType()->isVariablyModifiedType()) {
-            QualType QTy = Var->getType();
-            if (ParmVarDecl *PVD = dyn_cast_or_null<ParmVarDecl>(Var))
-              QTy = PVD->getOriginalType();
-            for (int I = 1, E = getNumberOfConstructScopes(RSI->OpenMPLevel);
-                 I < E; ++I) {
-              auto *OuterRSI = cast<CapturedRegionScopeInfo>(
-                  FunctionScopes[FunctionScopesIndex - I]);
-              assert(RSI->OpenMPLevel == OuterRSI->OpenMPLevel &&
-                     "Wrong number of captured regions associated with the "
-                     "OpenMP construct.");
-              captureVariablyModifiedType(Context, QTy, OuterRSI);
+      if (getLangOpts().OpenMP) {
+        if (auto *RSI = dyn_cast<CapturedRegionScopeInfo>(CSI)) {
+          // OpenMP private variables should not be captured in outer scope, so
+          // just break here. Similarly, global variables that are captured in a
+          // target region should not be captured outside the scope of the region.
+          if (RSI->CapRegionKind == CR_OpenMP) {
+            OpenMPClauseKind IsOpenMPPrivateDecl = isOpenMPPrivateDecl(
+                Var, RSI->OpenMPLevel, RSI->OpenMPCaptureLevel);
+            // If the variable is private (i.e. not captured) and has variably
+            // modified type, we still need to capture the type for correct
+            // codegen in all regions, associated with the construct. Currently,
+            // it is captured in the innermost captured region only.
+            if (IsOpenMPPrivateDecl != OMPC_unknown &&
+                Var->getType()->isVariablyModifiedType()) {
+              QualType QTy = Var->getType();
+              if (ParmVarDecl *PVD = dyn_cast_or_null<ParmVarDecl>(Var))
+                QTy = PVD->getOriginalType();
+              for (int I = 1, E = getNumberOfConstructScopes(RSI->OpenMPLevel);
+                   I < E; ++I) {
+                auto *OuterRSI = cast<CapturedRegionScopeInfo>(
+                    FunctionScopes[FunctionScopesIndex - I]);
+                assert(RSI->OpenMPLevel == OuterRSI->OpenMPLevel &&
+                       "Wrong number of captured regions associated with the "
+                       "OpenMP construct.");
+                captureVariablyModifiedType(Context, QTy, OuterRSI);
+              }
+            }
+            bool IsTargetCap =
+                IsOpenMPPrivateDecl != OMPC_private &&
+                isOpenMPTargetCapturedDecl(Var, RSI->OpenMPLevel,
+                                           RSI->OpenMPCaptureLevel);
+            // Do not capture global if it is not privatized in outer regions.
+            bool IsGlobalCap =
+                IsGlobal && isOpenMPGlobalCapturedDecl(Var, RSI->OpenMPLevel,
+                                                       RSI->OpenMPCaptureLevel);
+
+            // When we detect target captures we are looking from inside the
+            // target region, therefore we need to propagate the capture from the
+            // enclosing region. Therefore, the capture is not initially nested.
+            if (IsTargetCap)
+              adjustOpenMPTargetScopeIndex(FunctionScopesIndex, RSI->OpenMPLevel);
+
+            if (IsTargetCap || IsOpenMPPrivateDecl == OMPC_private ||
+                (IsGlobal && !IsGlobalCap)) {
+              Nested = !IsTargetCap;
+              DeclRefType = DeclRefType.getUnqualifiedType();
+              CaptureType = Context.getLValueReferenceType(DeclRefType);
+              break;
             }
-          }
-          bool IsTargetCap =
-              IsOpenMPPrivateDecl != OMPC_private &&
-              isOpenMPTargetCapturedDecl(Var, RSI->OpenMPLevel,
-                                         RSI->OpenMPCaptureLevel);
-          // Do not capture global if it is not privatized in outer regions.
-          bool IsGlobalCap =
-              IsGlobal && isOpenMPGlobalCapturedDecl(Var, RSI->OpenMPLevel,
-                                                     RSI->OpenMPCaptureLevel);
-
-          // When we detect target captures we are looking from inside the
-          // target region, therefore we need to propagate the capture from the
-          // enclosing region. Therefore, the capture is not initially nested.
-          if (IsTargetCap)
-            adjustOpenMPTargetScopeIndex(FunctionScopesIndex, RSI->OpenMPLevel);
-
-          if (IsTargetCap || IsOpenMPPrivateDecl == OMPC_private ||
-              (IsGlobal && !IsGlobalCap)) {
-            Nested = !IsTargetCap;
-            DeclRefType = DeclRefType.getUnqualifiedType();
-            CaptureType = Context.getLValueReferenceType(DeclRefType);
-            break;
           }
         }
       }
-    }
-    if (CSI->ImpCaptureStyle == CapturingScopeInfo::ImpCap_None && !Explicit) {
-      // No capture-default, and this is not an explicit capture
-      // so cannot capture this variable.
-      if (BuildAndDiagnose) {
-        Diag(ExprLoc, diag::err_lambda_impcap) << Var->getDeclName();
-        Diag(Var->getLocation(), diag::note_previous_decl)
-          << Var->getDeclName();
-        if (cast<LambdaScopeInfo>(CSI)->Lambda)
-          Diag(cast<LambdaScopeInfo>(CSI)->Lambda->getBeginLoc(),
-               diag::note_lambda_decl);
-        // FIXME: If we error out because an outer lambda can not implicitly
-        // capture a variable that an inner lambda explicitly captures, we
-        // should have the inner lambda do the explicit capture - because
-        // it makes for cleaner diagnostics later.  This would purely be done
-        // so that the diagnostic does not misleadingly claim that a variable
-        // can not be captured by a lambda implicitly even though it is captured
-        // explicitly.  Suggestion:
-        //  - create const bool VariableCaptureWasInitiallyExplicit = Explicit
-        //    at the function head
-        //  - cache the StartingDeclContext - this must be a lambda
-        //  - captureInLambda in the innermost lambda the variable.
+      if (CSI->ImpCaptureStyle == CapturingScopeInfo::ImpCap_None && !Explicit) {
+        // No capture-default, and this is not an explicit capture
+        // so cannot capture this variable.
+        if (BuildAndDiagnose) {
+          Diag(ExprLoc, diag::err_lambda_impcap) << Var->getDeclName();
+          Diag(Var->getLocation(), diag::note_previous_decl)
+            << Var->getDeclName();
+          if (cast<LambdaScopeInfo>(CSI)->Lambda)
+            Diag(cast<LambdaScopeInfo>(CSI)->Lambda->getBeginLoc(),
+                 diag::note_lambda_decl);
+          // FIXME: If we error out because an outer lambda can not implicitly
+          // capture a variable that an inner lambda explicitly captures, we
+          // should have the inner lambda do the explicit capture - because
+          // it makes for cleaner diagnostics later.  This would purely be done
+          // so that the diagnostic does not misleadingly claim that a variable
+          // can not be captured by a lambda implicitly even though it is captured
+          // explicitly.  Suggestion:
+          //  - create const bool VariableCaptureWasInitiallyExplicit = Explicit
+          //    at the function head
+          //  - cache the StartingDeclContext - this must be a lambda
+          //  - captureInLambda in the innermost lambda the variable.
+        }
+        return true;
       }
-      return true;
     }
 
     FunctionScopesIndex--;
@@ -17444,40 +17446,42 @@ bool Sema::tryCaptureVariable(
   bool Invalid = false;
   for (unsigned I = ++FunctionScopesIndex, N = MaxFunctionScopesIndex + 1; I != N;
        ++I) {
-    CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FunctionScopes[I]);
-
-    // Certain capturing entities (lambdas, blocks etc.) are not allowed to capture
-    // certain types of variables (unnamed, variably modified types etc.)
-    // so check for eligibility.
-    if (!Invalid)
-      Invalid =
-          !isVariableCapturable(CSI, Var, ExprLoc, BuildAndDiagnose, *this);
-
-    // After encountering an error, if we're actually supposed to capture, keep
-    // capturing in nested contexts to suppress any follow-on diagnostics.
-    if (Invalid && !BuildAndDiagnose)
-      return true;
+    if (!(getLangOpts().OmpSs && FunctionScopes[I]->HasOSSExecutableDirective)) {
+      CapturingScopeInfo *CSI = cast<CapturingScopeInfo>(FunctionScopes[I]);
+
+      // Certain capturing entities (lambdas, blocks etc.) are not allowed to capture
+      // certain types of variables (unnamed, variably modified types etc.)
+      // so check for eligibility.
+      if (!Invalid)
+        Invalid =
+            !isVariableCapturable(CSI, Var, ExprLoc, BuildAndDiagnose, *this);
+
+      // After encountering an error, if we're actually supposed to capture, keep
+      // capturing in nested contexts to suppress any follow-on diagnostics.
+      if (Invalid && !BuildAndDiagnose)
+        return true;
 
-    if (BlockScopeInfo *BSI = dyn_cast<BlockScopeInfo>(CSI)) {
-      Invalid = !captureInBlock(BSI, Var, ExprLoc, BuildAndDiagnose, CaptureType,
-                               DeclRefType, Nested, *this, Invalid);
-      Nested = true;
-    } else if (CapturedRegionScopeInfo *RSI = dyn_cast<CapturedRegionScopeInfo>(CSI)) {
-      Invalid = !captureInCapturedRegion(RSI, Var, ExprLoc, BuildAndDiagnose,
-                                         CaptureType, DeclRefType, Nested,
-                                         *this, Invalid);
-      Nested = true;
-    } else {
-      LambdaScopeInfo *LSI = cast<LambdaScopeInfo>(CSI);
-      Invalid =
-          !captureInLambda(LSI, Var, ExprLoc, BuildAndDiagnose, CaptureType,
-                           DeclRefType, Nested, Kind, EllipsisLoc,
-                           /*IsTopScope*/ I == N - 1, *this, Invalid);
-      Nested = true;
-    }
+      if (BlockScopeInfo *BSI = dyn_cast<BlockScopeInfo>(CSI)) {
+        Invalid = !captureInBlock(BSI, Var, ExprLoc, BuildAndDiagnose, CaptureType,
+                                 DeclRefType, Nested, *this, Invalid);
+        Nested = true;
+      } else if (CapturedRegionScopeInfo *RSI = dyn_cast<CapturedRegionScopeInfo>(CSI)) {
+        Invalid = !captureInCapturedRegion(RSI, Var, ExprLoc, BuildAndDiagnose,
+                                           CaptureType, DeclRefType, Nested,
+                                           *this, Invalid);
+        Nested = true;
+      } else {
+        LambdaScopeInfo *LSI = cast<LambdaScopeInfo>(CSI);
+        Invalid =
+            !captureInLambda(LSI, Var, ExprLoc, BuildAndDiagnose, CaptureType,
+                             DeclRefType, Nested, Kind, EllipsisLoc,
+                             /*IsTopScope*/ I == N - 1, *this, Invalid);
+        Nested = true;
+      }
 
-    if (Invalid && !BuildAndDiagnose)
-      return true;
+      if (Invalid && !BuildAndDiagnose)
+        return true;
+    }
   }
   return Invalid;
 }
diff --git a/clang/lib/Sema/SemaOmpSs.cpp b/clang/lib/Sema/SemaOmpSs.cpp
@@ -1281,6 +1281,21 @@ Sema::DeclGroupPtrTy Sema::ActOnOmpSsDeclareReductionDirectiveEnd(
   return DeclReductions;
 }
 
+void Sema::ActOnOmpSsExecutableDirectiveStart() {
+  // Enter new function scope.
+  PushFunctionScope();
+  setFunctionHasBranchProtectedScope();
+  getCurFunction()->setHasOSSExecutableDirective();
+  PushExpressionEvaluationContext(
+      ExpressionEvaluationContext::PotentiallyEvaluated);
+}
+
+void Sema::ActOnOmpSsExecutableDirectiveEnd() {
+  DiscardCleanupsInEvaluationContext();
+  PopExpressionEvaluationContext();
+
+  PopFunctionScopeInfo();
+}
 
 StmtResult Sema::ActOnOmpSsExecutableDirective(ArrayRef<OSSClause *> Clauses,
     OmpSsDirectiveKind Kind, Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc) {
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
diff --git a/clang/test/OmpSs/Sema/task_body_return1.cpp b/clang/test/OmpSs/Sema/task_body_return1.cpp

Original file line number	Diff line number	Diff line change
`@@ -594,7 +594,9 @@ StmtResult Parser::ParseOmpSsDeclarativeOrExecutableDirective(`
`594`	`594`
`595`	`595`	`StmtResult AssociatedStmt;`
`596`	`596`	`if (HasAssociatedStatement) {`
	`597`	`+ Actions.ActOnOmpSsExecutableDirectiveStart();`
`597`	`598`	`AssociatedStmt = (Sema::CompoundScopeRAII(Actions), ParseStatement());`
	`599`	`+ Actions.ActOnOmpSsExecutableDirectiveEnd();`
`598`	`600`	`}`
`599`	`601`
`600`	`602`	`Clauses = StackClauses.back();`