Allow dllimport non-type template arguments in C++17

[rnk]

Summary

Fixes PR35772.

[rsmith]

clang/lib/AST/ExprConstant.cpp

10377–10393

Neither EM_ConstantFold nor EM_IgnoreSideEffects seem like the right evaluation modes to be using to evaluate a non-type template argument. I would expect EM_ConstantExpression to be used for both cases.

[rsmith]

clang/lib/AST/ExprConstant.cpp

10377–10393

Oh, I see, we're allowing side-effects here and then issuing a constant-folding warning elsewhere if there actually were any. *sigh*

I would expect we can get away with not doing that for template arguments. It'd be worth testing whether GCC actually allows constant folding there, or requires a real constant expression.

clang/lib/Sema/SemaOverload.cpp

5411–5412

Don't we get here for CCEKinds other than the non-type template argument case?

[rnk]

clang/lib/AST/ExprConstant.cpp

10377–10393

"Allowing side effects" here looks like it really means allowing non-constant subexpressions when those subexpressions would be discarded:

template <int *N> struct Foo {};
int x;
Foo<(x, &x)> f;

constexpr bool is_true = true;

int *fn() {
  if constexpr (((void)x, &x) != nullptr) {
    return &x;
  } else {
    return nullptr;
  }

  if constexpr (is_true || x == 2) {
    return &x;
  } else {
    return nullptr;
  }
}

Clang accepts this, GCC rejects a few. I'm guessing we want that kind of behavior for if constexpr conditions, but maybe not for non-type template parameters.

So, it sounds like we should check the CCE kind in SemaOverload, and use a more restrictive constant expression evaluation mode in that case. Perhaps even a new one for non-type template arguments.

clang/lib/Sema/SemaOverload.cpp

5411–5412

You're right, but I wasn't able to construct a test case where we would call CheckConvertedConstantExpression and we would reject it today because it is dllimported. This was my best idea, using if constexpr:

struct __declspec(dllimport) Foo {
  static constexpr bool imported_foo = true;
};
const bool some_bool = false;
const bool *f() {
  if constexpr (Foo::imported_foo) {
    return &Foo::imported_foo;
  } else {
    return &some_bool;
  }
}

Any other ideas on how to get more coverage of this path through CheckConvertedConstantExpression?

[rnk]

• revise as discussed

[rsmith]

clang/include/clang/AST/Expr.h

103

If we end up moving this out of Sema into the clang namespace, it needs a longer name than this.

661–663

Seems strange to pass a converted constant expression kind to an 'evaluate as core constant expression' function. And it seems like we don't need the kind here, just an "evaluating for emission w/relocations" vs "evaluating for mangling" enum.

Also, we need to evaluate non-type template arguments as constant expressions, not just as core constant expressions, which the implementation does, but the name and comment here don't reflect. (The difference is that you can't result in a pointer/reference to a temporary or automatic / thread storage duration entity.)

[rnk]

• don't expose CCEK, use a bool

[rnk]

clang/include/clang/AST/Expr.h

661–663

So... what are these things? Converted constant expressions? What are we evaluating them as? I guess they're not rvalues or lvalues.

[rsmith]

clang/include/clang/AST/Expr.h

661–663

I think this should just be called EvaluateAsConstantExpr, and you should drop all the "core"s throughout. The difference between a constant expression and a core constant expression is that a core constant expression allows the evaluated value to refer to entities with automatic storage duration etc, which is exactly the case you want to disallow here :)

I also don't think you need the ParamType, and can instead just look at whether the expression itself is an rvalue.

663

I would prefer an enum { EvaluateForCodeGen, EvaluateForMangling } over a bool IsTemplateArg; I would not be surprised if we find other cases where we want to evaluate an expression for mangling purposes only.

clang/lib/AST/ExprConstant.cpp

707

No "core" here.

709

I don't think we need this. See below.

10384–10385

Instead of a new EvaluationMode which is actually not used by evaluation, we could pass a parameter into CheckLValueConstantExpression to indicate if we're in the "for-mangling" mode.

10397

If you switch from using ParamType->isReferenceType() to asking the expression for its value category, you don't need the implicit-lvalue-to-rvalue-conversion semantics of EvaluateAsRValue, and can just call ::Evaluate here followed by a call to CheckConstantExpression (passing the latter the IsTemplateArg flag).

