<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 13, 2015 at 5:15 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>> writes:<br>
> majnemer created this revision.<br>
> majnemer added a reviewer: rsmith.<br>
> majnemer added a subscriber: cfe-commits.<br>
><br>
> We risk iterator invalidation issues if we use DenseMap structures for<br>
> MaterializedTemporaryExprs. Use a std::map to ensure that they don't<br>
> move around in memory.<br>
><br>
> This fixes PR24289.<br>
><br>
> <a href="http://reviews.llvm.org/D11629" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11629</a><br>
><br>
> Files:<br>
> include/clang/AST/ASTContext.h<br>
> lib/AST/ASTContext.cpp<br>
> lib/CodeGen/CodeGenModule.h<br>
> test/CodeGenCXX/PR24289.cpp<br>
><br>
</div></div>> Index: test/CodeGenCXX/PR24289.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGenCXX/PR24289.cpp<br>
> @@ -0,0 +1,82 @@<br>
> +// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-pc-linux-gnu -std=c++11 | FileCheck %s<br>
> +<br>
> +namespace std {<br>
> +template <class T><br>
> +struct initializer_list {<br>
> + const T *Begin;<br>
> + __SIZE_TYPE__ Size;<br>
> +<br>
> + constexpr initializer_list(const T *B, __SIZE_TYPE__ S)<br>
> + : Begin(B), Size(S) {}<br>
> +};<br>
> +}<br>
> +<br>
> +void f() {<br>
> + static std::initializer_list<std::initializer_list<int>> a{<br>
> + {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}};<br>
> + static std::initializer_list<std::initializer_list<int>> b{<br>
> + {0}, {0}, {0}, {0}};<br>
> + static std::initializer_list<std::initializer_list<int>> c{<br>
> + {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}};<br>
> + static std::initializer_list<std::initializer_list<int>> d{<br>
> + {0}, {0}, {0}, {0}, {0}};<br>
> + static std::initializer_list<std::initializer_list<int>> e{<br>
> + {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}};<br>
> +}<br>
> +<br>
> +// CHECK-DAG: @_ZZ1fvE1a = internal global %{{.*}} { %{{.*}}* getelementptr inbounds ([14 x %{{.*}}], [14 x %{{.*}}]<br>
> +// CHECK-DAG: * @_ZGRZ1fvE1a_, i32 0, i32 0), i64 14 }<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a0_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a1_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a2_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a3_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a4_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a5_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a6_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a7_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a8_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a9_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1aA_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1aB_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1aC_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1aD_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1a_ = internal constant [14 x %{{.*}}] [%{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a0_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a1_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a2_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a3_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a4_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a5_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a6_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a7_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a8_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1a9_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1aA_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1aB_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1aC_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1aD_, i32 0, i32 0), i64 1 }]<br>
> +// CHECK-DAG: @_ZZ1fvE1b = internal global %{{.*}} { %{{.*}}* getelementptr inbounds ([4 x %{{.*}}], [4 x %{{.*}}]*<br>
> +// CHECK-DAG: @_ZGRZ1fvE1b_, i32 0, i32 0), i64 4 }<br>
> +// CHECK-DAG: @_ZGRZ1fvE1b0_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1b1_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1b2_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1b3_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1b_ = internal constant [4 x %{{.*}}] [%{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1b0_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1b1_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1b2_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1b3_, i32 0, i32 0), i64 1 }]<br>
> +// CHECK-DAG: @_ZZ1fvE1c = internal global %{{.*}} { %{{.*}}* getelementptr inbounds ([9 x %{{.*}}], [9 x %{{.*}}]*<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c_, i32 0, i32 0), i64 9 }<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c0_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c1_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c2_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c3_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c4_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c5_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c6_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c7_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c8_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1c_ = internal constant [9 x %{{.*}}] [%{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c0_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c1_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c2_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c3_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c4_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c5_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c6_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c7_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1c8_, i32 0, i32 0), i64 1 }]<br>
> +// CHECK-DAG: @_ZZ1fvE1d = internal global %{{.*}} { %{{.*}}* getelementptr inbounds ([5 x %{{.*}}], [5 x %{{.*}}]* @_ZGRZ1fvE1d_, i32 0, i32 0), i64 5 }<br>
> +// CHECK-DAG: @_ZGRZ1fvE1d0_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1d1_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1d2_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1d3_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1d4_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1d_ = internal constant [5 x %{{.*}}] [%{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1d0_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1d1_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1d2_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1d3_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1d4_, i32 0, i32 0), i64 1 }]<br>
> +// CHECK-DAG: @_ZZ1fvE1e = internal global %{{.*}} { %{{.*}}* getelementptr inbounds ([11 x %{{.*}}], [11 x %{{.*}}]* @_ZGRZ1fvE1e_, i32 0, i32 0), i64 11 }<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e0_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e1_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e2_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e3_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e4_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e5_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e6_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e7_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e8_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e9_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1eA_ = internal constant [1 x i32] zeroinitializer<br>
> +// CHECK-DAG: @_ZGRZ1fvE1e_ = internal constant [11 x %{{.*}}] [%{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e0_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e1_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e2_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e3_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e4_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e5_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e6_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e7_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e8_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1e9_, i32 0, i32 0), i64 1 }, %{{.*}} { i32* getelementptr inbounds ([1 x i32], [1 x i32]* @_ZGRZ1fvE1eA_, i32 0, i32 0), i64 1 }]<br>
<br>
I don't suppose we can reduce this test case at all? Maybe a unit test<br>
would be workable to expose this?<br></blockquote><div><br></div><div>You suppose correctly, this is maximally reduced. The bug only exposes itself due to a DenseMap regrowing at just the wrong time.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Index: lib/CodeGen/CodeGenModule.h<br>
> ===================================================================<br>
> --- lib/CodeGen/CodeGenModule.h<br>
> +++ lib/CodeGen/CodeGenModule.h<br>
> @@ -374,7 +374,7 @@<br>
> llvm::DenseMap<llvm::Constant *, llvm::GlobalVariable *> ConstantStringMap;<br>
> llvm::DenseMap<const Decl*, llvm::Constant *> StaticLocalDeclMap;<br>
> llvm::DenseMap<const Decl*, llvm::GlobalVariable*> StaticLocalDeclGuardMap;<br>
> - llvm::DenseMap<const Expr*, llvm::Constant *> MaterializedGlobalTemporaryMap;<br>
> + std::map<const Expr*, llvm::Constant *> MaterializedGlobalTemporaryMap;<br>
<br>
A comment explaining why DenseMap isn't appropriate would be nice.<br></blockquote><div><br></div><div>Sure.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> llvm::DenseMap<QualType, llvm::Constant *> AtomicSetterHelperFnMap;<br>
> llvm::DenseMap<QualType, llvm::Constant *> AtomicGetterHelperFnMap;<br>
> Index: lib/AST/ASTContext.cpp<br>
> ===================================================================<br>
> --- lib/AST/ASTContext.cpp<br>
> +++ lib/AST/ASTContext.cpp<br>
> @@ -8556,7 +8556,7 @@<br>
> if (MayCreate)<br>
> return &MaterializedTemporaryValues[E];<br>
><br>
> - llvm::DenseMap<const MaterializeTemporaryExpr *, APValue>::iterator I =<br>
> + std::map<const MaterializeTemporaryExpr *, APValue>::iterator I =<br>
> MaterializedTemporaryValues.find(E);<br>
<br>
This is a good place for auto.<br></blockquote><div><br></div><div>Ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> return I == MaterializedTemporaryValues.end() ? nullptr : &I->second;<br>
> }<br>
> Index: include/clang/AST/ASTContext.h<br>
> ===================================================================<br>
> --- include/clang/AST/ASTContext.h<br>
> +++ include/clang/AST/ASTContext.h<br>
> @@ -177,7 +177,7 @@<br>
><br>
> /// \brief Mapping from materialized temporaries with static storage duration<br>
> /// that appear in constant initializers to their evaluated values.<br>
> - llvm::DenseMap<const MaterializeTemporaryExpr*, APValue><br>
> + std::map<const MaterializeTemporaryExpr *, APValue><br>
> MaterializedTemporaryValues;<br>
><br>
> /// \brief Representation of a "canonical" template template parameter that<br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>