From 18a77c6d41c62bbcd62148aa1cba8584321a203d Mon Sep 17 00:00:00 2001
From: David Tarditi <dtarditi@microsoft.com>
Date: Fri, 2 Nov 2018 18:13:09 -0700
Subject: [PATCH] Fix compiler assert. (#585)

This change fixes the compiler assert reported in Github issue #575. We are inserting temporary variables for strings so that we can describe their bounds.  The assert was checking that we don't compute a temporary variable more than once in an expression.

The problem was that we were copying an expression that creates and binds a temporary into the bounds.   The bounds is used at runtime in the dynamic_bounds_cast.  This causes the temporary to be computed twice, which is what the assert is protecting against.  Since we've already computed the value into a temporary, the fix is to replace the binding with a read of the temporary.

Testing:
- Added simple repro case that caused a compiler crash.
- Added a runtime test that will be covered by a PR to the Checked C repo
- Passed testing locally for Window x64.
- Passed automated testing for Linux.
---
 lib/AST/Expr.cpp                              |  3 --
 lib/Sema/SemaBounds.cpp                       | 34 +++++++++++++++++++
 .../regression-cases/bug_575_string_literal.c | 14 ++++++++
 3 files changed, 48 insertions(+), 3 deletions(-)
 create mode 100644 test/CheckedC/regression-cases/bug_575_string_literal.c

diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index 70458073549f..d8d273569b41 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -1668,9 +1668,6 @@ namespace {
     if (CXXBindTemporaryExpr *Binder = dyn_cast<CXXBindTemporaryExpr>(expr))
       expr = Binder->getSubExpr();
 
-    if (CHKCBindTemporaryExpr *Temp = dyn_cast<CHKCBindTemporaryExpr>(expr))
-      expr = Temp->getSubExpr();
-
     return expr;
   }
 }
diff --git a/lib/Sema/SemaBounds.cpp b/lib/Sema/SemaBounds.cpp
index c7ad5822a149..73fa6aa1f4cc 100644
--- a/lib/Sema/SemaBounds.cpp
+++ b/lib/Sema/SemaBounds.cpp
@@ -404,6 +404,32 @@ namespace {
 }
 #endif
 
+// Convert all temporary bindings in an expression to uses of the values
+// produced by a binding.   This should be done for bounds expressions that
+// are used in runtime checks.  That way we don't try to recompute a
+// temporary multiple times in an expression.
+namespace {
+  class PruneTemporaryHelper : public TreeTransform<PruneTemporaryHelper> {
+    typedef TreeTransform<PruneTemporaryHelper> BaseTransform;
+
+
+  public:
+    PruneTemporaryHelper(Sema &SemaRef) :
+      BaseTransform(SemaRef) { }
+
+    ExprResult TransformCHKCBindTemporaryExpr(CHKCBindTemporaryExpr *E) {
+      return new (SemaRef.Context) BoundsValueExpr(SourceLocation(), E);
+    }
+  };
+
+  Expr *PruneTemporaryBindings(Sema &SemaRef, Expr *E) {
+    Sema::ExprSubstitutionScope Scope(SemaRef); // suppress diagnostics
+    ExprResult R = PruneTemporaryHelper(SemaRef).TransformExpr(E);
+    assert(!R.isInvalid());
+    return R.get();
+  }
+}
+
 namespace {
   // Class for inferring bounds expressions for C expressions.
 
@@ -2664,6 +2690,14 @@ namespace {
         }
 
         assert(NormalizedBounds);
+
+        // These bounds will be computed and tested at runtime.  Don't
+        // recompute any expressions computed to temporaries already.
+        NormalizedBounds =
+          cast<BoundsExpr>(PruneTemporaryBindings(S, NormalizedBounds));
+        SubExprBounds =
+          cast<BoundsExpr>(PruneTemporaryBindings(S, SubExprBounds));
+
         E->setNormalizedBoundsExpr(NormalizedBounds);
         E->setSubExprBoundsExpr(SubExprBounds);
         if (DumpBounds)
diff --git a/test/CheckedC/regression-cases/bug_575_string_literal.c b/test/CheckedC/regression-cases/bug_575_string_literal.c
new file mode 100644
index 000000000000..907afc7892b8
--- /dev/null
+++ b/test/CheckedC/regression-cases/bug_575_string_literal.c
@@ -0,0 +1,14 @@
+// This is a regression test case for 
+// https://github.com/Microsoft/checkedc-clang/issues/575
+//
+// This test checks that the compiler does not crash on a dynamic bounds
+// cast whose argument is a string literal.
+//
+// RUN: %clang -cc1 -verify -fcheckedc-extension %s
+// expected-no-diagnostics
+
+int main(void) {
+  _Nt_array_ptr<const char> str_with_len : count(2) = 
+    _Dynamic_bounds_cast<_Nt_array_ptr<const char>>(("\n"), count(2));
+  return 0;
+}