In fact, you don't need the separate case for lvalues above, either, since ::Evaluate does the right thing for an lvalue expression by itself.

clang/lib/Sema/SemaOverload.cpp

5411–5412

All the other forms of CCEKind also set RequireInt to true, and so any case your check catches would also be caught on the line below.

[rnk]

• getting closer

[rnk]

clang/include/clang/AST/Expr.h

662

I expect we could come up with a better name, but is this closer to what you had in mind?

[rsmith]

clang/lib/AST/ExprConstant.cpp

1881–1912

Usage should be passed into these recursive calls to CheckConstantExpression, although that's NFC for now, until/unless we start allowing class types as template parameters.

[rnk]

Thanks for the guidance!

clang/lib/AST/ExprConstant.cpp

1881–1912

Makes sense, done.

clang/include/clang/AST/Expr.h

struct SubobjectAdjustment {
   SubobjectAdjustment(const MemberPointerType *MPT, Expr *RHS)
     : Kind(MemberPointerAdjustment) {
     this->Ptr.MPT = MPT;
     this->Ptr.RHS = RHS;
   }
 };
 
 /// Expr - This represents one expression.  Note that Expr's are subclasses of
 /// Stmt.  This allows an expression to be transparently used any place a Stmt
 /// is required.
 ///
 class Expr : public Stmt {
   QualType TR;
 
 protected:
   Expr(StmtClass SC, QualType T, ExprValueKind VK, ExprObjectKind OK,
        bool TD, bool VD, bool ID, bool ContainsUnexpandedParameterPack)
public:
   /// of a call to the given function with the given arguments, inside an
   /// unevaluated context. Returns true if the expression could be folded to a
   /// constant.
   bool EvaluateWithSubstitution(APValue &Value, ASTContext &Ctx,
                                 const FunctionDecl *Callee,
                                 ArrayRef<const Expr*> Args,
                                 const Expr *This = nullptr) const;
 
+  /// Evaluate an expression that is required to be a core constant expression.
+  bool EvaluateAsCoreConstExpr(EvalResult &Result, QualType ParamType,
+                               bool IsTemplateArg, const ASTContext &Ctx) const;
+
   /// If the current Expr is a pointer, this will try to statically
   /// determine the number of bytes available where the pointer is pointing.
   /// Returns true if all of the above holds and we were able to figure out the
   /// size, false otherwise.
   ///
   /// \param Type - How to evaluate the size of the Expr, as defined by the
   /// "type" parameter of __builtin_object_size
   bool tryEvaluateObjectSize(uint64_t &Result, ASTContext &Ctx,

clang/lib/AST/ExprConstant.cpp

enum EvaluationMode {
       EM_ConstantExpression,
 
       /// Evaluate as a potential constant expression. Keep going if we hit a
       /// construct that we can't evaluate yet (because we don't yet know the
       /// value of something) but stop if we hit something that could never be
       /// a constant expression.
       EM_PotentialConstantExpression,
 
+      /// Evaluate as a C++17 non-type template argument, which is a core
+      /// constant expression with a special case for dllimport declarations.
+      EM_TemplateArgument,
+
       /// Fold the expression to a constant. Stop if we hit a side-effect that
       /// we can't model.
       EM_ConstantFold,
 
       /// Evaluate the expression looking for integer overflow and similar
       /// issues. Don't worry about side-effects, and try to visit all
       /// subexpressions.
       EM_EvaluateForOverflow,
bool CheckCallLimit(SourceLocation Loc) {
       }
       if (CallStackDepth <= getLangOpts().ConstexprCallDepth)
         return true;
       FFDiag(Loc, diag::note_constexpr_depth_limit_exceeded)
         << getLangOpts().ConstexprCallDepth;
       return false;
     }
 
+    /// Return true if this declaration is dllimport and we cannot treat the
+    /// address as a constant expression. Generally, we do not want to constant
+    /// fold dllimport declarations unless they are used in a non-type template
+    /// parameter.
+    bool isNonConstDllImportDecl(const Decl *D) {
+      return EvalMode != EM_TemplateArgument && D->hasAttr<DLLImportAttr>();
+    }
+
     CallStackFrame *getCallFrame(unsigned CallIndex) {
       assert(CallIndex && "no call index in getCallFrame");
       // We will eventually hit BottomFrame, which has Index 1, so Frame can't
       // be null in this loop.
       CallStackFrame *Frame = CurrentCall;
       while (Frame->Index > CallIndex)
         Frame = Frame->Caller;
       return (Frame->Index == CallIndex) ? Frame : nullptr;
OptionalDiagnostic Diag(SourceLocation Loc, diag::kind DiagId,
           case EM_IgnoreSideEffects:
           case EM_EvaluateForOverflow:
             if (!HasFoldFailureDiagnostic)
               break;
             // We've already failed to fold something. Keep that diagnostic.
             LLVM_FALLTHROUGH;
           case EM_ConstantExpression:
           case EM_PotentialConstantExpression:
+          case EM_TemplateArgument:
           case EM_ConstantExpressionUnevaluated:
           case EM_PotentialConstantExpressionUnevaluated:
           case EM_OffsetFold:
             HasActiveDiagnostic = false;
             return OptionalDiagnostic();
           }
         }
 
bool keepEvaluatingAfterSideEffect() {
       switch (EvalMode) {
       case EM_PotentialConstantExpression:
       case EM_PotentialConstantExpressionUnevaluated:
       case EM_EvaluateForOverflow:
       case EM_IgnoreSideEffects:
         return true;
 
       case EM_ConstantExpression:
+      case EM_TemplateArgument:
       case EM_ConstantExpressionUnevaluated:
       case EM_ConstantFold:
       case EM_OffsetFold:
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
 
bool keepEvaluatingAfterUndefinedBehavior() {
       case EM_IgnoreSideEffects:
       case EM_ConstantFold:
       case EM_OffsetFold:
         return true;
 
       case EM_PotentialConstantExpression:
       case EM_PotentialConstantExpressionUnevaluated:
       case EM_ConstantExpression:
+      case EM_TemplateArgument:
       case EM_ConstantExpressionUnevaluated:
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
 
     /// Note that we hit something that was technically undefined behavior, but
     /// that we can evaluate past it (such as signed overflow or floating-point
bool keepEvaluatingAfterFailure() {
 
       switch (EvalMode) {
       case EM_PotentialConstantExpression:
       case EM_PotentialConstantExpressionUnevaluated:
       case EM_EvaluateForOverflow:
         return true;
 
       case EM_ConstantExpression:
+      case EM_TemplateArgument:
       case EM_ConstantExpressionUnevaluated:
       case EM_ConstantFold:
       case EM_IgnoreSideEffects:
       case EM_OffsetFold:
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
static bool CheckLValueConstantExpression(EvalInfo &Info, SourceLocation Loc,
 
   if (const ValueDecl *VD = Base.dyn_cast<const ValueDecl*>()) {
     if (const VarDecl *Var = dyn_cast<const VarDecl>(VD)) {
       // Check if this is a thread-local variable.
       if (Var->getTLSKind())
         return false;
 
       // A dllimport variable never acts like a constant.
-      if (Var->hasAttr<DLLImportAttr>())
+      if (Info.isNonConstDllImportDecl(Var))
         return false;
     }
     if (const auto *FD = dyn_cast<const FunctionDecl>(VD)) {
       // __declspec(dllimport) must be handled very carefully:
       // We must never initialize an expression with the thunk in C++.
       // Doing otherwise would allow the same id-expression to yield
       // different addresses for the same function in different translation
       // units.  However, this means that we must dynamically initialize the
       // expression with the contents of the import address table at runtime.
       //
       // The C language has no notion of ODR; furthermore, it has no notion of
       // dynamic initialization.  This means that we are permitted to
       // perform initialization with the address of the thunk.
-      if (Info.getLangOpts().CPlusPlus && FD->hasAttr<DLLImportAttr>())
+      if (Info.getLangOpts().CPlusPlus && Info.isNonConstDllImportDecl(FD))
         return false;
     }
   }
 
   // Allow address constant expressions to be past-the-end pointers. This is
   // an extension: the standard requires them to point to an object.
   if (!IsReferenceType)
     return true;

 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>();
+  return FD->isVirtual() || !Info.isNonConstDllImportDecl(FD);
 }
 
 /// 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;
if (const AtomicType *AT = Type->getAs<AtomicType>())
     Type = AT->getValueType();
 
   // Core issue 1454: For a literal constant expression of array or class type,
   // each subobject of its value shall have been initialized by a constant
   // expression.
   if (Value.isArray()) {
     QualType EltTy = Type->castAsArrayTypeUnsafe()->getElementType();
     for (unsigned I = 0, N = Value.getArrayInitializedElts(); I != N; ++I) {
       if (!CheckConstantExpression(Info, DiagLoc, EltTy,
                                    Value.getArrayInitializedElt(I)))
         return false;
     }
     if (!Value.hasArrayFiller())
       return true;
     return CheckConstantExpression(Info, DiagLoc, EltTy,
                                    Value.getArrayFiller());
   }
   if (Value.isUnion() && Value.getUnionField()) {
     return CheckConstantExpression(Info, DiagLoc,
                                    Value.getUnionField()->getType(),
                                    Value.getUnionValue());
   }
   if (Value.isStruct()) {
     RecordDecl *RD = Type->castAs<RecordType>()->getDecl();
     if (const CXXRecordDecl *CD = dyn_cast<CXXRecordDecl>(RD)) {
       unsigned BaseIndex = 0;
       for (CXXRecordDecl::base_class_const_iterator I = CD->bases_begin(),
              End = CD->bases_end(); I != End; ++I, ++BaseIndex) {
         if (!CheckConstantExpression(Info, DiagLoc, I->getType(),
                                      Value.getStructBase(BaseIndex)))
           return false;
       }
     }
     for (const auto *I : RD->fields()) {
       if (I->isUnnamedBitfield())
         continue;
 
       if (!CheckConstantExpression(Info, DiagLoc, I->getType(),
                                    Value.getStructField(I->getFieldIndex())))
         return false;
     }
   }
 
   if (Value.isLValue()) {
     LValue LVal;
     LVal.setFrom(Info.Ctx, Value);
     return CheckLValueConstantExpression(Info, DiagLoc, Type, LVal);
   }
case Builtin::BI__builtin_object_size: {
 
     if (E->getArg(0)->HasSideEffects(Info.Ctx))
       return Success((Type & 2) ? 0 : -1, E);
 
     // Expression had no side effects, but we couldn't statically determine the
     // size of the referenced object.
     switch (Info.EvalMode) {
     case EvalInfo::EM_ConstantExpression:
+    case EvalInfo::EM_TemplateArgument:
     case EvalInfo::EM_PotentialConstantExpression:
     case EvalInfo::EM_ConstantFold:
     case EvalInfo::EM_EvaluateForOverflow:
     case EvalInfo::EM_IgnoreSideEffects:
     case EvalInfo::EM_OffsetFold:
       // Leave it to IR generation.
       return Error(E);
     case EvalInfo::EM_ConstantExpressionUnevaluated:
if (!EvaluateLValue(this, LV, Info) || Result.HasSideEffects ||
       !CheckLValueConstantExpression(Info, getExprLoc(),
                                      Ctx.getLValueReferenceType(getType()), LV))
     return false;
 
   LV.moveInto(Result.Val);
   return true;
 }
 
+bool Expr::EvaluateAsCoreConstExpr(EvalResult &Result, QualType ParamType,
+                                   bool IsTemplateArg,
+                                   const ASTContext &Ctx) const {
+  EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
+  if (IsTemplateArg)
+    EM = EvalInfo::EM_TemplateArgument;
+
+  // A reference must be an lvalue.
+  if (ParamType->isReferenceType()) {
+    EvalInfo Info(Ctx, Result, EM);
+    LValue LV;
+    if (!EvaluateLValue(this, LV, Info) || Result.HasSideEffects ||
+        !CheckLValueConstantExpression(
+            Info, getExprLoc(), Ctx.getLValueReferenceType(getType()), LV))
+      return false;
+
+    LV.moveInto(Result.Val);
+    return true;
+  }
+
+  // Otherwise, evaluate it as an rvalue.
+  bool IsConst;
+  if (FastEvaluateAsRValue(this, Result, Ctx, IsConst))
+    return IsConst;
+  EvalInfo Info(Ctx, Result, EM);
+  return ::EvaluateAsRValue(Info, this, Result.Val);
+}
+
 bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
                                  const VarDecl *VD,
                             SmallVectorImpl<PartialDiagnosticAt> &Notes) const {
   // FIXME: Evaluating initializers for large array and record types can cause
   // performance problems. Only do so in C++11 for now.
   if (isRValue() && (getType()->isArrayType() || getType()->isRecordType()) &&
       !Ctx.getLangOpts().CPlusPlus11)
     return false;

clang/lib/Sema/SemaOverload.cpp

if (Result.get()->isValueDependent()) {
     return Result;
   }
 
   // Check the expression is a constant expression.
   SmallVector<PartialDiagnosticAt, 8> Notes;
   Expr::EvalResult Eval;
   Eval.Diag = &Notes;
 
-  if ((T->isReferenceType()
-           ? !Result.get()->EvaluateAsLValue(Eval, S.Context)
-           : !Result.get()->EvaluateAsRValue(Eval, S.Context)) ||
+  if (!Result.get()->EvaluateAsCoreConstExpr(
+          Eval, T, CCE == Sema::CCEK_TemplateArg, S.Context) ||
       (RequireInt && !Eval.Val.isInt())) {
     // The expression can't be folded, so we can't keep it at this position in
     // the AST.
     Result = ExprError();
   } else {
     Value = Eval.Val;
 
     if (Notes.empty()) {

clang/test/SemaCXX/dllimport-constexpr.cpp

+// RUN: %clang_cc1 -std=c++14 %s -verify -fms-extensions -triple x86_64-windows-msvc
+// RUN: %clang_cc1 -std=c++17 %s -verify -fms-extensions -triple x86_64-windows-msvc
+
+__declspec(dllimport) void imported_func();
+__declspec(dllimport) int imported_int;
+struct Foo {
+  void __declspec(dllimport) imported_method();
+};
+
+// Instantiation is OK.
+template <void (*FP)()> struct TemplateFnPtr {
+  static void getit() { FP(); }
+};
+template <void (&FP)()> struct TemplateFnRef {
+  static void getit() { FP(); }
+};
+void instantiate1() {
+  TemplateFnPtr<&imported_func>::getit();
+  TemplateFnRef<imported_func>::getit();
+}
+
+// Check variable template instantiation.
+template <int *GI> struct TemplateIntPtr {
+  static int getit() { return *GI; }
+};
+template <int &GI> struct TemplateIntRef {
+  static int getit() { return GI; }
+};
+int instantiate2() {
+  int r = 0;
+  r += TemplateIntPtr<&imported_int>::getit();
+  r += TemplateIntRef<imported_int>::getit();
+  return r;
+}
+
+// Member pointer instantiation.
+template <void (Foo::*MP)()> struct TemplateMemPtr { };
+TemplateMemPtr<&Foo::imported_method> instantiate_mp;
+
+// constexpr initialization doesn't work for dllimport things.
+// expected-error@+1{{must be initialized by a constant expression}}
+constexpr void (*constexpr_import_func)() = &imported_func;
+// expected-error@+1{{must be initialized by a constant expression}}
+constexpr int *constexpr_import_int = &imported_int;
+// expected-error@+1{{must be initialized by a constant expression}}
+constexpr void (Foo::*constexpr_memptr)() = &Foo::imported_method;
+
+// We make dynamic initializers for 'const' globals, but not constexpr ones.
+void (*const const_import_func)() = &imported_func;
+int *const const_import_int = &imported_int;
+void (Foo::*const const_memptr)() = &Foo::imported_method;
+
+// Check that using a non-type template parameter for constexpr global
+// initialization is correctly diagnosed during template instantiation.
+template <void (*FP)()> struct StaticConstexpr {
+  // expected-error@+1{{must be initialized by a constant expression}}
+  static constexpr void (*g_fp)() = FP;
+};
+void instantiate3() {
+  // expected-note@+1 {{requested here}}
+  StaticConstexpr<imported_func>::g_fp();
+}

clang/test/SemaCXX/dllimport-memptr.cpp

 // RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -verify -std=c++17 %s
 
 // expected-no-diagnostics
 
 struct __declspec(dllimport) Foo { int get_a(); };
 template <int (Foo::*Getter)()> struct HasValue { };
 HasValue<&Foo::get_a> hv;