Check return value bounds

clang/include/clang/AST/ExprUtils.h

@@ -145,6 +145,10 @@ class ExprUtil {
   // pointer). Returns false if E is nullptr.
   static bool ReadsMemoryViaPointer(Expr *E, bool IncludeAllMemberExprs = false);
 
+  // IsReturnValueExpr return true if the expression E is a _Return_value
+  // expression.
+  static bool IsReturnValueExpr(Expr *E);
+
   // FindLValue returns true if the given lvalue expression occurs in E.
   static bool FindLValue(Sema &S, Expr *LValue, Expr *E);
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -11386,6 +11386,10 @@ def err_bounds_type_annotation_lost_checking : Error<
     "argument has unknown bounds, bounds expected because the "
     "%ordinal0 parameter has bounds">;
 
+  def err_expected_bounds_for_return : Error<
+    "return value has unknown bounds, bounds expected because the "
+    "function %0 has bounds">;
+
   def err_initializer_expected_with_bounds : Error<
     "automatic variable %0 with bounds must have initializer">;
 
@@ -11619,8 +11623,29 @@ def err_bounds_type_annotation_lost_checking : Error<
   def error_static_cast_bounds_invalid : Error<
     "cast source bounds are too narrow for %0">;
 
+  def error_return_bounds_invalid : Error<
+    "return value bounds do not imply declared return bounds for %0">;
+
+  def error_return_bounds_unprovable: Error<
+    "it is not possible to prove that return value bounds "
+    "imply declared return bounds for %0">;
+
+  def warn_return_bounds_invalid: Warning<
+    "cannot prove return value bounds imply declared return bounds for %0">,
+    InGroup<CheckBoundsDeclsUnchecked>;
+
+  def warn_checked_scope_return_bounds_invalid : Warning<
+    "cannot prove return value bounds imply declared return bounds for %0">,
+    InGroup<CheckBoundsDeclsChecked>;
+
+  def note_declared_return_bounds : Note<
+    "(expanded) declared return bounds are '%0'">;
+
+  def note_inferred_return_bounds : Note<
+    "(expanded) inferred return value bounds are '%0'">;
+
   def error_out_of_bounds_access : Error<
-    "out-of-bounds %select{||memory access|base value}0">;
+    "out-of-bounds %select{|||memory access|base value}0">;
 
   def note_source_bounds_empty : Note<"source bounds are an empty range">;
 
@@ -11631,21 +11656,22 @@ def err_bounds_type_annotation_lost_checking : Error<
   def note_destination_bounds_invalid : Note<"destination bounds are an invalid range">;
 
   def note_bounds_too_narrow : Note<
-    "%select{destination bounds are|target bounds are|memory accessed is|"
-    "struct/union pointed to by base is}0 wider than the "
-    "%select{source|source||}0 bounds">;
+    "%select{destination bounds are|target bounds are|declared return bounds are|"
+    "memory accessed is|struct/union pointed to by base is|}0 wider "
+    "than the %select{source|source|return value|source|source}0 bounds">;
 
   def note_lower_out_of_bounds : Note<
-    "%select{destination lower bound is|target lower bound is|accesses memory|"
-    "base value is}0 below %select{source|source|the|its}0 lower bound">;
+    "%select{destination lower bound is|target lower bound is|"
+    "declared return lower bound is|accesses memory|base value is}0 "
+    "below %select{source|source|return value|the|its}0 lower bound">;
 
   def note_upper_out_of_bounds : Note<
     "%select{destination upper bound is|target upper bound is|"
-    "accesses memory at or|base value is}0 "
-    "above %select{source|source|the|its}0 upper bound">;
+    "declared return upper bound is|accesses memory at or|base value is}0 "
+    "above %select{source|source|return value|the|its}0 upper bound">;
 
    def note_bounds_partially_overlap : Note<
-    "%select{||accesses memory that|struct/union pointed to by base value}0 is "
+    "%select{|||accesses memory that|struct/union pointed to by base value}0 is "
     "only partially in bounds">;
 
   def no_prototype_generic_function : Error<

clang/lib/AST/ExprUtils.cpp

@@ -306,6 +306,13 @@ bool ExprUtil::ReadsMemoryViaPointer(Expr *E, bool IncludeAllMemberExprs) {
   }
 }
 
