[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL329671: [ExprConstant] Use an AST node and a version number 
as a key to create (authored by ahatanak, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42776?vs=141788=141789#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42776

Files:
  cfe/trunk/include/clang/AST/APValue.h
  cfe/trunk/lib/AST/APValue.cpp
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
  cfe/trunk/test/SemaCXX/constexpr-default-arg.cpp

Index: cfe/trunk/lib/AST/ExprConstant.cpp
===
--- cfe/trunk/lib/AST/ExprConstant.cpp
+++ cfe/trunk/lib/AST/ExprConstant.cpp
@@ -452,8 +452,8 @@
 
 // Note that we intentionally use std::map here so that references to
 // values are stable.
-typedef std::map MapTy;
-typedef MapTy::const_iterator temp_iterator;
+typedef std::pair MapKeyTy;
+typedef std::map MapTy;
 /// Temporaries - Temporary lvalues materialized within this stack frame.
 MapTy Temporaries;
 
@@ -463,6 +463,20 @@
 /// Index - The call index of this call.
 unsigned Index;
 
+/// The stack of integers for tracking version numbers for temporaries.
+SmallVector TempVersionStack = {1};
+unsigned CurTempVersion = TempVersionStack.back();
+
+unsigned getTempVersion() const { return TempVersionStack.back(); }
+
+void pushTempVersion() {
+  TempVersionStack.push_back(++CurTempVersion);
+}
+
+void popTempVersion() {
+  TempVersionStack.pop_back();
+}
+
 // FIXME: Adding this to every 'CallStackFrame' may have a nontrivial impact
 // on the overall stack usage of deeply-recursing constexpr evaluataions.
 // (We should cache this map rather than recomputing it repeatedly.)
@@ -479,10 +493,36 @@
APValue *Arguments);
 ~CallStackFrame();
 
-APValue *getTemporary(const void *Key) {
-  MapTy::iterator I = Temporaries.find(Key);
-  return I == Temporaries.end() ? nullptr : >second;
+// Return the temporary for Key whose version number is Version.
+APValue *getTemporary(const void *Key, unsigned Version) {
+  MapKeyTy KV(Key, Version);
+  auto LB = Temporaries.lower_bound(KV);
+  if (LB != Temporaries.end() && LB->first == KV)
+return >second;
+  // Pair (Key,Version) wasn't found in the map. Check that no elements
+  // in the map have 'Key' as their key.
+  assert((LB == Temporaries.end() || LB->first.first != Key) &&
+ (LB == Temporaries.begin() || std::prev(LB)->first.first != Key) &&
+ "Element with key 'Key' found in map");
+  return nullptr;
 }
+
+// Return the current temporary for Key in the map.
+APValue *getCurrentTemporary(const void *Key) {
+  auto UB = Temporaries.upper_bound(MapKeyTy(Key, UINT_MAX));
+  if (UB != Temporaries.begin() && std::prev(UB)->first.first == Key)
+return ::prev(UB)->second;
+  return nullptr;
+}
+
+// Return the version number of the current temporary for Key.
+unsigned getCurrentTemporaryVersion(const void *Key) const {
+  auto UB = Temporaries.upper_bound(MapKeyTy(Key, UINT_MAX));
+  if (UB != Temporaries.begin() && std::prev(UB)->first.first == Key)
+return std::prev(UB)->first.second;
+  return 0;
+}
+
 APValue (const void *Key, bool IsLifetimeExtended);
   };
 
@@ -612,7 +652,8 @@
 
 /// EvaluatingObject - Pair of the AST node that an lvalue represents and
 /// the call index that that lvalue was allocated in.
-typedef std::pair EvaluatingObject;
+typedef std::pair>
+EvaluatingObject;
 
 /// EvaluatingConstructors - Set of objects that are currently being
 /// constructed.
@@ -631,8 +672,10 @@
   }
 };
 
