Update bounds checking notes (#1166)

* Return the Bounds argument from ReplaceLValueInBounds if Bounds is unknown or any

* Only store the first expression with unknown bounds assigned to an lvalue in A

Only update BlameAssignments and UnknownSrcBounds if the assignment actually changed the bounds of LValueAbstractSet

* Remove expected note about the expression 'b' with unknown bounds assigned to 'a'

* Add test to bounds-decl-checking.c testing notes for multiple assignments

* Add tests for bounds warnings

clang/lib/Sema/BoundsUtils.cpp

@@ -186,6 +186,8 @@ BoundsExpr *BoundsUtil::ExpandToRange(Sema &S, VarDecl *D, BoundsExpr *B) {
 BoundsExpr *BoundsUtil::ReplaceLValueInBounds(Sema &S, BoundsExpr *Bounds,
                                               Expr *LValue, Expr *OriginalValue,
                                               CheckedScopeSpecifier CSS) {
+  if (Bounds->isUnknown() || Bounds->isAny())
+    return Bounds;
   Expr *Replaced = ReplaceLValue(S, Bounds, LValue, OriginalValue, CSS);
   if (!Replaced)
     return CreateBoundsUnknown(S);

clang/lib/Sema/SemaBounds.cpp

@@ -549,12 +549,12 @@ namespace {
       llvm::DenseMap<const AbstractSet *, std::pair<BoundsExpr *, Expr *>> LostLValues;
 
       // UnknownSrcBounds maps an AbstractSet A whose observed bounds are
-      // unknown to a set of expressions with unknown bounds that have been
-      // assigned to A.
+      // unknown to the first expression with unknown bounds (if any) that
+      // has been assigned to an lvalue expression in A.
       //
       // UnknownSrcBounds is used to emit notes to provide more context to the
       // user when diagnosing unknown bounds errors.
-      llvm::DenseMap<const AbstractSet *, SmallVector<Expr *, 4>> UnknownSrcBounds;
+      llvm::DenseMap<const AbstractSet *, Expr *> UnknownSrcBounds;
 
       // BlameAssignments maps an AbstractSet A to an expression in a top-level
       // CFG statement that last updates any variable used in the declared
@@ -4521,14 +4521,14 @@ namespace {
 
       // The observed bounds of A are unknown because at least one expression
       // e with unknown bounds was assigned to an lvalue expression in A.
+      // Emit a note for the first expression with unknown bounds that was
+      // assigned to A (this expression is the only one that is tracked in
+      // State.UnknownSrcBounds).
       auto BlameSrcIt = State.UnknownSrcBounds.find(A);
       if (BlameSrcIt != State.UnknownSrcBounds.end()) {
-        SmallVector<Expr *, 4> UnknownSources = BlameSrcIt->second;
-        for (auto I = UnknownSources.begin(); I != UnknownSources.end(); ++I) {
-          Expr *Src = *I;
-          S.Diag(Src->getBeginLoc(), diag::note_unknown_source_bounds)
+        Expr *Src = BlameSrcIt->second;
+        S.Diag(Src->getBeginLoc(), diag::note_unknown_source_bounds)
             << Src << A->GetRepresentative() << Src->getSourceRange();
-        }
       }
     }
 
@@ -4860,8 +4860,10 @@ namespace {
       // If LValue has target bounds, the initial observed bounds of LValue
       // are SrcBounds. These bounds will be updated to account for any uses
       // of LValue below.
+      BoundsExpr *PrevLValueBounds = nullptr;
       if (HasTargetBounds) {
         LValueAbstractSet = AbstractSetMgr.GetOrCreateAbstractSet(LValue);
+        PrevLValueBounds = State.ObservedBounds[LValueAbstractSet];
         State.ObservedBounds[LValueAbstractSet] = SrcBounds;
 
         // In an unchecked scope, if an expression with checked pointer type
@@ -4880,14 +4882,6 @@ namespace {
         }
       }
 
-      // If Src initially has unknown bounds (before making any lvalue
-      // replacements), use Src to explain bounds checking errors that
-      // can occur when validating the bounds context.
-      if (HasTargetBounds) {
-        if (SrcBounds->isUnknown())
-          State.UnknownSrcBounds[LValueAbstractSet].push_back(Src);
-      }
-
       // Adjust ObservedBounds to account for any uses of LValue in the bounds.
       for (auto const &Pair : State.ObservedBounds) {
         const AbstractSet *A = Pair.first;
@@ -4921,10 +4915,22 @@ namespace {
           BoundsUtil::ReplaceLValueInBounds(S, SrcBounds, LValue,
                                             OriginalValue, CSS);
 
-      // Record that E updates the observed bounds of LValue.
-      if (HasTargetBounds)
+      // If the updated observed bounds of LValue are different than the
+      // previous observed bounds of LValue, record that E updates the
+      // observed bounds of LValue.
+      // We can check this cheaply because ReplaceLValueInBounds returns
+      // PrevLValueBounds as AdjustedSrcBounds if the previous observed
+      // bounds of LValue were not adjusted.
+      if (HasTargetBounds && PrevLValueBounds != AdjustedSrcBounds) {
         State.BlameAssignments[LValueAbstractSet] = E;
 
+        // If the original bounds of Src (before replacing LValue) were
+        // unknown, record that the expression Src with unknown bounds was
+        // assigned to LValue.
+        if (SrcBounds->isUnknown())
+          State.UnknownSrcBounds[LValueAbstractSet] = Src;
+      }
+
       // If the initial source bounds were not unknown, but they are unknown
       // after replacing uses of LValue, then the assignment to LValue caused
       // the source bounds (which are the observed bounds for LValue) to be

clang/test/CheckedC/inferred-bounds/bounds-context.c

@@ -1138,7 +1138,6 @@ void multiple_assign1(
   // Observed bounds context after assignments: { a => bounds(unknown), b => bounds(unknown) }
   len = 0, a = b; // expected-error {{inferred bounds for 'a' are unknown after assignment}} \
                   // expected-note {{lost the value of the expression 'len' which is used in the (expanded) inferred bounds 'bounds(a, a + len)' of 'a'}} \
-                  // expected-note {{assigned expression 'b' with unknown bounds to 'a'}} \
                   // expected-error {{inferred bounds for 'b' are unknown after assignment}} \
                   // expected-note {{lost the value of the expression 'len' which is used in the (expanded) inferred bounds 'bounds(b, b + len)' of 'b'}}
   // CHECK: Statement S:

clang/test/CheckedC/static-checking/bounds-decl-checking.c

@@ -817,6 +817,35 @@ void f96(_Nt_array_ptr<char> p : bounds(p, (p + (len + 4)) + 4), int len) {
                                                               // expected-note {{(expanded) inferred bounds are 'bounds(p, (p + (len + 4)) + 4)'}}
 }
 
+//
+// Test diagnostic behavior for bounds checking across multiple assignments.
+//
+
+void f97(_Array_ptr<int> p : count(i), // expected-note 4 {{(expanded) declared bounds are 'bounds(p, p + i)'}}
+         unsigned int i, 
+         _Array_ptr<int> unknwn1,
+         _Array_ptr<int> unknwn2) {
+  p = unknwn1, p = p; // expected-error {{inferred bounds for 'p' are unknown after assignment}} \
+                      // expected-note {{assigned expression 'unknwn1' with unknown bounds to 'p'}}
+
+  p = unknwn1, p++; // expected-error {{inferred bounds for 'p' are unknown after assignment}} \
+                    // expected-note {{assigned expression 'unknwn1' with unknown bounds to 'p'}}
+
+  p = unknwn1, p = unknwn2; // expected-error {{inferred bounds for 'p' are unknown after assignment}} \
+                            // expected-note {{assigned expression 'unknwn1' with unknown bounds to 'p'}}
+
+  p++, i = 0, p = unknwn1; // expected-error {{inferred bounds for 'p' are unknown after assignment}} \
+                           // expected-note {{lost the value of the expression 'i' which is used in the (expanded) inferred bounds 'bounds(p - 1, p - 1 + i)' of 'p'}}
+}
+
+void f98(_Array_ptr<int> p : count(i), unsigned int i) { // expected-note 2 {{(expanded) declared bounds are 'bounds(p, p + i)'}}
+  i++, p += 2; // expected-warning {{cannot prove declared bounds for 'p' are valid after assignment}} \
+               // expected-note {{(expanded) inferred bounds are 'bounds(p - 2, p - 2 + i)'}}
+
+  p += 2, i++; // expected-warning {{cannot prove declared bounds for 'p' are valid after increment}} \
+               // expected-note {{(expanded) inferred bounds are 'bounds(p - 2, p - 2 + i - 1U)'}}
+}
+
 //
 // Test validating bounds for unchecked pointer with bounds-safe interfaces
 // in unchecked and checked scopes.
@@ -825,7 +854,7 @@ void f96(_Nt_array_ptr<char> p : bounds(p, (p + (len + 4)) + 4), int len) {
 _Unchecked extern int *get_unchecked(_Array_ptr<int> a);
 extern _Array_ptr<int> get_checked(unsigned int i) : count(i);
 
-_Unchecked void f97(int *p : count(i), // expected-note 8 {{(expanded) declared bounds are 'bounds(p, p + i)'}}
+_Unchecked void f99(int *p : count(i), // expected-note 8 {{(expanded) declared bounds are 'bounds(p, p + i)'}}
                     int *q : bounds(unknown),
                     _Array_ptr<int> r,
                     unsigned int i) {