[MS] Don't statically initialize dllimport member function pointers

[rnk]

Summary

r306137 made dllimport pointers to member functions non-constant. This
is correct because a load must be executed to resolve any dllimported
data. However, r306137 did not account for the use of dllimport member
function pointers used as template arguments.

This change piggie-backs almost entirely on Reid's r306137, I just added
the template instantiation fix.

This fixes PR33570.

[rnk]

Did you locally add a test case for the dllimport member function pointer template argument?

lib/Sema/SemaTemplate.cpp

5704

Think we should pass in Entity as an optional parameter to isNullPointerValueTemplateArgument to share the logic?

[majnemer]

In D34714#793205, @rnk wrote:

Did you locally add a test case for the dllimport member function pointer template argument?

Arg, yes. Forgot to add the file...

lib/Sema/SemaTemplate.cpp

5704

Seems reasonable, I'll give that a go.

[rnk]

Have a minute to get to this? My attempt to work around the bug in Chromium was incomplete.

[rnk]

@majnemer will be back Monday, grabbing this.

lib/AST/ExprConstant.cpp

if (!Designator.Invalid && Designator.isOnePastTheEnd()) {
     Info.FFDiag(Loc, diag::note_constexpr_past_end, 1)
       << !Designator.Entries.empty() << !!VD << VD;
     NoteLValueLocation(Info, Base);
   }
 
   return true;
 }
 
+/// Member pointers are constant expressions unless they point to a
+/// non-virtual dllimport member function.
+static bool CheckMemberPointerConstantExpression(EvalInfo &Info,
+                                                 SourceLocation Loc,
+                                                 QualType Type,
+                                                 const APValue &Value) {
+  const ValueDecl *Member = Value.getMemberPointerDecl();
+  const auto *FD = dyn_cast_or_null<CXXMethodDecl>(Member);
+  if (!FD)
+    return true;
+  return FD->isVirtual() || !FD->hasAttr<DLLImportAttr>();
+}
+
 /// Check that this core constant expression is of literal type, and if not,
 /// produce an appropriate diagnostic.
 static bool CheckLiteralType(EvalInfo &Info, const Expr *E,
                              const LValue *This = nullptr) {
   if (!E->isRValue() || E->getType()->isLiteralType(Info.Ctx))
     return true;
 
   // C++1y: A constant initializer for an object o [...] may also invoke
static bool CheckConstantExpression(EvalInfo &Info, SourceLocation DiagLoc,
   }
 
   if (Value.isLValue()) {
     LValue LVal;
     LVal.setFrom(Info.Ctx, Value);
     return CheckLValueConstantExpression(Info, DiagLoc, Type, LVal);
   }
 
+  if (Value.isMemberPointer())
+    return CheckMemberPointerConstantExpression(Info, DiagLoc, Type, Value);
+
   // Everything else is fine.
   return true;
 }
 
 static const ValueDecl *GetLValueBaseDecl(const LValue &LVal) {
   return LVal.Base.dyn_cast<const ValueDecl*>();
 }
 

lib/Sema/SemaTemplate.cpp

 /// member constant according to C++ [temp.arg.nontype]p1.
 static bool CheckTemplateArgumentPointerToMember(Sema &S,
                                                  NonTypeTemplateParmDecl *Param,
                                                  QualType ParamType,
                                                  Expr *&ResultArg,
                                                  TemplateArgument &Converted) {
   bool Invalid = false;
 
-  // Check for a null pointer value.
   Expr *Arg = ResultArg;
-  switch (isNullPointerValueTemplateArgument(S, Param, ParamType, Arg)) {
-  case NPV_Error:
-    return true;
-  case NPV_NullPointer:
-    S.Diag(Arg->getExprLoc(), diag::warn_cxx98_compat_template_arg_null);
-    Converted = TemplateArgument(S.Context.getCanonicalType(ParamType),
-                                 /*isNullPtr*/true);
-    return false;
-  case NPV_NotNullPointer:
-    break;
-  }
-
   bool ObjCLifetimeConversion;
-  if (S.IsQualificationConversion(Arg->getType(),
-                                  ParamType.getNonReferenceType(),
-                                  false, ObjCLifetimeConversion)) {
-    Arg = S.ImpCastExprToType(Arg, ParamType, CK_NoOp,
-                              Arg->getValueKind()).get();
-    ResultArg = Arg;
-  } else if (!S.Context.hasSameUnqualifiedType(Arg->getType(),
-                ParamType.getNonReferenceType())) {
-    // We can't perform this conversion.
-    S.Diag(Arg->getLocStart(), diag::err_template_arg_not_convertible)
-      << Arg->getType() << ParamType << Arg->getSourceRange();
-    S.Diag(Param->getLocation(), diag::note_template_param_here);
-    return true;
-  }
-
-  // See through any implicit casts we added to fix the type.
-  while (ImplicitCastExpr *Cast = dyn_cast<ImplicitCastExpr>(Arg))
-    Arg = Cast->getSubExpr();
 
   // C++ [temp.arg.nontype]p1:
   //
   //   A template-argument for a non-type, non-template
   //   template-parameter shall be one of: [...]
   //
   //     -- a pointer to member expressed as described in 5.3.1.
   DeclRefExpr *DRE = nullptr;
if (ValueDecl *VD = dyn_cast<ValueDecl>(DRE->getDecl())) {
           return Invalid;
         }
       }
     }
 
     DRE = nullptr;
   }
 
