[MS ABI] Prevent some expressions from evaluating to a constant

[• ehsan]

Summary

In the Microsoft ABI, some expressions containing references to variables
cannot be evaluated as a constant. These are expressions containing a
conditional, logical and/or, or comma operator with an operand that is a
variable name.

This is observable at the ABI level by whether the compiler would emit a
static initializer for such expressions where normally it would emit a
readonly constant data. See PR26210 for more details.

[• ehsan]

lib/AST/ExprConstant.cpp

4113

Oops, sorry, just noticed this whitespace issue, will fix when landing!

[majnemer]

Adding @rsmith as a reviewer.

[rnk]

Your code won't catch this test case:

static int x;
extern inline const bool *f() {
  static const bool p = !&x;
  return &p;
}

Getting this exactly right is going to be a challenge. =/

include/clang/Basic/DiagnosticASTKinds.td

151

We should add this test case and decide what to do with it:

static int x;
inline int **f() {
  static constexpr int *p = true ? 0 : &x;
  return &p;
}

Currently, in your patch, this diagnostic will come out. MSVC compiles this to guarded, dynamic initialization, despite the constexpr. ;_;

David thinks we should just give the user the real deal constexpr behavior, even though it's ABI incompatible.

152–153

This isn't an extension, so we should say something else. Maybe:

"in the Microsoft ABI static local variables cannot contain references to variables"

lib/AST/ExprConstant.cpp

9008

This should be limited in scope to only apply to static locals. We should be able to statically initialize globals.

[• ehsan]

In D16465#333688, @rnk wrote:

Your code won't catch this test case:

static int x;
extern inline const bool *f() {
  static const bool p = !&x;
  return &p;
}

Getting this exactly right is going to be a challenge. =/

Oh you're right. I need to spend some more time comparing our behavior with cl's, it seems...

include/clang/Basic/DiagnosticASTKinds.td

151

One solution to this would be to create a variation of evaluateValue() which activates this new behavior and only use it from CodeGenModule::EmitConstantInit(), so that the behavior of evaluateValue() in this context doesn't change. How does that sound?

By the way, I just realized that CheckPotentialExpressionContainingDeclRefExpr() eats this diagnostic because of the SpeculativeLookForDeclRefExprRAII object. Should I propagate it up from CheckPotentialExpressionContainingDeclRefExpr() to make it user visible? Right now the test case above only emits "constexpr variable 'p' must be initialized by a constant expression" without any notes.

lib/AST/ExprConstant.cpp

9008

Right, my bad!

[• ehsan]

Addressed the review comments.

[rnk]

Richard really is the constexpr owner so I'd like him to chime in if he can.

include/clang/AST/Decl.h

1092 ↗