-bool isEvaluatingConstructor(APValue::LValueBase Decl, unsigned CallIndex) {
-  return EvaluatingConstructors.count(EvaluatingObject(Decl, CallIndex));
+bool isEvaluatingConstructor(APValue::LValueBase Decl, unsigned CallIndex,
+ unsigned Version) {
+  return EvaluatingConstructors.count(
+  EvaluatingObject(Decl, {CallIndex, Version}));
 }
 
 /// The current array initialization index, if we're performing array
@@ -728,7 +771,7 @@
 void setEvaluatingDecl(APValue::LValueBase Base, APValue ) {
   EvaluatingDecl = Base;
   EvaluatingDeclValue = 
-  EvaluatingConstructors.insert({Base, 0});
+  EvaluatingConstructors.insert({Base, {0, 0}});
 }
 
 const LangOptions () const { return Ctx.getLangOpts(); }
@@ -1092,11 +1135,16 @@
 unsigned OldStackSize;
   public:
 ScopeRAII(EvalInfo )
-: Info(Info), OldStackSize(Info.CleanupStack.size()) 

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 141788.
ahatanak marked an inline comment as done.
ahatanak set the repository for this revision to rC Clang.
ahatanak added a comment.

Use pair as the key.


Repository:
  rC Clang

https://reviews.llvm.org/D42776

Files:
  clang/include/clang/AST/APValue.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx1y.cpp
  clang/test/SemaCXX/constexpr-default-arg.cpp

Index: clang/test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++1y -S -o - -emit-llvm -verify %s
+
+namespace default_arg_temporary {
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr const int (const int  = 0) {
+  return p;
+}
+
+struct S {
+  constexpr S(const int  = 0) {}
+};
+
+void test_default_arg2() {
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because the same MTE is used to initilize
+  // both elements of the array (see PR33140).
+  constexpr S s[2] = {};
+
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because multiple CXXDefaultArgExpr share
+  // the same MTE (see PR33140).
+  static_assert(equals() && equals(), "");
+
+  // Test that constant expression evaluation produces distinct lvalues for
+  // each call.
+  static_assert(() != (), "");
+}
+
+// Check that multiple CXXDefaultInitExprs don't cause an assertion failure.
+struct A { int & = 0; }; // expected-warning {{binding reference member}} // expected-note {{reference member declared here}}
+struct B { A x, y; };
+B b = {};
+
+}
Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -852,7 +852,6 @@
   static_assert(h(2) == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
   static_assert(h(3) == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
 
-  // FIXME: This function should be treated as non-constant.
   constexpr void lifetime_versus_loops() {
 int *p = 0;
 for (int i = 0; i != 2; ++i) {
@@ -862,10 +861,10 @@
   if (i)
 // This modifies the 'n' from the previous iteration of the loop outside
 // its lifetime.
-++*q;
+++*q; // expected-note {{increment of object outside its lifetime}}
 }
   }
-  static_assert((lifetime_versus_loops(), true), "");
+  static_assert((lifetime_versus_loops(), true), ""); // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace Bitfields {
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -452,8 +452,8 @@
 
 // Note that we intentionally use std::map here so that references to
 // values are stable.
-typedef std::map MapTy;
-typedef MapTy::const_iterator temp_iterator;
+typedef std::pair MapKeyTy;
+typedef std::map MapTy;
 /// Temporaries - Temporary lvalues materialized within this stack frame.
 MapTy Temporaries;
 
@@ -463,6 +463,20 @@
 /// Index - The call index of this call.
 unsigned Index;
 
+/// The stack of integers for tracking version numbers for temporaries.
+SmallVector TempVersionStack = {1};
+unsigned CurTempVersion = TempVersionStack.back();
+
+unsigned getTempVersion() const { return TempVersionStack.back(); }
+
+void pushTempVersion() {
+  TempVersionStack.push_back(++CurTempVersion);
+}
+
+void popTempVersion() {
+  TempVersionStack.pop_back();
+}
+
 // FIXME: Adding this to every 'CallStackFrame' may have a nontrivial impact
 // on the overall stack usage of deeply-recursing constexpr evaluataions.
 // (We should cache this map rather than recomputing it repeatedly.)
@@ -479,10 +493,36 @@
APValue *Arguments);
 ~CallStackFrame();
 
-APValue *getTemporary(const void *Key) {
-  MapTy::iterator I = Temporaries.find(Key);
-  return I == Temporaries.end() ? nullptr : >second;
+// Return the temporary for Key whose version number is Version.
+APValue *getTemporary(const void *Key, unsigned Version) {
+  MapKeyTy KV(Key, Version);
+  auto LB = Temporaries.lower_bound(KV);
+  if (LB != Temporaries.end() && LB->first == KV)
+return >second;
+  // Pair (Key,Version) wasn't found in the map. Check that no elements
+  // in the map have 'Key' as their key.
+  assert((LB == Temporaries.end() || LB->first.first != Key) &&
+ (LB == Temporaries.begin() || std::prev(LB)->first.first != Key) &&
+ 

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

I think this can be cleaned up a bit further, but I'm fine with that happening 
after the patch lands. Thanks!




Comment at: lib/AST/ExprConstant.cpp:455-456
 // values are stable.
-typedef std::map MapTy;
-typedef MapTy::const_iterator temp_iterator;
+typedef std::map VersionToValueMap;
+typedef std::map MapTy;
 /// Temporaries - Temporary lvalues materialized within this stack frame.

Have you considered using a pair as the key rather than 
a map-of-maps? It'd be preferable to only allocate one heap node rather than 
two for the (hopefully common) case where there is only one version of an 
object.



Comment at: lib/AST/ExprConstant.cpp:466
 
+/// The stack of integers for tracking version numbers for temporaries.
+SmallVector TempVersionStack = {1};

If we're using this for variables in loops too, we should generalize the name 
and the description to not talk about temporaries.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 139797.
ahatanak added a comment.

Assign an uninitialized APValue to a temporary instead of removing it from the 
temporary map when the temporary's lifetime has ended.


https://reviews.llvm.org/D42776

Files:
  include/clang/AST/APValue.h
  lib/AST/APValue.cpp
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constant-expression-cxx1y.cpp
  test/SemaCXX/constexpr-default-arg.cpp

Index: test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++1y -S -o - -emit-llvm -verify %s
+
+namespace default_arg_temporary {
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr const int (const int  = 0) {
+  return p;
+}
+
+struct S {
+  constexpr S(const int  = 0) {}
+};
+
+void test_default_arg2() {
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because the same MTE is used to initilize
+  // both elements of the array (see PR33140).
+  constexpr S s[2] = {};
+
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because multiple CXXDefaultArgExpr share
+  // the same MTE (see PR33140).
+  static_assert(equals() && equals(), "");
+
+  // Test that constant expression evaluation produces distinct lvalues for
+  // each call.
+  static_assert(() != (), "");
+}
+
+// Check that multiple CXXDefaultInitExprs don't cause an assertion failure.
+struct A { int & = 0; }; // expected-warning {{binding reference member}} // expected-note {{reference member declared here}}
+struct B { A x, y; };
+B b = {};
+
+}
Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -852,7 +852,6 @@
   static_assert(h(2) == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
   static_assert(h(3) == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
 
-  // FIXME: This function should be treated as non-constant.
   constexpr void lifetime_versus_loops() {
 int *p = 0;
 for (int i = 0; i != 2; ++i) {
@@ -862,10 +861,10 @@
   if (i)
 // This modifies the 'n' from the previous iteration of the loop outside
 // its lifetime.
-++*q;
+++*q; // expected-note {{increment of object outside its lifetime}}
 }
   }
-  static_assert((lifetime_versus_loops(), true), "");
+  static_assert((lifetime_versus_loops(), true), ""); // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace Bitfields {
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -452,8 +452,8 @@
 
 // Note that we intentionally use std::map here so that references to
 // values are stable.
-typedef std::map MapTy;
-typedef MapTy::const_iterator temp_iterator;
+typedef std::map VersionToValueMap;
+typedef std::map MapTy;
 /// Temporaries - Temporary lvalues materialized within this stack frame.
 MapTy Temporaries;
 
@@ -463,6 +463,20 @@
 /// Index - The call index of this call.
 unsigned Index;
 
+/// The stack of integers for tracking version numbers for temporaries.
+SmallVector TempVersionStack = {1};
+unsigned CurTempVersion = TempVersionStack.back();
+
+unsigned getTempVersion() const { return TempVersionStack.back(); }
+
+void pushTempVersion() {
+  TempVersionStack.push_back(++CurTempVersion);
+}
+
+void popTempVersion() {
+  TempVersionStack.pop_back();
+}
+
 // FIXME: Adding this to every 'CallStackFrame' may have a nontrivial impact
 // on the overall stack usage of deeply-recursing constexpr evaluataions.
 // (We should cache this map rather than recomputing it repeatedly.)
@@ -479,10 +493,33 @@
APValue *Arguments);
 ~CallStackFrame();
 
-APValue *getTemporary(const void *Key) {
+// Return the temporary for Key whose version number is Version.
+APValue *getTemporary(const void *Key, unsigned Version) {
+  MapTy::iterator I = Temporaries.find(Key);
+  if (I == Temporaries.end())
+return nullptr;
+  VersionToValueMap::iterator J = I->second.find(Version);
+  assert(J != I->second.end() && "temporary not found");
+  return >second;
+}
+
+// Return the current temporary for Key in the map.
+APValue *getCurrentTemporary(const void *Key) {
   MapTy::iterator I = Temporaries.find(Key);
-  return I == Temporaries.end() ? nullptr : >second;
+  if (I == Temporaries.end())
+return nullptr;
+  assert(!I->second.empty() && "temporary not found");
+  return 

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/AST/ExprConstant.cpp:5236
 if (Frame) {
-  Result.set(VD, Frame->Index);
+  Result.set({VD, Frame->Index});
   return true;

rsmith wrote:
> Hmm. We should be versioning local variables as well. Currently we'll accept 
> invalid code such as:
> 
> ```
> constexpr int f() {
>   int *p = nullptr;
>   for (int k = 0; k != 2; ++k) {
> int local_var = 0;
> if (k == 0)
>   p = _var;
> else
>   return *p;
>   }
> }
> static_assert(f() == 0);
> ```
I made changes to the constructor and destructor of ScopeRAII so that a version 
is pushed at the start of a new iteration. This fixes the FIXME in 
test/SemaCXX/constant-expression-cxx1y.cpp.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 136124.
ahatanak marked 7 inline comments as done.
ahatanak added a comment.

Address review comments.


https://reviews.llvm.org/D42776

Files:
  include/clang/AST/APValue.h
  lib/AST/APValue.cpp
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constant-expression-cxx1y.cpp
  test/SemaCXX/constexpr-default-arg.cpp

Index: test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++1y -S -o - -emit-llvm -verify %s
+
+namespace default_arg_temporary {
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr const int (const int  = 0) {
+  return p;
+}
+
+struct S {
+  constexpr S(const int  = 0) {}
+};
+
+void test_default_arg2() {
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because the same MTE is used to initilize
+  // both elements of the array (see PR33140).
+  constexpr S s[2] = {};
+
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because multiple CXXDefaultArgExpr share
+  // the same MTE (see PR33140).
+  static_assert(equals() && equals(), "");
+
+  // Test that constant expression evaluation produces distinct lvalues for
+  // each call.
+  static_assert(() != (), "");
+}
+
+// Check that multiple CXXDefaultInitExprs don't cause an assertion failure.
+struct A { int & = 0; }; // expected-warning {{binding reference member}} // expected-note {{reference member declared here}}
+struct B { A x, y; };
+B b = {};
+
+}
Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -852,7 +852,6 @@
   static_assert(h(2) == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
   static_assert(h(3) == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
 
-  // FIXME: This function should be treated as non-constant.
   constexpr void lifetime_versus_loops() {
 int *p = 0;
 for (int i = 0; i != 2; ++i) {
@@ -862,10 +861,10 @@
   if (i)
 // This modifies the 'n' from the previous iteration of the loop outside
 // its lifetime.
-++*q;
+++*q; // expected-note {{increment of object outside its lifetime}}
 }
   }
-  static_assert((lifetime_versus_loops(), true), "");
+  static_assert((lifetime_versus_loops(), true), ""); // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace Bitfields {
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -438,8 +438,8 @@
 
 // Note that we intentionally use std::map here so that references to
 // values are stable.
-typedef std::map MapTy;
-typedef MapTy::const_iterator temp_iterator;
+typedef std::map VersionToValueMap;
+typedef std::map MapTy;
 /// Temporaries - Temporary lvalues materialized within this stack frame.
 MapTy Temporaries;
 
@@ -449,6 +449,20 @@
 /// Index - The call index of this call.
 unsigned Index;
 
+/// The stack of integers for tracking version numbers for temporaries.
+SmallVector TempVersionStack = {1};
+unsigned CurTempVersion = TempVersionStack.back();
+
+unsigned getTempVersion() const { return TempVersionStack.back(); }
+
+void pushTempVersion() {
+  TempVersionStack.push_back(++CurTempVersion);
+}
+
+void popTempVersion() {
+  TempVersionStack.pop_back();
+}
+
 // FIXME: Adding this to every 'CallStackFrame' may have a nontrivial impact
 // on the overall stack usage of deeply-recursing constexpr evaluataions.
 // (We should cache this map rather than recomputing it repeatedly.)
@@ -465,10 +479,36 @@
APValue *Arguments);
 ~CallStackFrame();
 
-APValue *getTemporary(const void *Key) {
+// Return the temporary for Key whose version number is Version.
+APValue *getTemporary(const void *Key, unsigned Version) {
+  MapTy::iterator I = Temporaries.find(Key);
+  if (I == Temporaries.end())
+return nullptr;
+  // Return an uninitialized value if the temporary is not found.
+  static APValue UninitVal;
+  VersionToValueMap::iterator J = I->second.find(Version);
+  return J == I->second.end() ?  : >second;
+}
+
+// Return the current temporary for Key in the map.
+APValue *getCurrentTemporary(const void *Key) {
   MapTy::iterator I = Temporaries.find(Key);
-  return I == Temporaries.end() ? nullptr : >second;
+  if (I == Temporaries.end())
+return nullptr;
+  // Return an uninitialized value if all temporaries for Key have been
+  // 

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thank you, this looks like a great direction.

As noted, there are a bunch of other cases that we should cover with this 
approach. I'm really happy about the number of related bugs we get to fix with 
this change.




Comment at: lib/AST/ExprConstant.cpp:3186-3187
   // object under construction.
-  if (Info.isEvaluatingConstructor(LVal.getLValueBase(), LVal.CallIndex)) {
+  if (Info.isEvaluatingConstructor(LVal.getLValueBase(),
+   LVal.getLValueCallIndex())) {
 BaseType = Info.Ctx.getCanonicalType(BaseType);

This should take the version into account.



Comment at: lib/AST/ExprConstant.cpp:5236
 if (Frame) {
-  Result.set(VD, Frame->Index);
+  Result.set({VD, Frame->Index});
   return true;

Hmm. We should be versioning local variables as well. Currently we'll accept 
invalid code such as:

```
constexpr int f() {
  int *p = nullptr;
  for (int k = 0; k != 2; ++k) {
int local_var = 0;
if (k == 0)
  p = _var;
else
  return *p;
  }
}
static_assert(f() == 0);
```



Comment at: lib/AST/ExprConstant.cpp:5275-5278
+unsigned Version = Info.CurrentCall->getMTEVersion();
 Value = >
-createTemporary(E, E->getStorageDuration() == SD_Automatic);
-Result.set(E, Info.CurrentCall->Index);
+createTemporary(E, E->getStorageDuration() == SD_Automatic, Version);
+Result.set({E, Info.CurrentCall->Index, Version});

Can you combine these, so we have a single function to create a temporary and 
produce both an `LValue` denoting it and an `APValue*` to hold its evaluated 
value?



Comment at: lib/AST/ExprConstant.cpp:5772
 } else {
-  Result.set(SubExpr, Info.CurrentCall->Index);
+  Result.set({SubExpr, Info.CurrentCall->Index});
   if (!EvaluateInPlace(Info.CurrentCall->createTemporary(SubExpr, false),

This should create a versioned temporary object.



Comment at: lib/AST/ExprConstant.cpp:6540
   bool VisitConstructExpr(const Expr *E) {
-Result.set(E, Info.CurrentCall->Index);
+Result.set({E, Info.CurrentCall->Index});
 return EvaluateInPlace(Info.CurrentCall->createTemporary(E, false),

This should create a versioned temporary object.



Comment at: lib/AST/ExprConstant.cpp:8031
+ (A.getLValueCallIndex() == B.getLValueCallIndex() &&
+  A.getLValueVersion() == B.getLValueVersion());
 }

You already checked this above. It'd make sense to check the call index and 
version in the same place here, but we should only need one check for each :)



Comment at: lib/AST/ExprConstant.cpp:9974-9997
+LV.set({E, Info.CurrentCall->Index});
 APValue  = Info.CurrentCall->createTemporary(E, false);
 if (!EvaluateArray(E, LV, Value, Info))
   return false;
 Result = Value;
   } else if (T->isRecordType()) {
 LValue LV;

These temporaries should all be versioned.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

ping


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/AST/ExprConstant.cpp:589-598
+/// Keep track of the version of MTEs that are used by CXXDefaultArgExpr.
+/// The version number is updated every time VisitCXXDefaultArgExpr is
+/// called.
+unsigned getDefaultArgNum() const { return CurDefaultArgNum; }
+void setDefaultArgNum() {
+  assert(CurDefaultArgNum == 0 && "CurDefaultArgNum hasn't been cleared");
+  CurDefaultArgNum = ++NextDefaultArgNum;

rsmith wrote:
> This seems extremely similar to the purpose of the existing `CallIndex` 
> parameter. Have you thought about ways the two might be unified?
> 
> If there's no reasonable path to unifying the two (which I suspect there may 
> not be), it would make more sense to me to store a current version number on 
> the `CallStackFrame` rather than globally in the `EvalInfo`, and to restart 
> the numbering in each new call frame. That also emphasizes something else: 
> this version number should be kept with the `CallIndex`. (We could achieve 
> that by either moving the `CallIndex` into the `LValueBase` or moving the 
> `Version` out of it.)
It couldn't come up with a way to unify CallIndex and Version. I think it's 
better to keep them separate because you need the CallIndex to get the frame 
that owns the temporary and you need Version to distinguish between two 
temporaries created by the same MTE.

I moved CallIndex into LValueBase as you suggested.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 134560.
ahatanak marked 3 inline comments as done.
ahatanak added a comment.

Address review comments.


https://reviews.llvm.org/D42776

Files:
  include/clang/AST/APValue.h
  lib/AST/APValue.cpp
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constexpr-default-arg.cpp

Index: test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++1y -S -o - -emit-llvm -verify %s
+
+namespace default_arg_temporary {
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr const int (const int  = 0) {
+  return p;
+}
+
+struct S {
+  constexpr S(const int  = 0) {}
+};
+
+void test_default_arg2() {
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because the same MTE is used to initilize
+  // both elements of the array (see PR33140).
+  constexpr S s[2] = {};
+
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because multiple CXXDefaultArgExpr share
+  // the same MTE (see PR33140).
+  static_assert(equals() && equals(), "");
+
+  // Test that constant expression evaluation produces distinct lvalues for
+  // each call.
+  static_assert(() != (), "");
+}
+
+// Check that multiple CXXDefaultInitExprs don't cause an assertion failure.
+struct A { int & = 0; }; // expected-warning {{binding reference member}} // expected-note {{reference member declared here}}
+struct B { A x, y; };
+B b = {};
+
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -438,7 +438,8 @@
 
 // Note that we intentionally use std::map here so that references to
 // values are stable.
-typedef std::map MapTy;
+typedef std::pair KeyTy;
+typedef std::map MapTy;
 typedef MapTy::const_iterator temp_iterator;
 /// Temporaries - Temporary lvalues materialized within this stack frame.
 MapTy Temporaries;
@@ -449,6 +450,22 @@
 /// Index - The call index of this call.
 unsigned Index;
 
+/// Keep track of the version of MTEs that are used by CXXDefaultArgExpr or
+/// CXXDefaultInitExpr. The version number is updated every time
+/// VisitCXXDefaultArgExpr or VisitCXXDefaultInitExpr is visited.
+unsigned NextMTEVersion = 0;
+SmallVector MTEVersionStack = {0};
+
+unsigned getMTEVersion() const { return MTEVersionStack.back(); }
+
+void pushMTEVersion() {
+  MTEVersionStack.push_back(++NextMTEVersion);
+}
+
+void popMTEVersion() {
+  MTEVersionStack.pop_back();
+}
+
 // FIXME: Adding this to every 'CallStackFrame' may have a nontrivial impact
 // on the overall stack usage of deeply-recursing constexpr evaluataions.
 // (We should cache this map rather than recomputing it repeatedly.)
@@ -465,11 +482,12 @@
APValue *Arguments);
 ~CallStackFrame();
 
-APValue *getTemporary(const void *Key) {
-  MapTy::iterator I = Temporaries.find(Key);
+APValue *getTemporary(const void *Key, unsigned Version = 0) {
+  MapTy::iterator I = Temporaries.find(KeyTy(Key, Version));
   return I == Temporaries.end() ? nullptr : >second;
 }
-APValue (const void *Key, bool IsLifetimeExtended);
+APValue (const void *Key, bool IsLifetimeExtended,
+ unsigned Version = 0);
   };
 
   /// Temporarily override 'this'.
@@ -1161,8 +1179,9 @@
 }
 
 APValue ::createTemporary(const void *Key,
- bool IsLifetimeExtended) {
-  APValue  = Temporaries[Key];
+ bool IsLifetimeExtended,
+ unsigned Version) {
+  APValue  = Temporaries[KeyTy(Key, Version)];
   assert(Result.isUninit() && "temporary created multiple times");
   Info.CleanupStack.push_back(Cleanup(, IsLifetimeExtended));
   return Result;
@@ -1254,40 +1273,39 @@
   struct LValue {
 APValue::LValueBase Base;
 CharUnits Offset;
-unsigned InvalidBase : 1;
-unsigned CallIndex : 31;
 SubobjectDesignator Designator;
-bool IsNullPtr;
+bool IsNullPtr : 1;
+bool InvalidBase : 1;
 
 const APValue::LValueBase getLValueBase() const { return Base; }
 CharUnits () { return Offset; }
 const CharUnits () const { return Offset; }
-unsigned getLValueCallIndex() const { return CallIndex; }
 SubobjectDesignator () { return Designator; }
 const SubobjectDesignator () const { return Designator;}
 bool isNullPointer() const { return IsNullPtr;}
 
+unsigned getLValueCallIndex() const { return Base.getCallIndex(); }
+unsigned getLValueVersion() const { return Base.getVersion(); }
+
 void moveInto(APValue ) const {
   if (Designator.Invalid)
-  

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

OK, I see. It's pretty easy to come up with an example.

  constexpr int foo1(int a = 12) {
return a * a;
  }
  
  constexpr int foo2(int a = foo1()) {
return a - 12;
  }


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/AST/ExprConstant.cpp:597
+}
+void clearDefaultArgNum() { CurDefaultArgNum = 0; }
+unsigned CurDefaultArgNum = 0, NextDefaultArgNum = 0;

rsmith wrote:
> This is wrong: these scopes can nest, so you can't just reset the number to 0 
> when you're done. You should restore the prior number when you're done here, 
> to divide up evaluation into CXXDefaultArgExpr scopes. (Technically, I think 
> it would also be correct to leave the number alone when you leave one of 
> these scopes, but only because a scope for a particular parameter's default 
> argument can't nest within another scope for the same default argument 
> expression -- and even that might not be true in the presence of template 
> instantiation.)
Do you mean VisitCXXDefaultArgExpr can be called the second time before the 
first call returns? Do you have an example code that would cause that to happen 
(which I can perhaps add as a test case)?

It seemed to me that that would happen only if you used a lambda expression for 
the default argument, but I thought the current standard doesn't allow using 
lambda expressions in constant expressions.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/APValue.cpp:27
+APValue::LValueBase Base;
+bool IsOnePastTheEnd;
 CharUnits Offset;

Move this to the end so it can share space with `IsNullPtr`.



Comment at: lib/AST/ExprConstant.cpp:589-598
+/// Keep track of the version of MTEs that are used by CXXDefaultArgExpr.
+/// The version number is updated every time VisitCXXDefaultArgExpr is
+/// called.
+unsigned getDefaultArgNum() const { return CurDefaultArgNum; }
+void setDefaultArgNum() {
+  assert(CurDefaultArgNum == 0 && "CurDefaultArgNum hasn't been cleared");
+  CurDefaultArgNum = ++NextDefaultArgNum;

This seems extremely similar to the purpose of the existing `CallIndex` 
parameter. Have you thought about ways the two might be unified?

If there's no reasonable path to unifying the two (which I suspect there may 
not be), it would make more sense to me to store a current version number on 
the `CallStackFrame` rather than globally in the `EvalInfo`, and to restart the 
numbering in each new call frame. That also emphasizes something else: this 
version number should be kept with the `CallIndex`. (We could achieve that by 
either moving the `CallIndex` into the `LValueBase` or moving the `Version` out 
of it.)



Comment at: lib/AST/ExprConstant.cpp:597
+}
+void clearDefaultArgNum() { CurDefaultArgNum = 0; }
+unsigned CurDefaultArgNum = 0, NextDefaultArgNum = 0;

This is wrong: these scopes can nest, so you can't just reset the number to 0 
when you're done. You should restore the prior number when you're done here, to 
divide up evaluation into CXXDefaultArgExpr scopes. (Technically, I think it 
would also be correct to leave the number alone when you leave one of these 
scopes, but only because a scope for a particular parameter's default argument 
can't nest within another scope for the same default argument expression -- and 
even that might not be true in the presence of template instantiation.)



Comment at: lib/AST/ExprConstant.cpp:4595
+  }
   bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E) {
 // The initializer may not have been parsed yet, or might be erroneous.

We need the same handling for `CXXDefaultInitExpr`, too.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 133947.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.

Address Erik's and Roman's review comments.


https://reviews.llvm.org/D42776

Files:
  include/clang/AST/APValue.h
  lib/AST/APValue.cpp
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constexpr-default-arg.cpp

Index: test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -std=c++1y -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+namespace default_arg_temporary {
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr const int (const int  = 0) {
+  return p;
+}
+
+struct S {
+  constexpr S(const int  = 0) {}
+};
+
+void test_default_arg2() {
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because the same MTE is used to initilize
+  // both elements of the array (see PR33140).
+  constexpr S s[2] = {};
+
+  // This piece of code used to cause an assertion failure in
+  // CallStackFrame::createTemporary because multiple CXXDefaultArgExpr share
+  // the same MTE (see PR33140).
+  static_assert(equals() && equals(), "");
+
+  // Test that constant expression evaluation produces distinct lvalues for
+  // each call.
+  static_assert(() != (), "");
+}
+
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -438,7 +438,8 @@
 
 // Note that we intentionally use std::map here so that references to
 // values are stable.
-typedef std::map MapTy;
+typedef std::pair KeyTy;
+typedef std::map MapTy;
 typedef MapTy::const_iterator temp_iterator;
 /// Temporaries - Temporary lvalues materialized within this stack frame.
 MapTy Temporaries;
@@ -465,11 +466,12 @@
APValue *Arguments);
 ~CallStackFrame();
 
-APValue *getTemporary(const void *Key) {
-  MapTy::iterator I = Temporaries.find(Key);
+APValue *getTemporary(const void *Key, unsigned Version = 0) {
+  MapTy::iterator I = Temporaries.find(KeyTy(Key, Version));
   return I == Temporaries.end() ? nullptr : >second;
 }
-APValue (const void *Key, bool IsLifetimeExtended);
+APValue (const void *Key, bool IsLifetimeExtended,
+ unsigned Version = 0);
   };
 
   /// Temporarily override 'this'.
@@ -584,6 +586,17 @@
 /// initialized after CurrentCall and CallStackDepth.
 CallStackFrame BottomFrame;
 
+/// Keep track of the version of MTEs that are used by CXXDefaultArgExpr.
+/// The version number is updated every time VisitCXXDefaultArgExpr is
+/// called.
+unsigned getDefaultArgNum() const { return CurDefaultArgNum; }
+void setDefaultArgNum() {
+  assert(CurDefaultArgNum == 0 && "CurDefaultArgNum hasn't been cleared");
+  CurDefaultArgNum = ++NextDefaultArgNum;
+}
+void clearDefaultArgNum() { CurDefaultArgNum = 0; }
+unsigned CurDefaultArgNum = 0, NextDefaultArgNum = 0;
+
 /// A stack of values whose lifetimes end at the end of some surrounding
 /// evaluation frame.
 llvm::SmallVector CleanupStack;
@@ -1161,8 +1174,9 @@
 }
 
 APValue ::createTemporary(const void *Key,
- bool IsLifetimeExtended) {
-  APValue  = Temporaries[Key];
+ bool IsLifetimeExtended,
+ unsigned Version) {
+  APValue  = Temporaries[KeyTy(Key, Version)];
   assert(Result.isUninit() && "temporary created multiple times");
   Info.CleanupStack.push_back(Cleanup(, IsLifetimeExtended));
   return Result;
@@ -3147,7 +3161,7 @@
 return CompleteObject();
   }
 } else {
-  BaseVal = Frame->getTemporary(Base);
+  BaseVal = Frame->getTemporary(Base, LVal.Base.getVersion());
   assert(BaseVal && "missing value for temporary");
 }
 
@@ -4529,6 +4543,17 @@
 
   bool ZeroInitialization(const Expr *E) { return Error(E); }
 
+  struct DefaultArgRAII {
+EvalInfo 
+
+DefaultArgRAII(EvalInfo ) : Info(Info) {
+  Info.setDefaultArgNum();
+}
+~DefaultArgRAII() {
+  Info.clearDefaultArgNum();
+}
+  };
+
 public:
   ExprEvaluatorBase(EvalInfo ) : Info(Info) {}
 
@@ -4563,8 +4588,10 @@
 { return StmtVisitorTy::Visit(E->getResultExpr()); }
   bool VisitSubstNonTypeTemplateParmExpr(const SubstNonTypeTemplateParmExpr *E)
 { return StmtVisitorTy::Visit(E->getReplacement()); }
-  bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E)
-{ return StmtVisitorTy::Visit(E->getExpr()); }
+  bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E) {
+DefaultArgRAII RAII(Info);
+return StmtVisitorTy::Visit(E->getExpr());
+  }
   bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr 

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/AST/ExprConstant.cpp:1165-1173
+  auto LB = Temporaries.lower_bound(Key);
+
+  // If an element with key Key is found, reset the value and return it. This
+  // can happen if Key is part of a default argument expression.
+  if (LB != Temporaries.end() && LB->first == Key)
+return LB->second = APValue();
+

erik.pilkington wrote:
> I think that the problem is more subtle than this. This static assert errors 
> (previously clang would assert) when it really it should be fine.
> ```
> constexpr const int (const int  = 0) { return p; }
> static_assert(() != ());
> ```
> Because default arguments are allocated on the caller side, both the calls to 
> `x()` call createTemporary for the same MaterializeTemporaryExpr in the same 
> CallStackFrame, when really that MTE should correspond to two distinct 
> values. This patch just hides that underlying problem by recycling the value 
> created during the first call during the second call.
> 
> Maybe we could have a fancier key that incorporates a node on the caller 
> side, such as the CXXDefaultArgExpr as well at the MTE, and store that fancy 
> key in APValue::LValueBases? That would allow us generate distinct values for 
> these MTEs, and also remember what expression originated it. What do you 
> think about that?
> 
> There is small discussion about this problem here: 
> https://bugs.llvm.org/show_bug.cgi?id=33140
Thank you Erik for explaining the problem and pointing me to the PR. 

I ended up using a version number that is updated every time 
VisitCXXDefaultArgExpr is called. I initially tried using CXXDefaultArgExpr and 
the void* pointer as the key, but discovered that that wouldn't fix the 
assertion failure when compiling the first test case in PR33140 since 
InitListExpr uses the same CXXConstructExpr (and hence the same 
CXXDefaultArgExpr) to initialize the array.

Let me know if there are other cases I haven't thought about.



Comment at: test/SemaCXX/constexpr-default-arg.cpp:3
+
+// expected-no-diagnostics
+

lebedev.ri wrote:
> Down the line, it won't be obvious *what* this testcase is checking.
> At the very least wrap it into `namespace rdar_problem_36505742 {}`
I added comments that explain what it's trying to check.


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Hi Akira, thanks for working on this!




Comment at: lib/AST/ExprConstant.cpp:1165-1173
+  auto LB = Temporaries.lower_bound(Key);
+
+  // If an element with key Key is found, reset the value and return it. This
+  // can happen if Key is part of a default argument expression.
+  if (LB != Temporaries.end() && LB->first == Key)
+return LB->second = APValue();
+

I think that the problem is more subtle than this. This static assert errors 
(previously clang would assert) when it really it should be fine.
```
constexpr const int (const int  = 0) { return p; }
static_assert(() != ());
```
Because default arguments are allocated on the caller side, both the calls to 
`x()` call createTemporary for the same MaterializeTemporaryExpr in the same 
CallStackFrame, when really that MTE should correspond to two distinct values. 
This patch just hides that underlying problem by recycling the value created 
during the first call during the second call.

Maybe we could have a fancier key that incorporates a node on the caller side, 
such as the CXXDefaultArgExpr as well at the MTE, and store that fancy key in 
APValue::LValueBases? That would allow us generate distinct values for these 
MTEs, and also remember what expression originated it. What do you think about 
that?

There is small discussion about this problem here: 
https://bugs.llvm.org/show_bug.cgi?id=33140


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: test/SemaCXX/constexpr-default-arg.cpp:3
+
+// expected-no-diagnostics
+

Down the line, it won't be obvious *what* this testcase is checking.
At the very least wrap it into `namespace rdar_problem_36505742 {}`


https://reviews.llvm.org/D42776



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added a reviewer: rsmith.

The assertion fails when a function with a default argument that materializes a 
temporary is called more than once in an expression. The assertion fails in 
CallStackFrame::createTemporary when it searches map Temporaries using the 
default argument's expression (which is a MaterializeTemporaryExpr) as the key 
and it discovers that there is already an element with that key that has been 
initialized.

  constexpr bool equals(const float& arg = 1.0f) {
return arg == 1.0f;
  }
  
  constexpr bool test_default_arg() {
return equals() && equals();
  }

This patch removes the assertion and makes CallStackFrame::createTemporary 
reset the existing element and return it if the element is found in the map.

rdar://problem/36505742


https://reviews.llvm.org/D42776

Files:
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constexpr-default-arg.cpp


Index: test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++1y -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr bool test_default_arg() {
+  return equals() && equals();
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -1162,8 +1162,15 @@
 
 APValue ::createTemporary(const void *Key,
  bool IsLifetimeExtended) {
-  APValue  = Temporaries[Key];
-  assert(Result.isUninit() && "temporary created multiple times");
+  auto LB = Temporaries.lower_bound(Key);
+
+  // If an element with key Key is found, reset the value and return it. This
+  // can happen if Key is part of a default argument expression.
+  if (LB != Temporaries.end() && LB->first == Key)
+return LB->second = APValue();
+
+  APValue  =
+  Temporaries.insert(LB, std::make_pair(Key, APValue()))->second;
   Info.CleanupStack.push_back(Cleanup(, IsLifetimeExtended));
   return Result;
 }


Index: test/SemaCXX/constexpr-default-arg.cpp
===
--- /dev/null
+++ test/SemaCXX/constexpr-default-arg.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++1y -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+constexpr bool equals(const float& arg = 1.0f) {
+  return arg == 1.0f;
+}
+
+constexpr bool test_default_arg() {
+  return equals() && equals();
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -1162,8 +1162,15 @@
 
 APValue ::createTemporary(const void *Key,
  bool IsLifetimeExtended) {
-  APValue  = Temporaries[Key];
-  assert(Result.isUninit() && "temporary created multiple times");
+  auto LB = Temporaries.lower_bound(Key);
+
+  // If an element with key Key is found, reset the value and return it. This
+  // can happen if Key is part of a default argument expression.
+  if (LB != Temporaries.end() && LB->first == Key)
+return LB->second = APValue();
+
+  APValue  =
+  Temporaries.insert(LB, std::make_pair(Key, APValue()))->second;
   Info.CleanupStack.push_back(Cleanup(, IsLifetimeExtended));
   return Result;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits