Remove C++ default arg side table for MS ABI ctor closures

[rnk]

Summary

We don't need a side table in ASTContext to hold CXXDefaultArgExprs. The
important part of building the CXXDefaultArgExprs was to ODR use the
default argument expressions, not to make AST nodes. Refactor the code
to only check the default argument, and remove the side table in
ASTContext which wasn't being serialized.

Fixes PR31121

[rnk]

It's worth pointing out that we have a few more side tables right next door in ASTContext, addCopyConstructorForExceptionObject / getCopyConstructorForExceptionObject. It looks like if those aren't populated, they we might copy the bytes of an exception when catching something by value.

[rsmith]

lib/Sema/SemaExprCXX.cpp

861

It would seem a lot more reasonable to actually form an expression to perform the copy of the exception object, and attach that to the ThrowExpr somehow.

Most of this work should not be MS-ABI-specific -- we're required to check that a thrown exception object can be copied (from a non-cv-qualified lvalue) regardless of ABI mode (see [except.throw]/5). In fact, for AST consumers other than IR generation, we could argue that /none/ of this should be MS-ABI specific; those consumers might want to find the implied call to the exception type's copy constructor within the guts of the throw expression, even if we never happen to use it when targeting Itanium.

So... how about we just unconditionally store a copy expression on the ThrowExpr? (With an OpaqueValueExpr representing the exception itself.)

[rnk]

lib/Sema/SemaExprCXX.cpp

861

Sounds like a good plan. I'm going to take a shot at that ArrayRef on FunctionDecl thing first, though.

include/clang/AST/ASTContext.h

public:
   bool DeclMustBeEmitted(const Decl *D);
 
   const CXXConstructorDecl *
   getCopyConstructorForExceptionObject(CXXRecordDecl *RD);
 
   void addCopyConstructorForExceptionObject(CXXRecordDecl *RD,
                                             CXXConstructorDecl *CD);
 
-  void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                       unsigned ParmIdx, Expr *DAE);
-
-  Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                        unsigned ParmIdx);
-
   void addTypedefNameForUnnamedTagDecl(TagDecl *TD, TypedefNameDecl *TND);
 
   TypedefNameDecl *getTypedefNameForUnnamedTagDecl(const TagDecl *TD);
 
   void addDeclaratorForUnnamedTagDecl(TagDecl *TD, DeclaratorDecl *DD);
 
   DeclaratorDecl *getDeclaratorForUnnamedTagDecl(const TagDecl *TD);
 

include/clang/Sema/Sema.h

BuildCXXConstructExpr(SourceLocation ConstructLoc, QualType DeclInitType,
                         CXXConstructorDecl *Constructor, bool Elidable,
                         MultiExprArg Exprs, bool HadMultipleCandidates,
                         bool IsListInitialization,
                         bool IsStdInitListInitialization, bool RequiresZeroInit,
                         unsigned ConstructKind, SourceRange ParenRange);
 
   ExprResult BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field);
 
+
+  /// Instantiate or parse a C++ default argument expression as necessary.
+  /// Return true on error.
+  bool CheckCXXDefaultArgExpr(SourceLocation CallLoc, FunctionDecl *FD,
+                              ParmVarDecl *Param);
+
   /// BuildCXXDefaultArgExpr - Creates a CXXDefaultArgExpr, instantiating
   /// the default expr if needed.
   ExprResult BuildCXXDefaultArgExpr(SourceLocation CallLoc,
                                     FunctionDecl *FD,
                                     ParmVarDecl *Param);
 
   /// FinalizeVarWithDestructor - Prepare for calling destructor on the
   /// constructed variable.

lib/AST/ASTContext.cpp

 
 void ASTContext::addCopyConstructorForExceptionObject(CXXRecordDecl *RD,
                                                       CXXConstructorDecl *CD) {
   return ABI->addCopyConstructorForExceptionObject(
       cast<CXXRecordDecl>(RD->getFirstDecl()),
       cast<CXXConstructorDecl>(CD->getFirstDecl()));
 }
 