(On Diff #47021)

Maybe this parameter would be better named IsStaticLocalInit or something.

include/clang/Basic/DiagnosticASTKinds.td

151

One solution to this would be to create a variation of evaluateValue() which activates this new behavior and only use it from CodeGenModule::EmitConstantInit(), so that the behavior of evaluateValue() in this context doesn't change. How does that sound?

Right, only having this behavior change in the awkward static local case is good.

By the way, I just realized that CheckPotentialExpressionContainingDeclRefExpr() eats this diagnostic because of the SpeculativeLookForDeclRefExprRAII object. Should I propagate it up from CheckPotentialExpressionContainingDeclRefExpr() to make it user visible? Right now the test case above only emits "constexpr variable 'p' must be initialized by a constant expression" without any notes.

Yeah, I'd like it if we could get that note out. I'm surprised we can't use the normal recursive traversal to find the DeclRefExprs...

[rsmith]

We stand no hope of being bug-for-bug compatible with MSVC's constant expression evaluator -- we should assume there will be expressions that we evaluate as a constant and they do not, and vice versa. For reference, we have the same problem between code built by Clang and by GCC, and in some cases for linking together multiple object files compiled by clang (an initializer for a static local in an inline function can be constant in one TU but not another). But those have never been enough of a problem for us to actually fix the root cause.

Why is this set of MSVC behavior the right set to be compatible with? Is there some important code for which this makes us ABI-compatible? I really don't believe the heuristics you've got here are an exact match for the MSVC semantics...

This feels like patching the symptoms of a bigger issue; is there something we can do to address the underlying problem? Perhaps we could always emit a mutable variable for a static local, even if we are able to constant-initialize it.

lib/AST/ExprConstant.cpp

9008

This also needs to apply for implicitly-instantiated static data members of class templates.

test/CodeGenCXX/static-init-msvc.cpp

7

These should all be inline functions. There's no need to apply any special behavior in any of these cases, as only one object file will contain a definition of the static local.

include/clang/Basic/DiagnosticASTKinds.td

 def note_constexpr_call_here : Note<"in call to '%0'">;
 def note_constexpr_baa_insufficient_alignment : Note<
   "%select{alignment of|offset of the aligned pointer from}0 the base pointee "
   "object (%1 %plural{1:byte|:bytes}1) is %select{less than|not a multiple of}0 the "
   "asserted %2 %plural{1:byte|:bytes}2">;
 def note_constexpr_baa_value_insufficient_alignment : Note<
   "value of the aligned pointer (%0) is not a multiple of the asserted %1 "
   "%plural{1:byte|:bytes}1">;
+def note_constexpr_microsoft_abi_declrefexpr : Note<
+  "the constant expression cannot contain a reference to a variable as a Microsoft "
+  "ABI extension">;
 
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"integer-overflow">>;
 
 // inline asm related.
 let CategoryName = "Inline Assembly Issue" in {
   def err_asm_invalid_escape : Error<

lib/AST/ExprConstant.cpp

struct EvalInfo {
     /// HasActiveDiagnostic - Was the previous diagnostic stored? If so, further
     /// notes attached to it will also be stored, otherwise they will not be.
     bool HasActiveDiagnostic;
 
     /// \brief Have we emitted a diagnostic explaining why we couldn't constant
     /// fold (not just why it's not strictly a constant expression)?
     bool HasFoldFailureDiagnostic;
 
+    /// \brief True if we need to obey the Microsoft ABI.  This affects whether
+    /// some expressions can be evaluated as a constant if they refer to
+    /// variables in some cases.  See PR26210 for the discussion.
+    bool IsMicrosoftABI;
+
+    /// \brief True if we are looking for a DeclRefExpr.
+    bool LookingForDeclRefExpr;
+
+    /// \brief True if we have observed a DeclRefExpr since the last time that
+    /// awaitDeclRefExpr() was called.  This is used in order to handle Microsoft
+    /// ABI requirements.
+    bool HaveSeenDeclRefExpr;
+
     enum EvaluationMode {
       /// Evaluate as a constant expression. Stop if we find that the expression
       /// is not a constant expression.
       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
struct EvalInfo {
 
     EvalInfo(const ASTContext &C, Expr::EvalStatus &S, EvaluationMode Mode)
       : Ctx(const_cast<ASTContext &>(C)), EvalStatus(S), CurrentCall(nullptr),
         CallStackDepth(0), NextCallIndex(1),
         StepsLeft(getLangOpts().ConstexprStepLimit),
         BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
         EvaluatingDecl((const ValueDecl *)nullptr),
         EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
-        HasFoldFailureDiagnostic(false), EvalMode(Mode) {}
+        HasFoldFailureDiagnostic(false), IsMicrosoftABI(false),
+        LookingForDeclRefExpr(false), HaveSeenDeclRefExpr(false),
+        EvalMode(Mode) {}
 
     void setEvaluatingDecl(APValue::LValueBase Base, APValue &Value) {
       EvaluatingDecl = Base;
       EvaluatingDeclValue = &Value;
     }
 
     const LangOptions &getLangOpts() const { return Ctx.getLangOpts(); }
 
bool keepEvaluatingAfterFailure() {
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
 
     bool allowInvalidBaseExpr() const {
       return EvalMode == EM_DesignatorFold;
     }
+
+    /// Use the Microsoft ABI during evaluation.
+    void useMicrosoftABI() {
+      IsMicrosoftABI = true;
+    }
+
+    /// Start watching for a DeclRefExpr.
+    void awaitDeclRefExpr() {
+      LookingForDeclRefExpr = true;
+      HaveSeenDeclRefExpr = false;
+    }
+
+    /// Remember that we have observed a DeclRefExpr.
+    void noteDeclRefExpr() {
+      HaveSeenDeclRefExpr = true;
+    }
   };
 
   /// Object used to treat all foldable expressions as constant expressions.
   struct FoldConstant {
     EvalInfo &Info;
     bool Enabled;
     bool HadNoPriorDiags;
     EvalInfo::EvaluationMode OldMode;
struct FoldOffsetRAII {
     }
 
     ~FoldOffsetRAII() { Info.EvalMode = OldMode; }
   };
 
   /// RAII object used to suppress diagnostics and side-effects from a
   /// speculative evaluation.
   class SpeculativeEvaluationRAII {
-    EvalInfo &Info;
     Expr::EvalStatus Old;
+  protected:
+    EvalInfo &Info;
 
   public:
     SpeculativeEvaluationRAII(EvalInfo &Info,
                         SmallVectorImpl<PartialDiagnosticAt> *NewDiag = nullptr)
-      : Info(Info), Old(Info.EvalStatus) {
+      : Old(Info.EvalStatus), Info(Info) {
       Info.EvalStatus.Diag = NewDiag;
       // If we're speculatively evaluating, we may have skipped over some
       // evaluations and missed out a side effect.
       Info.EvalStatus.HasSideEffects = true;
     }
     ~SpeculativeEvaluationRAII() {
       Info.EvalStatus = Old;
     }
   };
 
+  class SpeculativeLookForDeclRefExprRAII final : SpeculativeEvaluationRAII {
+  public:
+    SpeculativeLookForDeclRefExprRAII(EvalInfo &Info,
+                           SmallVectorImpl<PartialDiagnosticAt> *NewDiag = nullptr)
+      : SpeculativeEvaluationRAII(Info, NewDiag) {
+      Info.awaitDeclRefExpr();
+    }
+    ~SpeculativeLookForDeclRefExprRAII() {
+      Info.LookingForDeclRefExpr = false;
+    }
+  };
+
   /// RAII object wrapping a full-expression or block scope, and handling
   /// the ending of the lifetime of temporaries created within it.
   template<bool IsFullExpression>
   class ScopeRAII {
     EvalInfo &Info;
     unsigned OldStackSize;
   public:
     ScopeRAII(EvalInfo &Info)
// Speculatively evaluate both arms.
       StmtVisitorTy::Visit(E->getTrueExpr());
       if (Diag.empty())
         return;
     }
 
     Error(E, diag::note_constexpr_conditional_never_const);
   }
 
+  // Check whether an expression contains a DeclRefExpr where not permitted by
+  // the Microsoft ABI.
+  template<typename Expr>
+  bool CheckPotentialExpressionContainingDeclRefExpr(const Expr *E) {
+    SmallVector<PartialDiagnosticAt, 8> Diag;
+    SpeculativeLookForDeclRefExprRAII Speculate(Info, &Diag);
+
+    StmtVisitorTy::Visit(E);
+    if (Info.HaveSeenDeclRefExpr)
+      return Error(E, diag::note_constexpr_microsoft_abi_declrefexpr);
+    return true;
+  }
 
   template<typename ConditionalOperator>
   bool HandleConditionalOperator(const ConditionalOperator *E) {
     bool BoolResult;
     if (!EvaluateAsBooleanCondition(E->getCond(), BoolResult, Info)) {
       if (Info.checkingPotentialConstantExpression())
         CheckPotentialConstantConditional(E);
       return false;
     }
 
     Expr *EvalExpr = BoolResult ? E->getTrueExpr() : E->getFalseExpr();
+    if (Info.IsMicrosoftABI) {
+      // In the Microsoft ABI, a DeclRefExpr on either side of the conditional
+      // operator will prevent us from evaluating the expression to a constant.
+      if (!CheckPotentialExpressionContainingDeclRefExpr(E->getTrueExpr(),
+                                                          E->getFalseExpr()))
+        return false;
+    }
     return StmtVisitorTy::Visit(EvalExpr);
   }
 
 protected:
   EvalInfo &Info;
   typedef ConstStmtVisitor<Derived, bool> StmtVisitorTy;
   typedef ExprEvaluatorBase ExprEvaluatorBaseTy;
 
   OptionalDiagnostic CCEDiag(const Expr *E, diag::kind D) {
     return Info.CCEDiag(E, D);
   }
 
   bool ZeroInitialization(const Expr *E) { return Error(E); }
 
+  // Check whether either of the two expressions contains a DeclRefExpr where
+  // not permitted by the Microsoft ABI.
+  template<typename ExprLHS, typename ExprRHS>
+  bool CheckPotentialExpressionContainingDeclRefExpr(const ExprLHS *ELHS,
+                                                     const ExprRHS *ERHS) {
+    return CheckPotentialExpressionContainingDeclRefExpr(ELHS) &&
+           CheckPotentialExpressionContainingDeclRefExpr(ERHS);
+  }
+
 public:
   ExprEvaluatorBase(EvalInfo &Info) : Info(Info) {}
 
   EvalInfo &getEvalInfo() { return Info; }
 
   /// Report an evaluation error. This should only be called when an error is
   /// first discovered. When propagating an error, just return false.
   bool Error(const Expr *E, diag::kind D) {

 ///  * @selector() expressions in Objective-C
 static bool EvaluateLValue(const Expr *E, LValue &Result, EvalInfo &Info) {
   assert(E->isGLValue() || E->getType()->isFunctionType() ||
          E->getType()->isVoidType() || isa<ObjCSelectorExpr>(E));
   return LValueExprEvaluator(Info, Result).Visit(E);
 }
 
 bool LValueExprEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
+  Info.noteDeclRefExpr();
+
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(E->getDecl()))
     return Success(FD);
   if (const VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
     return VisitVarDecl(E, VD);
   return Error(E);
 }
 
 bool LValueExprEvaluator::VisitVarDecl(const Expr *E, const VarDecl *VD) {
bool VisitIntegerLiteral(const IntegerLiteral *E) {
     return Success(E->getValue(), E);
   }
   bool VisitCharacterLiteral(const CharacterLiteral *E) {
     return Success(E->getValue(), E);
   }
 
   bool CheckReferencedDecl(const Expr *E, const Decl *D);
   bool VisitDeclRefExpr(const DeclRefExpr *E) {
+    Info.noteDeclRefExpr();
+
     if (CheckReferencedDecl(E, E->getDecl()))
       return true;
 
     return ExprEvaluatorBaseTy::VisitDeclRefExpr(E);
   }
   bool VisitMemberExpr(const MemberExpr *E) {
     if (CheckReferencedDecl(E, E->getMemberDecl())) {
       VisitIgnoredValue(E->getBase());
case Job::BinOpVisitedLHSKind: {
       return;
     }
   }
   
   llvm_unreachable("Invalid Job::Kind!");
 }
 
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
+    if (Info.IsMicrosoftABI) {
+      switch (E->getOpcode()) {
+      case BO_Comma:
+      case BO_LAnd:
+      case BO_LOr:
+        // In the Microsoft ABI, a DeclRefExpr on either side of some binary
+        // operators will prevent us from evaluating the expression to a constant.
+        if (!CheckPotentialExpressionContainingDeclRefExpr(E->getLHS(), E->getRHS()))
+          return false;
+      default:
+        /* No need to do anything. */ ;
+      }
+    }
+
   if (!Info.keepEvaluatingAfterFailure() && E->isAssignmentOp())
     return Error(E);
 
   if (DataRecursiveIntBinOpEvaluator::shouldEnqueue(E))
     return DataRecursiveIntBinOpEvaluator(*this, Result).Traverse(E);
 
   QualType LHSTy = E->getLHS()->getType();
   QualType RHSTy = E->getRHS()->getType();
bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
   Expr::EvalStatus EStatus;
   EStatus.Diag = &Notes;
 
   EvalInfo InitInfo(Ctx, EStatus, VD->isConstexpr()
                                       ? EvalInfo::EM_ConstantExpression
                                       : EvalInfo::EM_ConstantFold);
   InitInfo.setEvaluatingDecl(VD, Value);
 
+  if (Ctx.getTargetInfo().getCXXABI().isMicrosoft())
+    InitInfo.useMicrosoftABI();
+
   LValue LVal;
   LVal.set(VD);
 
   // C++11 [basic.start.init]p2:
   //  Variables with static storage duration or thread storage duration shall be
   //  zero-initialized before any other initialization takes place.
   // This behavior is not present in C.
   if (Ctx.getLangOpts().CPlusPlus && !VD->hasLocalStorage() &&

test/CodeGenCXX/static-init-msvc.cpp

+// RUN: %clang_cc1 -triple i686-windows-msvc -fms-extensions -emit-llvm -O0 -w -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -emit-llvm -O0 -w -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple i686-windows-gnu -fms-extensions -emit-llvm -O0 -w -o - %s | FileCheck %s --check-prefix GNU
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -fms-extensions -emit-llvm -O0 -w -o - %s | FileCheck %s --check-prefix GNU
+
+void fun_and() {
+// CHECK-LABEL: @"\01?fun_and@@YAXXZ"()
+// GNU-LABEL: @_Z7fun_andv()
+  static int k;
+  static const int foo = 0 && k;
+// CHECK: init:
+// CHECK-NEXT: store i32 0, i32* @"\01?foo@?1??fun_and@@YAXXZ@4HB", align 4
+// GNU: entry:
+// GNU-NEXT: ret void
+}
+
+void fun_or() {
+// CHECK-LABEL: @"\01?fun_or@@YAXXZ"()
+// GNU-LABEL: @_Z6fun_orv()
+  static int k;
+  static const int foo = 1 || k;
+// CHECK: init:
+// CHECK-NEXT: store i32 1, i32* @"\01?foo@?1??fun_or@@YAXXZ@4HB", align 4
+// GNU: entry:
+// GNU-NEXT: ret void
+}
+
+void fun_comma() {
+// CHECK-LABEL: @"\01?fun_comma@@YAXXZ"()
+// GNU-LABEL: @_Z9fun_commav()
+  static int k;
+  static const int foo = (k, 0);
+// CHECK: init:
+// CHECK-NEXT: store i32 0, i32* @"\01?foo@?1??fun_comma@@YAXXZ@4HB", align 4
+// GNU: entry:
+// GNU-NEXT: ret void
+}
+
+void fun_cond() {
+// CHECK-LABEL: @"\01?fun_cond@@YAXXZ"()
+// GNU-LABEL: @_Z8fun_condv()
+  static int k;
+  static const int foo = true ? 0 : k;
+// CHECK: init:
+// CHECK-NEXT: store i32 0, i32* @"\01?foo@?1??fun_cond@@YAXXZ@4HB", align 4
+// GNU: entry:
+// GNU-NEXT: ret void
+}