+bool ExprUtil::IsReturnValueExpr(Expr *E) {
+  BoundsValueExpr *BVE = dyn_cast_or_null<BoundsValueExpr>(E);
+  if (!BVE)
+    return false;
+  return BVE->getKind() == BoundsValueExpr::Kind::Return;
+}
+
 namespace {
   class FindLValueHelper : public RecursiveASTVisitor<FindLValueHelper> {
     private:

clang/lib/Sema/SemaBounds.cpp

@@ -652,8 +652,20 @@ namespace {
     uint64_t PointerWidth;
     Stmt *Body;
     CFG *Cfg;
-    BoundsExpr *ReturnBounds; // return bounds expression for enclosing
-                              // function, if any.
+
+    // Declaration for enclosing function. Having this here allows us to emit
+    // the name of the function in any diagnostic message when checking return
+    // bounds.
+    FunctionDecl *FunctionDeclaration;
+    // Return value expression for enclosing function, if any. Having this
+    // here allows us to avoid reconstructing a return value for each
+    // return statement.
+    BoundsValueExpr *ReturnVal;
+    // Expanded declared return bounds expression for enclosing function, if
+    // any. Having this here allows us to avoid re-expanding the return bounds
+    // for each return statement.
+    BoundsExpr *ReturnBounds;
+
     ASTContext &Context;
     std::pair<ComparisonSet, ComparisonSet> &Facts;
 
@@ -946,6 +958,7 @@ namespace {
     enum class ProofStmtKind : unsigned {
       BoundsDeclaration,
       StaticBoundsCast,
+      ReturnStmt,
       MemoryAccess,
       MemberArrowBase
     };
@@ -1366,6 +1379,11 @@ namespace {
             ExprUtil::ReadsMemoryViaPointer(E2, true))
           return false;
 
+        // If E1 or E2 is a _Return_value expression, we skip since we cannot
+        // determine the set of variables that occur in these expressions.
+        if (ExprUtil::IsReturnValueExpr(E1) || ExprUtil::IsReturnValueExpr(E2))
+          return false;
+
         bool HasFreeVariables = false;
         EqualExprTy Vars1 = CollectVariableSet(S, E1);
         EqualExprTy Vars2 = CollectVariableSet(S, E2);
@@ -1943,6 +1961,9 @@ namespace {
       return false;
     }
 
+    // Methods to try to prove that an inferred bounds expression implies
+    // the validity of a target bounds expression.
+
     // Try to prove that SrcBounds implies the validity of DeclaredBounds.
     //
     // If Kind is StaticBoundsCast, check whether a static cast between Ptr
@@ -2020,6 +2041,80 @@ namespace {
       return ProofResult::Maybe;
     }
 
+    // Try to prove that RetExprBounds implies the validity of ReturnBounds
+    // (the declared return bounds for the enclosing function).
+    ProofResult ProveReturnBoundsValidity(Expr *RetExpr,
+                                          BoundsExpr *RetExprBounds,
+                                          const EquivExprSets EQ,
+                                          const EqualExprTy G,
+                                          ProofFailure &Cause,
+                                          FreeVariableListTy &FreeVariables) {
+      // Check some basic properties of the declared ReturnBounds and the
+      // source RetExprBounds. Even though these checks will also be done
+      // in ProveBoundsDeclValidity, if any of these checks result in an
+      // early return, we can avoid constructing a modified EquivExprs set
+      // that records equality between RetVal and RetExpr.
+
+      // Null declared return bounds or declared return bounds(unknown) 
+      // implied by any other bounds.
+      if (!ReturnBounds || ReturnBounds->isUnknown())
+        return ProofResult::True;
+
+      // Ignore invalid bounds.
+      if (RetExprBounds->isInvalid() || ReturnBounds->isInvalid())
+        return ProofResult::True;
+
+      // Return expression bounds(any) implies any declared return bounds.
+      if (RetExprBounds->isAny())
+        return ProofResult::True;
+
+      // Return expression bounds(unknown) cannot imply any non-unknown
+      // declared return bounds.
+      if (RetExprBounds->isUnknown())
+        return ProofResult::False;
+
+      // Record equality between ReturnVal and all expressions that produce
+      // the same value as RetExpr. This allows ProveBoundsDeclValidity to
+      // validate bounds that depend on the return value of the function.
+      // For example, if the declared function bounds are count(1), then the
+      // expanded ReturnBounds will be bounds(RetVal, RetVal + 1).
+      EquivExprSets EquivExprs = EQ;
+
+      // Determine the set of expressions that produce the same value as
+      // RetExpr.
+      // If G (the set of expressions that produce the same value as RetExpr)
+      // is empty, then RetExpr may be an expression that is not allowed to
+      // be recorded in State.EquivExprs (e.g. an expression that reads memory
+      // via a pointer). The EquivExprs set that we construct here is temporary
+      // and is only used to check the return bounds for one return statement,
+      // so we can add RetExpr to EquivExprs in this case.
+      EqualExprTy RetSameValue = G;
+      if (RetSameValue.size() == 0)
+        RetSameValue.push_back(RetExpr);
+
+      bool FoundRetExpr = false;
+      for (auto F = EquivExprs.begin(); F != EquivExprs.end(); ++F) {
+        if (DoExprSetsIntersect(*F, RetSameValue)) {
+          // Add all expressions in RetSameValue to F that are not already in F.
+          for (Expr *E : RetSameValue)
+            if (!EqualExprsContainsExpr(*F, E))
+              F->push_back(E);
+          F->push_back(ReturnVal);
+          FoundRetExpr = true;
+          break;
+        }
+      }
+      if (!FoundRetExpr) {
+        EqualExprTy F = RetSameValue;
+        F.push_back(ReturnVal);
+        EquivExprs.push_back(F);
+      }
+
+      return ProveBoundsDeclValidity(ReturnBounds, RetExprBounds, Cause,
+                                     &EquivExprs, FreeVariables,
+                                     ProofStmtKind::ReturnStmt);
+    }
+
     // CompareNormalizedBounds returns true if SrcBounds implies DeclaredBounds
     // after applying certain transformations to the upper bound expressions
     // of both bounds.
@@ -2569,14 +2664,16 @@ namespace {
 
 
   public:
-    CheckBoundsDeclarations(Sema &SemaRef, PrepassInfo &Info, Stmt *Body, CFG *Cfg, BoundsExpr *ReturnBounds, std::pair<ComparisonSet, ComparisonSet> &Facts) : S(SemaRef),
+    CheckBoundsDeclarations(Sema &SemaRef, PrepassInfo &Info, Stmt *Body, CFG *Cfg, FunctionDecl *FD, std::pair<ComparisonSet, ComparisonSet> &Facts) : S(SemaRef),
       DumpBounds(SemaRef.getLangOpts().DumpInferredBounds),
       DumpState(SemaRef.getLangOpts().DumpCheckingState),
       DumpSynthesizedMembers(SemaRef.getLangOpts().DumpSynthesizedMembers),
       PointerWidth(SemaRef.Context.getTargetInfo().getPointerWidth(0)),
       Body(Body),
       Cfg(Cfg),
-      ReturnBounds(ReturnBounds),
+      FunctionDeclaration(FD),
+      ReturnVal(nullptr),
+      ReturnBounds(nullptr),
       Context(SemaRef.Context),
       Facts(Facts),
       BoundsWideningAnalyzer(BoundsWideningAnalysis(SemaRef, Cfg,
@@ -2585,7 +2682,16 @@ namespace {
                                                     Info.CheckedScopeMap)),
       AbstractSetMgr(AbstractSetManager(SemaRef, Info.VarUses)),
       BoundsSiblingFields(Info.BoundsSiblingFields),
-      IncludeNullTerminator(false) {}
+      IncludeNullTerminator(false) {
+        if (FD) {
+          ReturnVal =
+            new (S.Context) BoundsValueExpr(SourceLocation(),
+                                            FD->getReturnType(),
+                                            BoundsValueExpr::Kind::Return);
+          ReturnBounds =
+            BoundsUtil::ExpandToRange(S, ReturnVal, FD->getBoundsExpr());
+        }
+      }
 
     CheckBoundsDeclarations(Sema &SemaRef, PrepassInfo &Info, std::pair<ComparisonSet, ComparisonSet> &Facts) : S(SemaRef),
       DumpBounds(SemaRef.getLangOpts().DumpInferredBounds),
@@ -2594,6 +2700,8 @@ namespace {
       PointerWidth(SemaRef.Context.getTargetInfo().getPointerWidth(0)),
       Body(nullptr),
       Cfg(nullptr),
+      FunctionDeclaration(nullptr),
+      ReturnVal(nullptr),
       ReturnBounds(nullptr),
       Context(SemaRef.Context),
       Facts(Facts),
@@ -3922,14 +4030,13 @@ namespace {
         return ResultBounds;
 
       // Check the return value if it exists.
-      Check(RetValue, CSS, State);
+      BoundsExpr *RetValueBounds = Check(RetValue, CSS, State);
 
-      if (!ReturnBounds)
-        return ResultBounds;
+      // Check that the return expression bounds imply the return bounds.
+      ValidateReturnBounds(RS, RetValue, RetValueBounds, State.EquivExprs,
+                           State.SameValue, CSS);
 
-      // TODO: Actually check that the return expression bounds imply the 
-      // return bounds.
-      // TODO: Also check that any parameters used in the return bounds are
+      // TODO: Check that any parameters used in the return bounds are
       // unmodified.
       return ResultBounds;
     }
@@ -4500,6 +4607,66 @@ namespace {
       return Loc;
     }
 
+    // ValidateReturnBounds checks that the observed bounds for the return
+    // value RetExpr imply the declared bounds for the enclosing function.
+    void ValidateReturnBounds(ReturnStmt *RS, Expr *RetExpr,
+                              BoundsExpr *RetExprBounds,
+                              const EquivExprSets EquivExprs,
+                              const EqualExprTy RetSameValue,
+                              CheckedScopeSpecifier CSS) {
+      ProofFailure Cause;
+      FreeVariableListTy FreeVars;
+      ProofResult Result = ProveReturnBoundsValidity(RetExpr, RetExprBounds,
+                                                     EquivExprs, RetSameValue,
+                                                     Cause, FreeVars);
+
+      if (Result == ProofResult::True)
+        return;
+
+      if (RetExprBounds->isUnknown()) {
+        DiagnoseUnknownReturnBounds(RS, RetExpr);
+        return;
+      }
+
+      // Which diagnostic message to print?
+      unsigned DiagId =
+          (Result == ProofResult::False)
+              ? (TestFailure(Cause, ProofFailure::HasFreeVariables)
+                     ? diag::error_return_bounds_unprovable
+                     : diag::error_return_bounds_invalid)
+              : (CSS != CheckedScopeSpecifier::CSS_Unchecked
+                     ? diag::warn_checked_scope_return_bounds_invalid
+                     : diag::warn_return_bounds_invalid);
+
+      SourceLocation Loc = RetExpr->getBeginLoc();
+      S.Diag(Loc, DiagId) << FunctionDeclaration << RS->getSourceRange();
+      if (Result == ProofResult::False)
+        ExplainProofFailure(Loc, Cause, ProofStmtKind::ReturnStmt);
+
+      if (TestFailure(Cause, ProofFailure::HasFreeVariables))
+        DiagnoseFreeVariables(diag::note_free_variable_decl_or_inferred, Loc,
+                              FreeVars);
+
+      SourceLocation DeclaredLoc = FunctionDeclaration->getLocation();
+      S.Diag(DeclaredLoc, diag::note_declared_return_bounds)
+        << ReturnBounds << ReturnBounds->getSourceRange();
+      S.Diag(Loc, diag::note_inferred_return_bounds)
+        << RetExprBounds << RetExprBounds->getSourceRange();
+    }
+
+    // DiagnoseUnknownReturnBounds emits an error message at the return
+    // statement RS and return expression RetExpr whose inferred bounds are
+    // unknown, where the enclosing function has declared bounds ReturnBounds.
+    void DiagnoseUnknownReturnBounds(ReturnStmt *RS, Expr *RetExpr) {
+      SourceLocation Loc = RetExpr->getBeginLoc();
+      S.Diag(Loc, diag::err_expected_bounds_for_return)
+        << FunctionDeclaration << RS->getSourceRange();
+
+      SourceLocation DeclaredLoc = FunctionDeclaration->getLocation();
+      S.Diag(DeclaredLoc, diag::note_declared_return_bounds)
+        << ReturnBounds << ReturnBounds->getSourceRange();
+    }
+
     // Methods to update the checking state.
 
     // UpdateAfterAssignment updates the checking state after the lvalue
@@ -6134,7 +6301,7 @@ void Sema::CheckFunctionBodyBoundsDecls(FunctionDecl *FD, Stmt *Body) {
   BO.AddLifetime = true;
   BO.AddNullStmt = true;
   std::unique_ptr<CFG> Cfg = CFG::buildCFG(nullptr, Body, &getASTContext(), BO);
-  CheckBoundsDeclarations Checker(*this, Info, Body, Cfg.get(), FD->getBoundsExpr(), EmptyFacts);
+  CheckBoundsDeclarations Checker(*this, Info, Body, Cfg.get(), FD, EmptyFacts);
   if (Cfg != nullptr) {
     AvailableFactsAnalysis Collector(*this, Cfg.get());
     Collector.Analyze();

clang/test/3C/functionDeclEnd.c

@@ -4,6 +4,16 @@
 // RUN: 3c -base-dir=%S -addcr -alltypes %s -- | %clang -c -fcheckedc-extension -x c -o /dev/null -
 // RUN: 3c -base-dir=%S -alltypes -output-dir=%t.checked %s --
 // RUN: 3c -base-dir=%t.checked -alltypes %t.checked/functionDeclEnd.c -- | diff %t.checked/functionDeclEnd.c -
+// XFAIL: *
+
+// TODO: checkedc-clang issue 1147. This test fails due to the compiler
+// checking that the inferred bounds for the return value of a function
+// imply the declared bounds for the function. The following functions in
+// this test file return expressions with unknown bounds, which do not imply
+// the function's declared bounds:
+// 1. test3
+// 2. test7
+// 3. test8
 
 // Tests for issue 392. When rewriting function prototypes sometimes code
 // falling between the start of the definition and the end of the prototype