[cfe-commits] [PATCH] Model C++11 rules for odr-use for variables correctly

Eli Friedman eli.friedman at gmail.com
Fri Jan 27 19:56:35 PST 2012


On Fri, Jan 27, 2012 at 3:26 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Fri, Jan 27, 2012 at 3:12 PM, John McCall <rjmccall at apple.com> wrote:
>> On Jan 27, 2012, at 2:53 PM, Eli Friedman wrote:
>>> On Thu, Jan 26, 2012 at 7:29 PM, John McCall <rjmccall at apple.com> wrote:
>>>> On Jan 24, 2012, at 9:34 PM, Eli Friedman wrote:
>>>>> Patch attached.  Basically, this is implementing the C++11 rules in
>>>>> [basic.def.odr]p2 for variables allowed in constant expressions.
>>>>> (Those rules don't matter so much for most code given how we do code
>>>>> generation, but we have to get the rules exactly right to get implicit
>>>>> capture in lambda expressions working correctly.)
>>>>>
>>>>> I don't really like this patch, but it seems like it is necessary.
>>>>
>>>> Yeah, I'm really not happy with this.  Obvious alternative:  mark ODR use
>>>> at the DRE creation site unless the object is not already marked and it
>>>> meets the constant-loading criteria.  If it does, flag that this full-expression
>>>> contains a potential reference, then walk back over the completed
>>>> full-expression looking for PE DREs not direct operands of L2R casts.
>>>
>>> Is there any existing hook that actually triggers where I need it to?
>>> MaybeCreateExprWithCleanups seems close... but doesn't quite cover
>>> everything.
>>
>> What's missing?
>
> In essence, things which are constant-expressions in the C++ grammar
> are missing.  Not too hard to add them, I guess.

New version attached; it doesn't pass regression tests because I still
need to add a hook to catch VLA array bounds attached to declarations,
but is this the direction you were thinking of?

-Eli
-------------- next part --------------
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h	(revision 149031)
+++ include/clang/Sema/Sema.h	(working copy)
@@ -2267,6 +2267,10 @@
   void MarkDeclarationsReferencedInType(SourceLocation Loc, QualType T);
   void MarkDeclarationsReferencedInExpr(Expr *E);
 
+  void MarkVarODRUsed(VarDecl *Var, SourceLocation Loc);
+  void FindUsesAsLValue(Stmt *S);
+  void MarkUseAsRValue(Expr *E);
+
   /// \brief Try to recover by turning the given expression into a
   /// call.  Returns true if recovery was attempted or an error was
   /// emitted; this may also leave the ExprResult invalid.
Index: lib/Sema/TreeTransform.h
===================================================================
--- lib/Sema/TreeTransform.h	(revision 149031)
+++ lib/Sema/TreeTransform.h	(working copy)
@@ -5313,6 +5313,7 @@
   ExprResult Target = getDerived().TransformExpr(S->getTarget());
   if (Target.isInvalid())
     return StmtError();
+  Target = SemaRef.MaybeCreateExprWithCleanups(Target.take());
 
   if (!getDerived().AlwaysRebuild() &&
       Target.get() == S->getTarget())
@@ -5725,10 +5726,14 @@
   ExprResult Cond = getDerived().TransformExpr(S->getCond());
   if (Cond.isInvalid())
     return StmtError();
+  if (Cond.get())
+    Cond = SemaRef.MaybeCreateExprWithCleanups(Cond.take());
 
   ExprResult Inc = getDerived().TransformExpr(S->getInc());
   if (Inc.isInvalid())
     return StmtError();
+  if (Inc.get())
+    Inc = SemaRef.MaybeCreateExprWithCleanups(Inc.take());
 
   StmtResult LoopVar = getDerived().TransformStmt(S->getLoopVarStmt());
   if (LoopVar.isInvalid())
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp	(revision 149003)
+++ lib/Sema/SemaExprCXX.cpp	(working copy)
@@ -2051,6 +2051,7 @@
     Condition = CheckBooleanCondition(Condition.take(), StmtLoc);
     if (Condition.isInvalid())
       return ExprError();
+    Condition = MaybeCreateExprWithCleanups(Condition);
   }
 
   return move(Condition);