-void ASTContext::addDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                                 unsigned ParmIdx, Expr *DAE) {
-  ABI->addDefaultArgExprForConstructor(
-      cast<CXXConstructorDecl>(CD->getFirstDecl()), ParmIdx, DAE);
-}
-
-Expr *ASTContext::getDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                                  unsigned ParmIdx) {
-  return ABI->getDefaultArgExprForConstructor(
-      cast<CXXConstructorDecl>(CD->getFirstDecl()), ParmIdx);
-}
-
 void ASTContext::addTypedefNameForUnnamedTagDecl(TagDecl *TD,
                                                  TypedefNameDecl *DD) {
   return ABI->addTypedefNameForUnnamedTagDecl(TD, DD);
 }
 
 TypedefNameDecl *
 ASTContext::getTypedefNameForUnnamedTagDecl(const TagDecl *TD) {
   return ABI->getTypedefNameForUnnamedTagDecl(TD);

lib/AST/CXXABI.h

public:
   /// Adds a mapping from class to copy constructor for this C++ ABI.
   virtual void addCopyConstructorForExceptionObject(CXXRecordDecl *,
                                                     CXXConstructorDecl *) = 0;
 
   /// Retrieves the mapping from class to copy constructor for this C++ ABI.
   virtual const CXXConstructorDecl *
   getCopyConstructorForExceptionObject(CXXRecordDecl *) = 0;
 
-  virtual void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                               unsigned ParmIdx, Expr *DAE) = 0;
-
-  virtual Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                                unsigned ParmIdx) = 0;
-
   virtual void addTypedefNameForUnnamedTagDecl(TagDecl *TD,
                                                TypedefNameDecl *DD) = 0;
 
   virtual TypedefNameDecl *
   getTypedefNameForUnnamedTagDecl(const TagDecl *TD) = 0;
 
   virtual void addDeclaratorForUnnamedTagDecl(TagDecl *TD,
                                               DeclaratorDecl *DD) = 0;

lib/AST/ItaniumCXXABI.cpp

public:
   const CXXConstructorDecl *
   getCopyConstructorForExceptionObject(CXXRecordDecl *RD) override {
     return nullptr;
   }
 
   void addCopyConstructorForExceptionObject(CXXRecordDecl *RD,
                                             CXXConstructorDecl *CD) override {}
 
