APValues and Constants and MaterializedTemporaryExpr need to have stable maps

[majnemer]

Summary

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.

[rsmith]

include/clang/AST/ASTContext.h

180 ↗

(On Diff #30999)

I would prefer a DenseMap<const MaterializedTemporaryExpr*, APValue*>, with APValues allocated by the ASTContext's BumpPtrAllocator.

[majnemer]

• Address Justin's review comments.
• Address Richard's review comments.

[rsmith]

lib/AST/ASTContext.cpp

8557 ↗

(On Diff #32100)

Maybe use a reference here rather than a pointer.

8559–8560 ↗

(On Diff #32100)

More succinctly expressed as:

MTVI = new (*this) APValue;

8565–8566 ↗

(On Diff #32100)

This is now just MaterializedTemporaryValues.lookup(E)

lib/CodeGen/CodeGenModule.h

377 ↗

(On Diff #32100)

Can you fix this instead by not holding onto Slot in CodeGenModule::GetAddrOfGlobalTemporary? (Do the map lookup again at the end of the function.) Twice as many DenseMap lookups seems likely to be cheaper than switching to a std::map.

[majnemer]

• Address Richard's latest review comments.

[rsmith]

LGTM

cfe/trunk/include/clang/AST/ASTContext.h

class ASTContext : public RefCountedBase<ASTContext> {
   llvm::DenseMap<const VarDecl*, Expr*> BlockVarCopyInits;
     
   /// \brief Mapping from class scope functions specialization to their
   /// template patterns.
   llvm::DenseMap<const FunctionDecl*, FunctionDecl*>
     ClassScopeSpecializationPattern;
 
   /// \brief Mapping from materialized temporaries with static storage duration
-  /// that appear in constant initializers to their evaluated values.
-  llvm::DenseMap<const MaterializeTemporaryExpr*, APValue>
+  /// that appear in constant initializers to their evaluated values.  These are
+  /// allocated in a std::map because their address must be stable.
+  llvm::DenseMap<const MaterializeTemporaryExpr *, APValue *>
     MaterializedTemporaryValues;
 
   /// \brief Representation of a "canonical" template template parameter that
   /// is used in canonical template names.
   class CanonicalTemplateTemplateParm : public llvm::FoldingSetNode {
     TemplateTemplateParmDecl *Parm;
     
   public:

cfe/trunk/lib/AST/ASTContext.cpp

unsigned ASTContext::getParameterIndex(const ParmVarDecl *D) const {
   return I->second;
 }
 
 APValue *
 ASTContext::getMaterializedTemporaryValue(const MaterializeTemporaryExpr *E,
                                           bool MayCreate) {
   assert(E && E->getStorageDuration() == SD_Static &&
          "don't need to cache the computed value for this temporary");
-  if (MayCreate)
-    return &MaterializedTemporaryValues[E];
+  if (MayCreate) {
+    APValue *&MTVI = MaterializedTemporaryValues[E];
+    if (!MTVI)
+      MTVI = new (*this) APValue;
+    return MTVI;
+  }
 
-  llvm::DenseMap<const MaterializeTemporaryExpr *, APValue>::iterator I =
-      MaterializedTemporaryValues.find(E);
-  return I == MaterializedTemporaryValues.end() ? nullptr : &I->second;
+  return MaterializedTemporaryValues.lookup(E);
 }
 
 bool ASTContext::AtomicUsesUnsupportedLibcall(const AtomicExpr *E) const {
   const llvm::Triple &T = getTargetInfo().getTriple();
   if (!T.isOSDarwin())
     return false;
 
   if (!(T.isiOS() && T.isOSVersionLT(7)) &&

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

llvm::Constant *CodeGenModule::GetAddrOfGlobalTemporary(
   const auto *VD = cast<VarDecl>(E->getExtendingDecl());
 
   // If we're not materializing a subobject of the temporary, keep the
   // cv-qualifiers from the type of the MaterializeTemporaryExpr.
   QualType MaterializedType = Init->getType();
   if (Init == E->GetTemporaryExpr())
     MaterializedType = E->getType();
 
-  llvm::Constant *&Slot = MaterializedGlobalTemporaryMap[E];
-  if (Slot)
+  if (llvm::Constant *Slot = MaterializedGlobalTemporaryMap[E])
     return Slot;
 
   // FIXME: If an externally-visible declaration extends multiple temporaries,
   // we need to give each temporary the same name in every translation unit (and
   // we also need to make the temporaries externally-visible).
   SmallString<256> Name;
   llvm::raw_svector_ostream Out(Name);
   getCXXABI().getMangleContext().mangleReferenceTemporary(
auto *GV = new llvm::GlobalVariable(
       AddrSpace);
   setGlobalVisibility(GV, VD);
   GV->setAlignment(
       getContext().getTypeAlignInChars(MaterializedType).getQuantity());
   if (supportsCOMDAT() && GV->isWeakForLinker())
     GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
   if (VD->getTLSKind())
     setTLSMode(GV, *VD);
-  Slot = GV;
+  MaterializedGlobalTemporaryMap[E] = GV;
   return GV;
 }
 
 /// EmitObjCPropertyImplementations - Emit information for synthesized
 /// properties for an implementation.
 void CodeGenModule::EmitObjCPropertyImplementations(const
                                                     ObjCImplementationDecl *D) {
   for (const auto *PID : D->property_impls()) {

cfe/trunk/test/CodeGenCXX/PR24289.cpp

+// 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 }]