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

[rnk]

Summary

Fixes PR35772.

[rsmith]

clang/lib/AST/ExprConstant.cpp

10166–10182 ↗

(On Diff #134324)

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

10166–10182 ↗

(On Diff #134324)

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

5401 ↗

(On Diff #134324)

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

[rnk]

clang/lib/AST/ExprConstant.cpp

10166–10182 ↗

(On Diff #134324)

"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

5401 ↗

(On Diff #134324)

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 ↗

(On Diff #134510)

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

670–672 ↗

(On Diff #134510)

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

670–672 ↗

(On Diff #134510)

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

663 ↗

(On Diff #146025)

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.

670–672 ↗

(On Diff #134510)

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.

clang/lib/AST/ExprConstant.cpp

707 ↗

(On Diff #146025)

No "core" here.

709 ↗

(On Diff #146025)

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

10384–10385 ↗

(On Diff #146025)

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 ↗

(On Diff #146025)

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

5401 ↗

(On Diff #134324)

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 ↗

(On Diff #146039)

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

1871–1902 ↗

(On Diff #146039)

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

1871–1902 ↗

(On Diff #146039)

Makes sense, done.

include/clang/AST/Expr.h

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;
 
+  /// Indicates how the constant expression will be used.
+  enum ConstExprUsage { EvaluateForCodeGen, EvaluateForMangling };
+
+  /// Evaluate an expression that is required to be a constant expression.
+  bool EvaluateAsConstantExpr(EvalResult &Result, ConstExprUsage Usage,
+                              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,

lib/AST/ExprConstant.cpp

else
     Info.Note(Base.get<const Expr*>()->getExprLoc(),
               diag::note_constexpr_temporary_here);
 }
 
 /// Check that this reference or pointer core constant expression is a valid
 /// value for an address or reference constant expression. Return true if we
 /// can fold this expression, whether or not it's a constant expression.
 static bool CheckLValueConstantExpression(EvalInfo &Info, SourceLocation Loc,
-                                          QualType Type, const LValue &LVal) {
+                                          QualType Type, const LValue &LVal,
+                                          Expr::ConstExprUsage Usage) {
   bool IsReferenceType = Type->isReferenceType();
 
   APValue::LValueBase Base = LVal.getLValueBase();
   const SubobjectDesignator &Designator = LVal.getLValueDesignator();
 
   // Check that the object is a global. Note that the fake 'this' object we
   // manufacture when checking potential constant expressions is conservatively
   // assumed to be global here.
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 (Usage == Expr::EvaluateForCodeGen && Var->hasAttr<DLLImportAttr>())
         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 && Usage == Expr::EvaluateForCodeGen &&
+          FD->hasAttr<DLLImportAttr>())
         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 CheckLValueConstantExpression(EvalInfo &Info, SourceLocation Loc,
   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 APValue &Value,
+                                                 Expr::ConstExprUsage Usage) {
   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 Usage == Expr::EvaluateForMangling || 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;
static bool CheckLiteralType(EvalInfo &Info, const Expr *E,
   else
     Info.FFDiag(E, diag::note_invalid_subexpr_in_const_expr);
   return false;
 }
 
 /// Check that this core constant expression value is a valid value for a
 /// constant expression. If not, report an appropriate diagnostic. Does not
 /// check that the expression is of literal type.
-static bool CheckConstantExpression(EvalInfo &Info, SourceLocation DiagLoc,
-                                    QualType Type, const APValue &Value) {
+static bool
+CheckConstantExpression(EvalInfo &Info, SourceLocation DiagLoc, QualType Type,
+                        const APValue &Value,
+                        Expr::ConstExprUsage Usage = Expr::EvaluateForCodeGen) {
   if (Value.isUninit()) {
     Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized)
       << true << Type;
     return false;
   }
 
   // We allow _Atomic(T) to be initialized from anything that T can be
   // initialized from.
   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)))
+                                   Value.getArrayInitializedElt(I), Usage))
         return false;
     }
     if (!Value.hasArrayFiller())
       return true;
-    return CheckConstantExpression(Info, DiagLoc, EltTy,
-                                   Value.getArrayFiller());
+    return CheckConstantExpression(Info, DiagLoc, EltTy, Value.getArrayFiller(),
+                                   Usage);
   }
   if (Value.isUnion() && Value.getUnionField()) {
     return CheckConstantExpression(Info, DiagLoc,
                                    Value.getUnionField()->getType(),
-                                   Value.getUnionValue());
+                                   Value.getUnionValue(), Usage);
   }
   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)))
+      for (const CXXBaseSpecifier &BS : CD->bases()) {
+        if (!CheckConstantExpression(Info, DiagLoc, BS.getType(),
+                                     Value.getStructBase(BaseIndex), Usage))
           return false;
+        ++BaseIndex;
       }
     }
     for (const auto *I : RD->fields()) {
       if (I->isUnnamedBitfield())
         continue;
 
       if (!CheckConstantExpression(Info, DiagLoc, I->getType(),
-                                   Value.getStructField(I->getFieldIndex())))
+                                   Value.getStructField(I->getFieldIndex()),
+                                   Usage))
         return false;
     }
   }
 
   if (Value.isLValue()) {
     LValue LVal;
     LVal.setFrom(Info.Ctx, Value);
-    return CheckLValueConstantExpression(Info, DiagLoc, Type, LVal);
+    return CheckLValueConstantExpression(Info, DiagLoc, Type, LVal, Usage);
   }
 
   if (Value.isMemberPointer())
-    return CheckMemberPointerConstantExpression(Info, DiagLoc, Type, Value);
+    return CheckMemberPointerConstantExpression(Info, DiagLoc, Type, Value, Usage);
 
   // Everything else is fine.
   return true;
 }
 
 static const ValueDecl *GetLValueBaseDecl(const LValue &LVal) {
   return LVal.Base.dyn_cast<const ValueDecl*>();
 }

 }
 
 bool Expr::EvaluateAsLValue(EvalResult &Result, const ASTContext &Ctx) const {
   EvalInfo Info(Ctx, Result, EvalInfo::EM_ConstantFold);
 
   LValue LV;
   if (!EvaluateLValue(this, LV, Info) || Result.HasSideEffects ||
       !CheckLValueConstantExpression(Info, getExprLoc(),
-                                     Ctx.getLValueReferenceType(getType()), LV))
+                                     Ctx.getLValueReferenceType(getType()), LV,
+                                     Expr::EvaluateForCodeGen))
     return false;
 
   LV.moveInto(Result.Val);
   return true;
 }
 
+bool Expr::EvaluateAsConstantExpr(EvalResult &Result, ConstExprUsage Usage,
+                                  const ASTContext &Ctx) const {
+  EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
+  EvalInfo Info(Ctx, Result, EM);
+  if (!::Evaluate(Result.Val, Info, this))
+    return false;
+
+  return CheckConstantExpression(Info, getExprLoc(), getType(), Result.Val,
+                                 Usage);
+}
+
 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;

lib/Sema/SemaOverload.cpp

if (Result.get()->isValueDependent()) {
     Value = APValue();
     return Result;
   }
 
   // Check the expression is a constant expression.
   SmallVector<PartialDiagnosticAt, 8> Notes;
   Expr::EvalResult Eval;
   Eval.Diag = &Notes;
+  Expr::ConstExprUsage Usage = CCE == Sema::CCEK_TemplateArg
+                                   ? Expr::EvaluateForMangling
+                                   : Expr::EvaluateForCodeGen;
 
-  if ((T->isReferenceType()
-           ? !Result.get()->EvaluateAsLValue(Eval, S.Context)
-           : !Result.get()->EvaluateAsRValue(Eval, S.Context)) ||
+  if (!Result.get()->EvaluateAsConstantExpr(Eval, Usage, 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()) {

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();
+}

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;