@@ -4213,6 +4214,8 @@
 Expr *Sema::MaybeCreateExprWithCleanups(Expr *SubExpr) {
   assert(SubExpr && "sub expression can't be null!");
 
+  FindUsesAsLValue(SubExpr);
+
   unsigned FirstCleanup = ExprEvalContexts.back().NumCleanupObjects;
   assert(ExprCleanupObjects.size() >= FirstCleanup);
   assert(ExprNeedsCleanups || ExprCleanupObjects.size() == FirstCleanup);
@@ -4232,6 +4235,8 @@
 Stmt *Sema::MaybeCreateStmtWithCleanups(Stmt *SubStmt) {
   assert(SubStmt && "sub statement can't be null!");
 
+  FindUsesAsLValue(SubStmt);
+
   if (!ExprNeedsCleanups)
     return SubStmt;
 
Index: lib/Sema/SemaTemplate.cpp
===================================================================
--- lib/Sema/SemaTemplate.cpp	(revision 148998)
+++ lib/Sema/SemaTemplate.cpp	(working copy)
@@ -3613,6 +3613,8 @@
   // Create the template argument.
   Converted = TemplateArgument(Entity->getCanonicalDecl());
   S.MarkDeclarationReferenced(Arg->getLocStart(), Entity);
+  if (VarDecl *Var = dyn_cast<VarDecl>(Entity))
+    S.MarkVarODRUsed(Var, Arg->getLocStart());
   return false;
 }
 
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp	(revision 149031)
+++ lib/Sema/SemaStmt.cpp	(working copy)
@@ -513,14 +513,14 @@
   if (CondResult.isInvalid()) return StmtError();
   Cond = CondResult.take();
 
-  if (!CondVar) {
+  if (!CondVar)
     CheckImplicitConversions(Cond, SwitchLoc);
-    CondResult = MaybeCreateExprWithCleanups(Cond);
-    if (CondResult.isInvalid())
-      return StmtError();
-    Cond = CondResult.take();
-  }
 
+  CondResult = MaybeCreateExprWithCleanups(Cond);
+  if (CondResult.isInvalid())
+    return StmtError();
+  Cond = CondResult.take();
+
   getCurFunction()->setHasBranchIntoScope();
 
   SwitchStmt *SS = new (Context) SwitchStmt(Context, ConditionVar, Cond);
@@ -1609,6 +1609,7 @@
     E = ExprRes.take();
     if (DiagnoseAssignmentResult(ConvTy, StarLoc, DestTy, ETy, E, AA_Passing))
       return StmtError();
+    E = MaybeCreateExprWithCleanups(E);
   }
 
   getCurFunction()->setHasIndirectGoto();
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp	(revision 149031)
+++ lib/Sema/SemaExpr.cpp	(working copy)
@@ -9459,6 +9459,30 @@
   return TranformToPotentiallyEvaluated(E);
 }
 
+static bool DelayVarODRUsedForType(QualType T) {
+  // C++11 [basic.def.odr]p1: A variable or non-overloaded function whose name
+  // appears as a potentially-evaluated expression is odr-used unless it is
+  // an object that satisfies the requirements for appearing in a
+  // constant expression (5.19) and the lvalue-to-rvalue conversion (4.1)
+  // is immediately applied.
+
+  // We don't try to check the constant expression bit here because it gives
+  // our assertions better coverage.
+
+  // Check for an "object".
+  if (!T->isObjectType())
+    return false;
+  // The lvalue-to-rvalue conversion can't be applied to arrays.
+  if (T->isArrayType())
+    return false;
+  // lvalue-to-rvalue conversion doesn't apply to classes.
+  // FIXME: That isn't completely true... but I doubt anyone cares about the
+  // one edge case where we actually do apply lvalue-to-rvalue conversion.
+  if (T->isRecordType())
+    return false;
+  return true;
+}
+
 /// \brief Note that the given declaration was referenced in the source code.
 ///
 /// This routine should be invoke whenever a given declaration is referenced
@@ -9619,24 +9643,99 @@
       }
     }
 