+  ValueDecl *Entity = DRE ? DRE->getDecl() : nullptr;
+  NullPointerValueKind NPV;
+  // dllimport'd entities aren't constant but are available inside of template
+  // arguments.
+  if (Entity && Entity->hasAttr<DLLImportAttr>())
+    NPV = NPV_NotNullPointer;
+  else
+    NPV = isNullPointerValueTemplateArgument(S, Param, ParamType, ResultArg);
+
+  // Check for a null pointer value.
+  switch (NPV) {
+  case NPV_Error:
+    return true;
+  case NPV_NullPointer:
+    S.Diag(ResultArg->getExprLoc(), diag::warn_cxx98_compat_template_arg_null);
+    Converted = TemplateArgument(S.Context.getCanonicalType(ParamType),
+                                 /*isNullPtr*/true);
+    return false;
+  case NPV_NotNullPointer:
+    break;
+  }
+
+  if (S.IsQualificationConversion(ResultArg->getType(),
+                                  ParamType.getNonReferenceType(), false,
+                                  ObjCLifetimeConversion)) {
+    ResultArg = S.ImpCastExprToType(ResultArg, ParamType, CK_NoOp,
+                                    ResultArg->getValueKind())
+                    .get();
+  } else if (!S.Context.hasSameUnqualifiedType(
+                 ResultArg->getType(), ParamType.getNonReferenceType())) {
+    // We can't perform this conversion.
+    S.Diag(ResultArg->getLocStart(), diag::err_template_arg_not_convertible)
+        << ResultArg->getType() << ParamType << ResultArg->getSourceRange();
+    S.Diag(Param->getLocation(), diag::note_template_param_here);
+    return true;
+  }
+
   if (!DRE)
     return S.Diag(Arg->getLocStart(),
                   diag::err_template_arg_not_pointer_to_member_form)
       << Arg->getSourceRange();
 
   if (isa<FieldDecl>(DRE->getDecl()) ||
       isa<IndirectFieldDecl>(DRE->getDecl()) ||
       isa<CXXMethodDecl>(DRE->getDecl())) {

test/CodeGenCXX/dllimport-memptr-global.cpp

+// Also check that -Wglobal-constructors does the right thing. Strictly
+// speaking, this is a Sema test, but this avoids test case duplication.
+// RUN: %clang_cc1 -Wglobal-constructors %s -verify -triple i686-windows-msvc -fms-extensions -std=c++11
+//
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple i686-windows-msvc -fms-extensions -std=c++11 | FileCheck %s
+
+struct __declspec(dllimport) Single {
+  void nonvirt();
+  virtual void virt();
+};
+
+struct A { int a; };
+struct B { int b; };
+struct __declspec(dllimport) Multi : A, B {
+  void nonvirt();
+  virtual void virt();
+};
+
+struct __declspec(dllimport) Virtual : virtual A {
+  void nonvirt();
+  virtual void virt();
+};
+
+struct General;
+static_assert(sizeof(void (General::*)()) == 16, "force general memptr model");
+struct __declspec(dllimport) General {
+  void nonvirt();
+  virtual void virt();
+};
+
+auto mp_single_nv = &Single::nonvirt; // expected-warning {{global constructor}}
+auto mp_multi_nv = &Multi::nonvirt; // expected-warning {{global constructor}}
+auto mp_virtual_nv = &Virtual::nonvirt; // expected-warning {{global constructor}}
+auto mp_general_nv = &General::nonvirt; // expected-warning {{global constructor}}
+
+auto mp_single_v = &Single::virt;
+auto mp_multi_v = &Multi::virt;
+auto mp_virtual_v = &Virtual::virt;
+auto mp_general_v = &General::virt;
+
+// All of the non-virtual globals need dynamic initializers.
+
+// CHECK: @"\01?mp_single_nv@@3P8Single@@AEXXZQ1@" = global i8* null, align 4
+// CHECK: @"\01?mp_multi_nv@@3P8Multi@@AEXXZQ1@" = global { i8*, i32 } zeroinitializer, align 4
+// CHECK: @"\01?mp_virtual_nv@@3P8Virtual@@AEXXZQ1@" = global { i8*, i32, i32 } zeroinitializer, align 4
+// CHECK: @"\01?mp_general_nv@@3P8General@@AEXXZQ1@" = global { i8*, i32, i32, i32 } zeroinitializer, align 4
+
+// CHECK: @"\01?mp_single_v@@3P8Single@@AEXXZQ1@" = global i8* bitcast (void (%struct.Single*, ...)* @"\01??_9Single@@$BA@AE" to i8*), align 4
+// CHECK: @"\01?mp_multi_v@@3P8Multi@@AEXXZQ1@" = global { i8*, i32 } { i8* bitcast (void (%struct.Multi*, ...)* @"\01??_9Multi@@$BA@AE" to i8*), i32 0 }, align 4
+// CHECK: @"\01?mp_virtual_v@@3P8Virtual@@AEXXZQ1@" = global { i8*, i32, i32 } { i8* bitcast (void (%struct.Virtual*, ...)* @"\01??_9Virtual@@$BA@AE" to i8*), i32 0, i32 0 }, align 4
+// CHECK: @"\01?mp_general_v@@3P8General@@AEXXZQ1@" = global { i8*, i32, i32, i32 } { i8* bitcast (void (%struct.General*, ...)* @"\01??_9General@@$BA@AE" to i8*), i32 0, i32 0, i32 0 }, align 4
+
+// CHECK: define internal void @_GLOBAL__sub_I{{.*}}() {{.*}} {
+// CHECK:   call void @"\01??__Emp_single_nv@@YAXXZ"()
+// CHECK:   call void @"\01??__Emp_multi_nv@@YAXXZ"()
+// CHECK:   call void @"\01??__Emp_virtual_nv@@YAXXZ"()
+// CHECK:   call void @"\01??__Emp_general_nv@@YAXXZ"()
+// CHECK: }