-  void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                       unsigned ParmIdx, Expr *DAE) override {}
-
-  Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                        unsigned ParmIdx) override {
-    return nullptr;
-  }
-
   void addTypedefNameForUnnamedTagDecl(TagDecl *TD,
                                        TypedefNameDecl *DD) override {}
 
   TypedefNameDecl *getTypedefNameForUnnamedTagDecl(const TagDecl *TD) override {
     return nullptr;
   }
 
   void addDeclaratorForUnnamedTagDecl(TagDecl *TD,

lib/AST/MicrosoftCXXABI.cpp

unsigned getManglingNumber(const TagDecl *TD,
                              unsigned MSLocalManglingNumber) override {
     return MSLocalManglingNumber;
   }
 };
 
 class MicrosoftCXXABI : public CXXABI {
   ASTContext &Context;
   llvm::SmallDenseMap<CXXRecordDecl *, CXXConstructorDecl *> RecordToCopyCtor;
-  llvm::SmallDenseMap<std::pair<const CXXConstructorDecl *, unsigned>, Expr *>
-      CtorToDefaultArgExpr;
 
   llvm::SmallDenseMap<TagDecl *, DeclaratorDecl *>
       UnnamedTagDeclToDeclaratorDecl;
   llvm::SmallDenseMap<TagDecl *, TypedefNameDecl *>
       UnnamedTagDeclToTypedefNameDecl;
 
 public:
   MicrosoftCXXABI(ASTContext &Ctx) : Context(Ctx) { }
 
   std::pair<uint64_t, unsigned>
   getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const override;
 
   CallingConv getDefaultMethodCallConv(bool isVariadic) const override {
     if (!isVariadic &&
         Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
       return CC_X86ThisCall;
     return CC_C;
   }
 
   bool isNearlyEmpty(const CXXRecordDecl *RD) const override {
     llvm_unreachable("unapplicable to the MS ABI");
   }
 
-  void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                       unsigned ParmIdx, Expr *DAE) override {
-    CtorToDefaultArgExpr[std::make_pair(CD, ParmIdx)] = DAE;
-  }
-
-  Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD,
-                                        unsigned ParmIdx) override {
-    return CtorToDefaultArgExpr[std::make_pair(CD, ParmIdx)];
-  }
-
   const CXXConstructorDecl *
   getCopyConstructorForExceptionObject(CXXRecordDecl *RD) override {
     return RecordToCopyCtor[RD];
   }
 
   void
   addCopyConstructorForExceptionObject(CXXRecordDecl *RD,
                                        CXXConstructorDecl *CD) override {

lib/CodeGen/MicrosoftCXXABI.cpp

MicrosoftCXXABI::getAddrOfCXXCtorClosure(const CXXConstructorDecl *CD,
   // Push the this ptr.
   Args.add(RValue::get(This), CD->getThisType(getContext()));
 
   // Push the src ptr.
   if (SrcVal)
     Args.add(RValue::get(SrcVal), SrcParam.getType());
 
   // Add the rest of the default arguments.
-  std::vector<Stmt *> ArgVec;
-  for (unsigned I = IsCopy ? 1 : 0, E = CD->getNumParams(); I != E; ++I) {
-    Stmt *DefaultArg = getContext().getDefaultArgExprForConstructor(CD, I);
-    assert(DefaultArg && "sema forgot to instantiate default args");
-    ArgVec.push_back(DefaultArg);
+  SmallVector<const Stmt *, 4> ArgVec;
+  ArrayRef<ParmVarDecl *> params = CD->parameters().drop_front(IsCopy ? 1 : 0);
+  for (const ParmVarDecl *PD : params) {
+    assert(PD->hasDefaultArg() && "ctor closure lacks default args");
+    ArgVec.push_back(PD->getDefaultArg());
   }
 
   CodeGenFunction::RunCleanupsScope Cleanups(CGF);
 
   const auto *FPT = CD->getType()->castAs<FunctionProtoType>();
   CGF.EmitCallArgs(Args, FPT, llvm::makeArrayRef(ArgVec), CD, IsCopy ? 1 : 0);
 
   // Insert any ABI-specific implicit constructor arguments.

lib/Sema/SemaDeclCXX.cpp

if (CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(CurContext)) {
     if (Record->isInvalidDecl()) {
       DelayedDefaultedMemberExceptionSpecs.clear();
       DelayedExceptionSpecChecks.clear();
       return;
     }
   }
 }
 
-static void getDefaultArgExprsForConstructors(Sema &S, CXXRecordDecl *Class) {
+static void checkDefaultArgExprsForConstructors(Sema &S, CXXRecordDecl *Class) {
   // Don't do anything for template patterns.
   if (Class->getDescribedClassTemplate())
     return;
 
   CallingConv ExpectedCallingConv = S.Context.getDefaultCallingConvention(
       /*IsVariadic=*/false, /*IsCXXMethod=*/true);
 
   CXXConstructorDecl *LastExportedDefaultCtor = nullptr;
   for (Decl *Member : Class->decls()) {
     auto *CD = dyn_cast<CXXConstructorDecl>(Member);
     if (!CD) {
       // Recurse on nested classes.
       if (auto *NestedRD = dyn_cast<CXXRecordDecl>(Member))
-        getDefaultArgExprsForConstructors(S, NestedRD);
+        checkDefaultArgExprsForConstructors(S, NestedRD);
       continue;
     } else if (!CD->isDefaultConstructor() || !CD->hasAttr<DLLExportAttr>()) {
       continue;
     }
 
     CallingConv ActualCallingConv =
         CD->getType()->getAs<FunctionProtoType>()->getCallConv();
 
     // Skip default constructors with typical calling conventions and no default
     // arguments.
     unsigned NumParams = CD->getNumParams();
     if (ExpectedCallingConv == ActualCallingConv && NumParams == 0)
       continue;
 
     if (LastExportedDefaultCtor) {
       S.Diag(LastExportedDefaultCtor->getLocation(),
              diag::err_attribute_dll_ambiguous_default_ctor) << Class;
       S.Diag(CD->getLocation(), diag::note_entity_declared_at)
           << CD->getDeclName();
       return;
     }
     LastExportedDefaultCtor = CD;
 
     for (unsigned I = 0; I != NumParams; ++I) {
-      // Skip any default arguments that we've already instantiated.
-      if (S.Context.getDefaultArgExprForConstructor(CD, I))
-        continue;
-
-      Expr *DefaultArg = S.BuildCXXDefaultArgExpr(Class->getLocation(), CD,
-                                                  CD->getParamDecl(I)).get();
+      (void)S.CheckCXXDefaultArgExpr(Class->getLocation(), CD,
+                                     CD->getParamDecl(I));
       S.DiscardCleanupsInEvaluationContext();
-      S.Context.addDefaultArgExprForConstructor(CD, I, DefaultArg);
     }
   }
 }
 
 void Sema::ActOnFinishCXXNonNestedClass(Decl *D) {
   auto *RD = dyn_cast<CXXRecordDecl>(D);
 
   // Default constructors that are annotated with __declspec(dllexport) which
   // have default arguments or don't use the standard calling convention are
   // wrapped with a thunk called the default constructor closure.
   if (RD && Context.getTargetInfo().getCXXABI().isMicrosoft())
-    getDefaultArgExprsForConstructors(*this, RD);
+    checkDefaultArgExprsForConstructors(*this, RD);
 
   referenceDLLExportedClassMethods();
 }
 
 void Sema::referenceDLLExportedClassMethods() {
   if (!DelayedDllExportClasses.empty()) {
     // Calling ReferenceDllExportedMethods might cause the current function to
     // be called again, so use a local copy of DelayedDllExportClasses.

lib/Sema/SemaExpr.cpp

Sema::CreateBuiltinArraySubscriptExpr(Expr *Base, SourceLocation LLoc,
 
   assert(VK == VK_RValue || LangOpts.CPlusPlus ||
          !ResultType.isCForbiddenLValueType());
 
   return new (Context)
       ArraySubscriptExpr(LHSExp, RHSExp, ResultType, VK, OK, RLoc);
 }
 
-ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
-                                        FunctionDecl *FD,
-                                        ParmVarDecl *Param) {
+bool Sema::CheckCXXDefaultArgExpr(SourceLocation CallLoc, FunctionDecl *FD,
   if (Param->hasUnparsedDefaultArg()) {
     Diag(CallLoc,
          diag::err_use_of_default_argument_to_function_declared_later) <<
       FD << cast<CXXRecordDecl>(FD->getDeclContext())->getDeclName();
     Diag(UnparsedDefaultArgLocs[Param],
          diag::note_default_argument_declared_here);
-    return ExprError();
+    return true;
   }
   
   if (Param->hasUninstantiatedDefaultArg()) {
     Expr *UninstExpr = Param->getUninstantiatedDefaultArg();
 
     EnterExpressionEvaluationContext EvalContext(*this, PotentiallyEvaluated,
                                                  Param);
 
     // Instantiate the expression.
     MultiLevelTemplateArgumentList MutiLevelArgList
       = getTemplateInstantiationArgs(FD, nullptr, /*RelativeToPrimary=*/true);
 
     InstantiatingTemplate Inst(*this, CallLoc, Param,
                                MutiLevelArgList.getInnermost());
     if (Inst.isInvalid())
-      return ExprError();
+      return true;
     if (Inst.isAlreadyInstantiating()) {
       Diag(Param->getLocStart(), diag::err_recursive_default_argument) << FD;
       Param->setInvalidDecl();
-      return ExprError();
+      return true;
     }
 
     ExprResult Result;
     {
       // C++ [dcl.fct.default]p5:
       //   The names in the [default argument] expression are bound, and
       //   the semantic constraints are checked, at the point where the
       //   default argument expression appears.
       ContextRAII SavedContext(*this, FD);
       LocalInstantiationScope Local(*this);
       Result = SubstInitializer(UninstExpr, MutiLevelArgList,
                                 /*DirectInit*/false);
     }
     if (Result.isInvalid())
-      return ExprError();
+      return true;
 
     // Check the expression as an initializer for the parameter.
     InitializedEntity Entity
       = InitializedEntity::InitializeParameter(Context, Param);
     InitializationKind Kind
       = InitializationKind::CreateCopy(Param->getLocation(),
              /*FIXME:EqualLoc*/UninstExpr->getLocStart());
     Expr *ResultE = Result.getAs<Expr>();
 
     InitializationSequence InitSeq(*this, Entity, Kind, ResultE);
     Result = InitSeq.Perform(*this, Entity, Kind, ResultE);
     if (Result.isInvalid())
-      return ExprError();
+      return true;
 
     Result = ActOnFinishFullExpr(Result.getAs<Expr>(),
                                  Param->getOuterLocStart());
     if (Result.isInvalid())
-      return ExprError();
+      return true;
 
     // Remember the instantiated default argument.
     Param->setDefaultArg(Result.getAs<Expr>());
     if (ASTMutationListener *L = getASTMutationListener()) {
       L->DefaultArgumentInstantiated(Param);
     }
   }
 
   // If the default argument expression is not set yet, we are building it now.
   if (!Param->hasInit()) {
     Diag(Param->getLocStart(), diag::err_recursive_default_argument) << FD;
     Param->setInvalidDecl();
-    return ExprError();
+    return true;
   }
 
   // If the default expression creates temporaries, we need to
   // push them to the current stack of expression temporaries so they'll
   // be properly destroyed.
   // FIXME: We should really be rebuilding the default argument with new
   // bound temporaries; see the comment in PR5810.
   // We don't need to do that with block decls, though, because
assert(!Init->getNumObjects() &&
            "default argument expression has capturing blocks?");
   }
 
   // We already type-checked the argument, so we know it works. 
   // Just mark all of the declarations in this potentially-evaluated expression
   // as being "referenced".
   MarkDeclarationsReferencedInExpr(Param->getDefaultArg(),
                                    /*SkipLocalVariables=*/true);
-  return CXXDefaultArgExpr::Create(Context, CallLoc, Param);
+  return false;
 }
 
+ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
+                                        FunctionDecl *FD, ParmVarDecl *Param) {
+  if (CheckCXXDefaultArgExpr(CallLoc, FD, Param))
+    return ExprError();
+  return CXXDefaultArgExpr::Create(Context, CallLoc, Param);
+}
 
 Sema::VariadicCallType
 Sema::getVariadicCallType(FunctionDecl *FDecl, const FunctionProtoType *Proto,
                           Expr *Fn) {
   if (Proto && Proto->isVariadic()) {
     if (dyn_cast_or_null<CXXConstructorDecl>(FDecl))
       return VariadicConstructor;
     else if (Fn && Fn->getType()->isBlockPointerType())

lib/Sema/SemaExprCXX.cpp

for (CXXRecordDecl *Subobject : UnambiguousPublicSubobjects) {
         continue;
 
       // The copy constructor is non-trivial, create a mapping from this class
       // type to this constructor.
       // N.B.  The selection of copy constructor is not sensitive to this
       // particular throw-site.  Lookup will be performed at the catch-site to
       // ensure that the copy constructor is, in fact, accessible (via
       // friendship or any other means).
       Context.addCopyConstructorForExceptionObject(Subobject, CD);
 
       // We don't keep the instantiated default argument expressions around so
       // we must rebuild them here.
       for (unsigned I = 1, E = CD->getNumParams(); I != E; ++I) {
-        // Skip any default arguments that we've already instantiated.
-        if (Context.getDefaultArgExprForConstructor(CD, I))
-          continue;
-
-        Expr *DefaultArg =
-            BuildCXXDefaultArgExpr(ThrowLoc, CD, CD->getParamDecl(I)).get();
-        Context.addDefaultArgExprForConstructor(CD, I, DefaultArg);
+        if (CheckCXXDefaultArgExpr(ThrowLoc, CD, CD->getParamDecl(I)))
+          return true;
       }
     }
   }
 
   return false;
 }
 
 static QualType adjustCVQualifiersForCXXThisWithinLambda(

test/SemaCXX/default-arg-closures.cpp

+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fexceptions -fcxx-exceptions -fms-extensions -verify %s -std=c++11
+
+// The MS ABI has a few ways to generate constructor closures, which require
+// instantiating and checking the semantics of default arguments. Make sure we
+// do that right.
+
+// FIXME: Don't diagnose this issue twice.
+template <typename T>
+struct DependentDefaultCtorArg { // expected-note {{in instantiation of default function argument}}
+  // expected-error@+1 2 {{type 'int' cannot be used prior to '::' because it has no members}}
+  DependentDefaultCtorArg(int n = T::error);
+};
+struct 
+__declspec(dllexport) // expected-note {{due to 'ExportDefaultCtorClosure' being dllexported}}
+ExportDefaultCtorClosure // expected-note {{implicit default constructor for 'ExportDefaultCtorClosure' first required here}}
+: DependentDefaultCtorArg<int> // expected-note {{in instantiation of template class}}
+{};
+
+template <typename T>
+struct DependentDefaultCopyArg {
+  DependentDefaultCopyArg() {}
+  // expected-error@+1 {{type 'int' cannot be used prior to '::' because it has no members}}
+  DependentDefaultCopyArg(const DependentDefaultCopyArg &o, int n = T::member) {}
+};
+
+struct HasMember {
+  enum { member = 0 };
+};
+void UseDependentArg() { throw DependentDefaultCopyArg<HasMember>(); }
+
+void ErrorInDependentArg() {
+  throw DependentDefaultCopyArg<int>(); // expected-note {{required here}}
+}
+
+struct HasCleanup {
+  ~HasCleanup();
+};
+
+struct Default {
+  Default(const Default &o, int d = (HasCleanup(), 42));
+};
+
+void f(const Default &d) {
+  throw d;
+}