-    // Keep track of used but undefined variables.  We make a hole in
-    // the warning for static const data members with in-line
-    // initializers.
-    // FIXME: The hole we make for static const data members is too wide!
-    // We need to implement the C++11 rules for odr-used.
-    if (Var->hasDefinition() == VarDecl::DeclarationOnly
-        && Var->getLinkage() != ExternalLinkage
-        && !(Var->isStaticDataMember() && Var->hasInit())) {
-      SourceLocation &old = UndefinedInternals[Var->getCanonicalDecl()];
-      if (old.isInvalid()) old = Loc;
-    }
+    // We delay actually treating the VarDecl as used in cases where it might
+    // not actually be odr-used.
+    if (!DelayVarODRUsedForType(Var->getType()))
+      MarkVarODRUsed(Var, Loc);
 
-    D->setUsed(true);
     return;
   }
 }
 
+void Sema::MarkVarODRUsed(VarDecl *Var, SourceLocation Loc) {
+  if (CurContext->isDependentContext())
+    return;
+
+  if (Var->isUsed(false))
+    return;
+
+  if (Var->hasDefinition() == VarDecl::DeclarationOnly &&
+      Var->getLinkage() != ExternalLinkage) {
+    SourceLocation &old = UndefinedInternals[Var->getCanonicalDecl()];
+    if (old.isInvalid()) old = Loc;
+  }
+
+  Var->setUsed(true);
+}
+
 namespace {
+  /// \brief Helper class that marks all of the declarations referenced by
+  /// potentially-evaluated subexpressions as "referenced".
+  class MarkVarODRUses : public RecursiveASTVisitor<MarkVarODRUses> {
+    Sema &S;
+
+  public:
+    typedef RecursiveASTVisitor<MarkVarODRUses> Inherited;
+    
+    explicit MarkVarODRUses(Sema &S) : S(S) { }
+    
+    bool VisitDeclRefExpr(DeclRefExpr *E) {
+      VarDecl *Var = dyn_cast<VarDecl>(E->getDecl());
+      if (Var && DelayVarODRUsedForType(Var->getType()))
+        S.MarkVarODRUsed(Var, E->getLocation());
+      return true;
+    }
+
+    bool TraverseImplicitCastExpr(ImplicitCastExpr *E) {
+      if (E->getCastKind() == CK_LValueToRValue) {
+        Expr *SubExpr = E->getSubExpr()->IgnoreParens();
+        if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(SubExpr)) {
+          VarDecl *Var = dyn_cast<VarDecl>(DRE->getDecl());
+          if (Var && DelayVarODRUsedForType(Var->getType()) &&
+              !DRE->isEvaluatable(S.Context))
+            S.MarkVarODRUsed(Var, DRE->getLocation());
+          return true;
+        }
+      }
+
+      return Inherited::TraverseImplicitCastExpr(E);
+    }
+
+    bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
+      if (E->getKind() != UETT_SizeOf ||
+          !E->getTypeOfArgument()->isVariableArrayType())
+        return true;
+      return Inherited::TraverseUnaryExprOrTypeTraitExpr(E);
+    }
+    bool TraverseExpressionTraitExpr(ExpressionTraitExpr *E) { return true; }
+    bool TraverseCXXUuidofExpr(CXXUuidofExpr *E) { return true; }  
+    bool TraverseCXXNoexceptExpr(CXXNoexceptExpr *E) { return true; }
+    bool TraverseChooseExpr(ChooseExpr *E) {
+      // Only the selected subexpression matters; the other one is not evaluated.
+      return TraverseStmt(E->getChosenSubExpr(S.Context));
+    }
+    bool TraverseCXXTypeidExpr(CXXTypeidExpr *E) {
+      // typeid(expression) is potentially evaluated when the argument is
+      // a glvalue of polymorphic type. (C++ 5.2.8p2-3)
+      if (!E->isTypeOperand() && E->Classify(S.Context).isGLValue())
+        if (const RecordType *Record 
+                   = E->getExprOperand()->getType()->getAs<RecordType>())
+          if (cast<CXXRecordDecl>(Record->getDecl())->isPolymorphic())
+            return Inherited::TraverseCXXTypeidExpr(E);
+      return true;
+    }
+    bool TraverseBlockExpr(BlockExpr *E) { return true; }
+  };
+}
+
+void Sema::FindUsesAsLValue(Stmt *S) {
+  if (CurContext->isDependentContext())
+    return;
+
+  MarkVarODRUses(*this).TraverseStmt(S);
+}
+
+namespace {
   // Mark all of the declarations referenced
   // FIXME: Not fully implemented yet! We need to have a better understanding
   // of when we're entering


More information about the cfe-commits mailing list