Fix compiler assert about bounds expression already existing. (#537)

The children() method for iterating over chidren of AST cast expressions
was incorrectly including compiler-generated bounds expressions.  Child AST
nodes should be nodes that appear in the source program and additional
information shouldn't be treated as child nodes.   There were
complex IR invariants about when a bounds expression stored within a cast
expression was child AST node or not.

This change fixes the bug and simplifies the AST invariants. This fixes
issue #526. for cast expressions, there is now one entry for bounds expressions
declared as part of the program. There are separate nodes for normalized
bounds and inferred bounds.

Testing:
- Added a new regression test case for the failing case.
- Passes existing Checked C and clang Checked C tests.

include/clang/AST/Expr.h

@@ -2958,7 +2958,8 @@ class BoundsExpr : public Expr {
 /// classes).
 class CastExpr : public Expr {
 private:
-  // BOUNDS - declared bounds of the result of the cast expression
+  // BOUNDS - declared bounds of a bounds cast expression.  Null
+  // for other cast expressions.
   // NORMALIZED_BOUNDS - normalized version of declared bounds.
   // SUBEXPRBOUNDS - inferred bounds of subexpression
   enum { OP, BOUNDS, NORMALIZED_BOUNDS, SUBEXPRBOUNDS, END_EXPR = 4 };
@@ -3067,41 +3068,33 @@ class CastExpr : public Expr {
   // Iterators
 
   child_range children() {
-    // For bounds cast expression, the bounds are included as a subexpression
-    // in the AST because they are part of the source program.  For other
-    // cast expressions, the bounds are not included because they are
-    // inferred by the compiler only when needed.
+    // For a bounds cast expression, the declared bounds are included as a child
+    // expression because they are part of the source program.  Other cast expressions
+    // will not have declared bounds.  We do not include the other bounds expressions
+    // in children because they are constructed by the compiler.
     if (getStmtClass() == BoundsCastExprClass)
-      return child_range(&SubExprs[OP], &SubExprs[END_EXPR]);
+      return child_range(&SubExprs[OP], &SubExprs[BOUNDS] + 1);
     else
       return child_range(&SubExprs[OP], &SubExprs[OP] + 1);
   }
 
   const_child_range children() const {
-    // For bounds cast expression, the bounds are included as a subexpression
-    // in the AST because they are part of the source program.  For other
-    // cast expressions, the bounds are not included because they are
-    // inferred by the compiler only when needed.
     if (getStmtClass() == BoundsCastExprClass)
-      return const_child_range(&SubExprs[OP], &SubExprs[END_EXPR]);
+      return const_child_range(&SubExprs[OP], &SubExprs[BOUNDS] + 1);
     else
       return const_child_range(&SubExprs[OP], &SubExprs[OP] + 1);
   }
 
   // Checked C bounds information
 
   /// \brief Return true if the cast expression has a bounds expression
-  /// associated with it.  Depending on the type of the cast expression,
-  /// the bounds expression may be declared as part of the program or inferred
-  /// by the compiler.
+  /// declared as part of it.  This should only be true for bounds cast
+  /// expressions.
   bool hasBoundsExpr() const { return SubExprs[BOUNDS] != nullptr; }
 
   /// \brief Returns the bounds associated with the cast expression, if any.
   /// * If the cast expression is a BoundsCast expression, it is the bounds
   /// declared for the expression.
-  /// * If the cast expression is a cast to Ptr, it is the compiler-inferred
-  /// bounds of the subexpression of the cast expression.   The bounds must be
-  /// provably large enough at compile-time to contain a value of the _Ptr type.
   BoundsExpr *getBoundsExpr() {
     return cast_or_null<BoundsExpr>(SubExprs[BOUNDS]);
   }
@@ -3116,7 +3109,7 @@ class CastExpr : public Expr {
 
   bool hasNormalizedBoundsExpr() const { return SubExprs[NORMALIZED_BOUNDS] != nullptr; }
 
-  // \brief Normalized version of bounds associated with the cast expression
+  // \brief Normalized version of declared bounds for the cast expression
   // (getBoundsExpr).
   BoundsExpr *getNormalizedBoundsExpr() {
     return cast_or_null<BoundsExpr>(SubExprs[NORMALIZED_BOUNDS]);
@@ -3129,7 +3122,8 @@ class CastExpr : public Expr {
     SubExprs[NORMALIZED_BOUNDS] = E;
   }
 
-  // The inferred bounds of the subexpression of the cast expression.
+  // The inferred bounds of the subexpression of the cast expression.  This is used
+  // for dynamic bounds cast.  It is also used for implicit or C-style casts Ptr types.
   bool hasSubExprBoundsExpr() const { return SubExprs[SUBEXPRBOUNDS] != nullptr; }
   BoundsExpr *getSubExprBoundsExpr() {
     return cast_or_null<BoundsExpr>(SubExprs[SUBEXPRBOUNDS]);

lib/AST/ASTDumper.cpp

@@ -2182,13 +2182,17 @@ void ASTDumper::VisitCastExpr(const CastExpr *Node) {
   if (Node->isBoundsSafeInterface())
     OS << " BoundsSafeInterface";
 
-  if (Node->getStmtClass() != Expr::BoundsCastExprClass)
-    if (const BoundsExpr *Bounds = Node->getBoundsExpr()) {
-      dumpChild([=] {
-        OS << "Inferred Bounds";
-        dumpStmt(Bounds);
-      });
-    }
+  if (const BoundsExpr *NormalizedBounds = Node->getNormalizedBoundsExpr())
+    dumpChild([=] {
+      OS << "Normalized Bounds";
+      dumpStmt(NormalizedBounds);
+    });
+
+  if (const BoundsExpr *SubExprBounds = Node->getSubExprBoundsExpr())
+    dumpChild([=] {
+      OS << "Inferred SubExpr Bounds";
+      dumpStmt(SubExprBounds);
+    });
 }
 
 void ASTDumper::VisitDeclRefExpr(const DeclRefExpr *Node) {

lib/Sema/SemaBounds.cpp

@@ -2285,6 +2285,7 @@ namespace {
 #if TRACE_CFG
             llvm::outs() << "Visiting ";
             S->dump(llvm::outs());
+            llvm::outs().flush();
 #endif
             TraverseStmt(S, IsChecked);
          }
@@ -2566,22 +2567,22 @@ namespace {
           !E->getType()->isFunctionPointerType()) {
         bool IncludeNullTerminator =
           E->getType()->getPointeeOrArrayElementType()->isNtCheckedArrayType();
-        BoundsExpr *SrcBounds =
+        BoundsExpr *SubExprBounds =
           S.InferRValueBounds(E->getSubExpr(), IncludeNullTerminator);
-        if (SrcBounds->isUnknown()) {
+        if (SubExprBounds->isUnknown()) {
           S.Diag(E->getSubExpr()->getLocStart(),
                  diag::err_expected_bounds_for_ptr_cast)
                  << E->getSubExpr()->getSourceRange();
-          SrcBounds = S.CreateInvalidBoundsExpr();
+          SubExprBounds = S.CreateInvalidBoundsExpr();
         } else {
           BoundsExpr *TargetBounds =
             S.CreateTypeBasedBounds(E, E->getType(), false, false);
           CheckBoundsDeclAtStaticPtrCast(E, TargetBounds, E->getSubExpr(),
-                                         SrcBounds, InCheckedScope);
+                                         SubExprBounds, InCheckedScope);
         }
-        assert(SrcBounds);
-        assert(!E->getBoundsExpr());
-        E->setBoundsExpr(SrcBounds);
+        assert(SubExprBounds);
+        assert(!E->getSubExprBoundsExpr());
+        E->setSubExprBoundsExpr(SubExprBounds);
 
         if (DumpBounds)
           DumpExpression(llvm::outs(), E);

test/CheckedC/inferred-bounds/member-reference.c

@@ -448,7 +448,7 @@ _Checked void f15(struct Interop_S4 a1) {
 }
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-ImplicitCastExpr {{0x[0-9a-f]+}} '_Array_ptr<int>':'_Array_ptr<int>' <ArrayToPointerDecay>
 // CHECK: |   | `-DeclRefExpr {{0x[0-9a-f]+}} 'int _Checked[5]' lvalue Var {{0x[0-9a-f]+}} 'global_arr2' 'int _Checked[5]'

test/CheckedC/inferred-bounds/ptr-cast.c

@@ -24,7 +24,7 @@ void f1(void) {
   _Ptr<int> p = &x;
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-UnaryOperator {{0x[0-9a-f]+}} '_Array_ptr<int>' prefix '&'
 // CHECK: |   | `-DeclRefExpr {{0x[0-9a-f]+}} 'int' lvalue Var {{0x[0-9a-f]+}} 'x' 'int'
@@ -38,7 +38,7 @@ void f1(void) {
   p = &y;
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-UnaryOperator {{0x[0-9a-f]+}} '_Array_ptr<int>' prefix '&'
 // CHECK: |   | `-DeclRefExpr {{0x[0-9a-f]+}} 'int' lvalue Var {{0x[0-9a-f]+}} 'y' 'int'
@@ -63,7 +63,7 @@ void f2(_Array_ptr<int> p : count(5)) {
   _Ptr<int> x = &p[2];
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-ImplicitCastExpr {{0x[0-9a-f]+}} '_Array_ptr<int>' <LValueToRValue>
 // CHECK: |   | `-DeclRefExpr {{0x[0-9a-f]+}} '_Array_ptr<int>' lvalue ParmVar {{0x[0-9a-f]+}} 'p' '_Array_ptr<int>'
@@ -93,7 +93,7 @@ void f3(void) {
    _Ptr<int> p = &v.f;
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-UnaryOperator {{0x[0-9a-f]+}} '_Array_ptr<int>' prefix '&'
 // CHECK: |   | `-MemberExpr {{0x[0-9a-f]+}} 'int' lvalue .f {{0x[0-9a-f]+}}
@@ -117,7 +117,7 @@ void f4(struct S b _Checked[9]) {
   _Ptr<int> p = &((*b).f);
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-UnaryOperator {{0x[0-9a-f]+}} '_Array_ptr<int>' prefix '&'
 // CHECK: |   | `-MemberExpr {{0x[0-9a-f]+}} 'int' lvalue .f {{0x[0-9a-f]+}}
@@ -151,7 +151,7 @@ void f5(struct S arr _Checked[][12] : count(len), int i, int j, int len) {
   p = &(arr[i][j].f);
 
 // CHECK: ImplicitCastExpr {{0x[0-9a-f]+}} '_Ptr<int>' <BitCast>
-// CHECK: |-Inferred Bounds
+// CHECK: |-Inferred SubExpr Bounds
 // CHECK: | `-RangeBoundsExpr {{0x[0-9a-f]+}} 'NULL TYPE'
 // CHECK: |   |-UnaryOperator {{0x[0-9a-f]+}} '_Array_ptr<int>' prefix '&'
 // CHECK: |   | `-MemberExpr {{0x[0-9a-f]+}} 'int' lvalue .f {{0x[0-9a-f]+}}

test/CheckedC/regression-cases/bug526_double_traversal.c

@@ -0,0 +1,29 @@
+//
+// This is a regression test case for 
+// https://github.com/Microsoft/checkedc-clang/issues/526
+//
+// The compiler was crashing with an internal assertion that an expression
+// to which a bounds check was going to be added already had a bounds 
+// check.
+//
+// The problem is that the compiler was traversing the 1st argument of a
+// _Dynamic_bounds_cast operation twice.  The compiler inferred bounds
+// that use the 1st argument.  It then attached them to the AST.  There
+// was a lack of clarity in the IR and the inferred bounds were also
+// traversed, causing the assert.
+//
+// RUN: %clang -cc1 -verify %s
+// expected-no-diagnostics
+
+struct obj {
+  _Array_ptr<_Nt_array_ptr<char>> names : count(len);
+  unsigned int len;
+};
+
+void f(const struct obj *object ) {
+   unsigned int i = 0;
+   _Nt_array_ptr<const char> t : count(0) = 
+      _Dynamic_bounds_cast<_Nt_array_ptr<const char>>(object->names[i], 
+                                                      count(0));
+ }
+