[PATCH] D11629: APValues and Constants and MaterializedTemporaryExpr need to have stable maps

David Majnemer via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 13 15:04:41 PDT 2015 

On Thu, Aug 13, 2015 at 5:15 PM, Justin Bogner <mail at justinbogner.com>
wrote:

> David Majnemer <david.majnemer at gmail.com> writes:
> > majnemer created this revision.
> > majnemer added a reviewer: rsmith.
> > majnemer added a subscriber: cfe-commits.
> >
> > We risk iterator invalidation issues if we use DenseMap structures for
> > MaterializedTemporaryExprs.  Use a std::map to ensure that they don't
> > move around in memory.
> >
> > This fixes PR24289.
> >
> > http://reviews.llvm.org/D11629
> >
> > Files:
> >   include/clang/AST/ASTContext.h
> >   lib/AST/ASTContext.cpp
> >   lib/CodeGen/CodeGenModule.h
> >   test/CodeGenCXX/PR24289.cpp
> >
> > Index: test/CodeGenCXX/PR24289.cpp
> > ===================================================================
> > --- /dev/null
> > +++ test/CodeGenCXX/PR24289.cpp
> > @@ -0,0 +1,82 @@
> > +// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-pc-linux-gnu
> -std=c++11 | FileCheck %s
> > +
> > +namespace std {
> > +template <class T>
> > +struct initializer_list {
> > +  const T *Begin;
> > +  __SIZE_TYPE__ Size;
> > +
> > +  constexpr initializer_list(const T *B, __SIZE_TYPE__ S)
> > +      : Begin(B), Size(S) {}
> > +};
> > +}
> > +
> > +void f() {
> > +  static std::initializer_list<std::initializer_list<int>> a{
> > +      {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0},
> {0}};
> > +  static std::initializer_list<std::initializer_list<int>> b{
> > +      {0}, {0}, {0}, {0}};
> > +  static std::initializer_list<std::initializer_list<int>> c{
> > +      {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}};
> > +  static std::initializer_list<std::initializer_list<int>> d{
> > +      {0}, {0}, {0}, {0}, {0}};
> > +  static std::initializer_list<std::initializer_list<int>> e{
> > +      {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}, {0}};
> > +}
> > +
> > +// CHECK-DAG: @_ZZ1fvE1a = internal global %{{.*}} { %{{.*}}*
> getelementptr inbounds ([14 x %{{.*}}], [14 x %{{.*}}]
> > +// CHECK-DAG: * @_ZGRZ1fvE1a_, i32 0, i32 0), i64 14 }
> > +// CHECK-DAG: @_ZGRZ1fvE1a0_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a1_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a2_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a3_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a4_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a5_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a6_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a7_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a8_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1a9_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1aA_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1aB_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1aC_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1aD_ = internal constant [1 x i32]
> zeroinitializer
> > +// 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 }]
> > +// CHECK-DAG: @_ZZ1fvE1b = internal global %{{.*}} { %{{.*}}*
> getelementptr inbounds ([4 x %{{.*}}], [4 x %{{.*}}]*
> > +// CHECK-DAG: @_ZGRZ1fvE1b_, i32 0, i32 0), i64 4 }
> > +// CHECK-DAG: @_ZGRZ1fvE1b0_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1b1_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1b2_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1b3_ = internal constant [1 x i32]
> zeroinitializer
> > +// 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 }]
> > +// CHECK-DAG: @_ZZ1fvE1c = internal global %{{.*}} { %{{.*}}*
> getelementptr inbounds ([9 x %{{.*}}], [9 x %{{.*}}]*
> > +// CHECK-DAG: @_ZGRZ1fvE1c_, i32 0, i32 0), i64 9 }
> > +// CHECK-DAG: @_ZGRZ1fvE1c0_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c1_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c2_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c3_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c4_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c5_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c6_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c7_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1c8_ = internal constant [1 x i32]
> zeroinitializer
> > +// 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 }]
> > +// CHECK-DAG: @_ZZ1fvE1d = internal global %{{.*}} { %{{.*}}*
> getelementptr inbounds ([5 x %{{.*}}], [5 x %{{.*}}]* @_ZGRZ1fvE1d_, i32 0,
> i32 0), i64 5 }
> > +// CHECK-DAG: @_ZGRZ1fvE1d0_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1d1_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1d2_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1d3_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1d4_ = internal constant [1 x i32]
> zeroinitializer
> > +// 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 }]
> > +// CHECK-DAG: @_ZZ1fvE1e = internal global %{{.*}} { %{{.*}}*
> getelementptr inbounds ([11 x %{{.*}}], [11 x %{{.*}}]* @_ZGRZ1fvE1e_, i32
> 0, i32 0), i64 11 }
> > +// CHECK-DAG: @_ZGRZ1fvE1e0_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e1_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e2_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e3_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e4_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e5_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e6_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e7_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e8_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1e9_ = internal constant [1 x i32]
> zeroinitializer
> > +// CHECK-DAG: @_ZGRZ1fvE1eA_ = internal constant [1 x i32]
> zeroinitializer
> > +// 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 }]
>
> I don't suppose we can reduce this test case at all? Maybe a unit test
> would be workable to expose this?
>

You suppose correctly, this is maximally reduced.  The bug only exposes
itself due to a DenseMap regrowing at just the wrong time.


>
> > Index: lib/CodeGen/CodeGenModule.h
> > ===================================================================
> > --- lib/CodeGen/CodeGenModule.h
> > +++ lib/CodeGen/CodeGenModule.h
> > @@ -374,7 +374,7 @@
> >    llvm::DenseMap<llvm::Constant *, llvm::GlobalVariable *>
> ConstantStringMap;
> >    llvm::DenseMap<const Decl*, llvm::Constant *> StaticLocalDeclMap;
> >    llvm::DenseMap<const Decl*, llvm::GlobalVariable*>
> StaticLocalDeclGuardMap;
> > -  llvm::DenseMap<const Expr*, llvm::Constant *>
> MaterializedGlobalTemporaryMap;
> > +  std::map<const Expr*, llvm::Constant *>
> MaterializedGlobalTemporaryMap;
>
> A comment explaining why DenseMap isn't appropriate would be nice.
>

Sure.


>
> >
> >    llvm::DenseMap<QualType, llvm::Constant *> AtomicSetterHelperFnMap;
> >    llvm::DenseMap<QualType, llvm::Constant *> AtomicGetterHelperFnMap;
> > Index: lib/AST/ASTContext.cpp
> > ===================================================================
> > --- lib/AST/ASTContext.cpp
> > +++ lib/AST/ASTContext.cpp
> > @@ -8556,7 +8556,7 @@
> >    if (MayCreate)
> >      return &MaterializedTemporaryValues[E];
> >
> > -  llvm::DenseMap<const MaterializeTemporaryExpr *, APValue>::iterator I
> =
> > +  std::map<const MaterializeTemporaryExpr *, APValue>::iterator I =
> >        MaterializedTemporaryValues.find(E);
>
> This is a good place for auto.
>

Ok.


>
> >    return I == MaterializedTemporaryValues.end() ? nullptr : &I->second;
> >  }
> > Index: include/clang/AST/ASTContext.h
> > ===================================================================
> > --- include/clang/AST/ASTContext.h
> > +++ include/clang/AST/ASTContext.h
> > @@ -177,7 +177,7 @@
> >
> >    /// \brief Mapping from materialized temporaries with static storage
> duration
> >    /// that appear in constant initializers to their evaluated values.
> > -  llvm::DenseMap<const MaterializeTemporaryExpr*, APValue>
> > +  std::map<const MaterializeTemporaryExpr *, APValue>
> >      MaterializedTemporaryValues;
> >
> >    /// \brief Representation of a "canonical" template template
> parameter that
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150813/43f30786/attachment-0001.html>

More information about the cfe-commits mailing list