From bb061491316bbd516a7551fe36128ead0935010d Mon Sep 17 00:00:00 2001
From: Richard Smith <richard@metafoo.co.uk>
Date: Wed, 30 Oct 2019 13:32:52 -0700
Subject: [PATCH] Fix __attribute__((enable_if)) to treat arguments with
 side-effects as non-constant.

We previously failed the entire condition evaluation if an unmodeled
side-effect was encountered in an argument, even if that argument was
unused in the attribute's condition.
---
 clang/lib/AST/ExprConstant.cpp   | 39 +++++++++++++++++++++++---------
 clang/test/SemaCXX/enable_if.cpp | 30 +++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 7ed082185670..ecd9eda76488 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -1039,10 +1039,13 @@ namespace {
     /// cleanups would have had a side-effect, note that as an unmodeled
     /// side-effect and return false. Otherwise, return true.
     bool discardCleanups() {
-      for (Cleanup &C : CleanupStack)
-        if (C.hasSideEffect())
-          if (!noteSideEffect())
-            return false;
+      for (Cleanup &C : CleanupStack) {
+        if (C.hasSideEffect() && !noteSideEffect()) {
+          CleanupStack.clear();
+          return false;
+        }
+      }
+      CleanupStack.clear();
       return true;
     }
 
@@ -14340,27 +14343,41 @@ bool Expr::EvaluateWithSubstitution(APValue &Value, ASTContext &Ctx,
     assert(MD && "Don't provide `this` for non-methods.");
     assert(!MD->isStatic() && "Don't provide `this` for static methods.");
 #endif
-    if (EvaluateObjectArgument(Info, This, ThisVal))
+    if (!This->isValueDependent() &&
+        EvaluateObjectArgument(Info, This, ThisVal) &&
+        !Info.EvalStatus.HasSideEffects)
       ThisPtr = &ThisVal;
-    if (Info.EvalStatus.HasSideEffects)
-      return false;
+
+    // Ignore any side-effects from a failed evaluation. This is safe because
+    // they can't interfere with any other argument evaluation.
+    Info.EvalStatus.HasSideEffects = false;
   }
 
   ArgVector ArgValues(Args.size());
   for (ArrayRef<const Expr*>::iterator I = Args.begin(), E = Args.end();
        I != E; ++I) {
     if ((*I)->isValueDependent() ||
-        !Evaluate(ArgValues[I - Args.begin()], Info, *I))
+        !Evaluate(ArgValues[I - Args.begin()], Info, *I) ||
+        Info.EvalStatus.HasSideEffects)
       // If evaluation fails, throw away the argument entirely.
       ArgValues[I - Args.begin()] = APValue();
-    if (Info.EvalStatus.HasSideEffects)
-      return false;
+
+    // Ignore any side-effects from a failed evaluation. This is safe because
+    // they can't interfere with any other argument evaluation.
+    Info.EvalStatus.HasSideEffects = false;
   }
 
+  // Parameter cleanups happen in the caller and are not part of this
+  // evaluation.
+  Info.discardCleanups();
+  Info.EvalStatus.HasSideEffects = false;
+
   // Build fake call to Callee.
   CallStackFrame Frame(Info, Callee->getLocation(), Callee, ThisPtr,
                        ArgValues.data());
-  return Evaluate(Value, Info, this) && Info.discardCleanups() &&
+  // FIXME: Missing ExprWithCleanups in enable_if conditions?
+  FullExpressionRAII Scope(Info);
+  return Evaluate(Value, Info, this) && Scope.destroy() &&
          !Info.EvalStatus.HasSideEffects;
 }
 
diff --git a/clang/test/SemaCXX/enable_if.cpp b/clang/test/SemaCXX/enable_if.cpp
index fd1375136a2e..37664276e470 100644
--- a/clang/test/SemaCXX/enable_if.cpp
+++ b/clang/test/SemaCXX/enable_if.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s
-
+// RUN: %clang_cc1 -std=c++2a -verify %s
 typedef int (*fp)(int);
 int surrogate(int);
 struct Incomplete;  // expected-note{{forward declaration of 'Incomplete'}} \
@@ -533,3 +533,31 @@ namespace StringLiteralDetector {
   }
 }
 
+namespace IgnoreUnusedArgSideEffects {
+  struct A { ~A(); };
+  void f(A a, bool b) __attribute__((enable_if(b, ""))); // expected-note 2-3{{disabled}}
+  void test() {
+    f(A(), true);
+    f(A(), false); // expected-error {{no matching function}}
+    int n;
+    f((n = 1, A()), true);
+    f(A(), (n = 1, true)); // expected-error {{no matching function}}
+    f(A(), (A(), true));
+  }
+
+#if __cplusplus > 201702L
+  struct B { constexpr ~B() {} bool b; };
+  void g(B b) __attribute__((enable_if(b.b, ""))); // expected-note {{disabled}}
+  void test2() {
+    g(B{true});
+    g(B{false}); // expected-error {{no matching function}}
+    f(A(), B{true}.b);
+    f(A(), B{false}.b); // expected-error {{no matching function}}
+  }
+
+  // First condition is non-constant due to non-constexpr destructor of A.
+  int &h() __attribute__((enable_if((A(), true), "")));
+  float &h() __attribute__((enable_if((B(), true), "")));
+  float &x = h();
+#endif
+}