[cfe-commits] [PATCH] Model C++11 rules for odr-use for variables correctly
Eli Friedman
eli.friedman at gmail.com
Wed Feb 1 20:15:55 PST 2012
On Wed, Feb 1, 2012 at 11:07 AM, John McCall <rjmccall at apple.com> wrote:
> On Jan 31, 2012, at 9:43 PM, Eli Friedman wrote:
>> New version attached; similar approach, but avoiding the recursive
>> walk by keeping the relevant expressions in a side-table. This is
>> essentially complete except for one issue: I think I need some sort of
>> stack to prevent incorrect odr-use marking in some edge cases.
>
> Clever idea! I think this works.
>
> I believe the only construct that allows nested full-expressions is a
> statement-expression. You should optimize for the case that doesn't
> involve statement-expressions, e.g. by just moving the current set
> aside whenever we enter one. I think we might even have a RAII
> object for doing things like this already.
>
> +void Sema::UpdateMarkingForLValueToRValue(Expr *E) {
> + MaybeODRUseExprs.erase(E->IgnoreParens());
> +}
>
> Hmm. It might be useful to stash in the expression that it's being used
> this way. That might not play well with expression re-use, though.
> No changes suggested. :)
>
> (leaving that comment in place for the evidence trail)
>
> /// \brief Build a MemberExpr AST node.
> -static MemberExpr *BuildMemberExpr(ASTContext &C, Expr *Base, bool isArrow,
> +static MemberExpr *BuildMemberExpr(Sema &SemaRef,
> + ASTContext &C, Expr *Base, bool isArrow,
>
> If you're passing in Sema, you don't also need to pass in the ASTContext.
>
> +bool IsPotentiallyEvaluatedContext(Sema &SemaRef) {
> ...
> +void DoMarkVarDeclReferenced(Sema &SemaRef, SourceLocation Loc,
> + VarDecl *Var, Expr *E) {
>
> These are missing 'static'. Also, the convention for functions is to start with
> a lowercase letter; we haven't been doing mass-changes, but if you're
> touching every call site anyway, please do follow the convention here.
>
> -/// \brief Note that the given declaration was referenced in the source code.
> -///
> -/// This routine should be invoke whenever a given declaration is referenced
> -/// in the source code, and where that reference occurred. If this declaration
> -/// reference means that the the declaration is used (C++ [basic.def.odr]p2,
> -/// C99 6.9p3), then the declaration will be marked as used.
> -///
> -/// \param Loc the location where the declaration was referenced.
> -///
> -/// \param D the declaration that has been referenced by the source code.
>
> Killing this comment is fine, but please add comments to the new functions,
> and make sure one of them conveys this whole scheme appropriately and
> why it matters. Make sure that the ones that don't take Exprs clearly spell
> out the semantic difference.
>
> Also, please split the restructure of MarkDeclarationReferenced into
> declaration-specific helpers into a separate "no functionality change" patch;
> otherwise it looks like you're significantly revising the logic there, when in
> fact you're mostly just re-indenting it.
r149586 is the separate "no functionality change" patch. Attaching
the remaining bits.
-Eli
-------------- next part --------------
Index: test/SemaCXX/undefined-internal.cpp
===================================================================
--- test/SemaCXX/undefined-internal.cpp (revision 149586)
+++ test/SemaCXX/undefined-internal.cpp (working copy)
@@ -122,3 +122,48 @@
f(Uncopyable()); // expected-warning {{C++98 requires an accessible copy constructor}}
};
}
+
+
+namespace std { class type_info; };
+namespace cxx11_odr_rules {
+ // Note: the way this test is written isn't really ideal, but there really
+ // isn't any other way to check that the odr-used logic for constants
+ // is working without working implicit capture in lambda-expressions.
+ // (The more accurate used-but-not-defined warning is the only other visible
+ // effect of accurate odr-used computation.)
+ //
+ // Note that the warning in question can trigger in cases some people would
+ // consider false positives; hopefully that happens rarely in practice.
+
+ namespace {
+ struct A {
+ static const int unused = 10;
+ static const int used1 = 20; // expected-warning {{internal linkage}}
+ static const int used2 = 20; // expected-warning {{internal linkage}}
+ virtual ~A() {}
+ };
+ }
+
+ void a(int,int);
+ A& p(const int&) { static A a; return a; }
+
+ // Check handling of default arguments
+ void b(int = A::unused);
+
+ void tests() {
+ // Basic test
+ a(A::unused, A::unused);
+
+ // Check that nesting an unevaluated or constant-evaluated context does
+ // the right thing.
+ a(A::unused, sizeof(int[10]));
+
+ // Check that the checks work with unevaluated contexts
+ (void)sizeof(p(A::used1));
+ (void)typeid(p(A::used1)); // expected-note {{used here}}
+
+ // Misc other testing
+ a(A::unused, 1 ? A::used2 : A::used2); // expected-note {{used here}}
+ b();
+ }
+}
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h (revision 149586)
+++ include/clang/Sema/Sema.h (working copy)
@@ -233,6 +233,8 @@
/// element type here is ExprWithCleanups::Object.
SmallVector<BlockDecl*, 8> ExprCleanupObjects;
+ llvm::SmallPtrSet<Expr*, 8> MaybeODRUseExprs;
+
/// \brief Stack containing information about each of the nested
/// function, block, and method scopes that are currently active.
///
@@ -556,6 +558,8 @@
/// this expression evaluation context.
unsigned NumCleanupObjects;
+ llvm::SmallPtrSet<Expr*, 8> SavedMaybeODRUseExprs;
+
ExpressionEvaluationContextRecord(ExpressionEvaluationContext Context,
unsigned NumCleanupObjects,
bool ParentNeedsCleanups)
@@ -2278,6 +2282,10 @@
void MarkDeclRefReferenced(DeclRefExpr *E);
void MarkMemberReferenced(MemberExpr *E);
+ void UpdateMarkingForLValueToRValue(Expr *E);
+ void CleanupVarDeclMarking();
+
+
void MarkDeclarationsReferencedInType(SourceLocation Loc, QualType T);
void MarkDeclarationsReferencedInExpr(Expr *E);
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp (revision 149586)
+++ lib/Sema/SemaExprCXX.cpp (working copy)
@@ -4232,6 +4232,8 @@
Expr *Sema::MaybeCreateExprWithCleanups(Expr *SubExpr) {
assert(SubExpr && "sub expression can't be null!");
+ CleanupVarDeclMarking();
+
unsigned FirstCleanup = ExprEvalContexts.back().NumCleanupObjects;
assert(ExprCleanupObjects.size() >= FirstCleanup);
assert(ExprNeedsCleanups || ExprCleanupObjects.size() == FirstCleanup);
@@ -4251,6 +4253,8 @@
Stmt *Sema::MaybeCreateStmtWithCleanups(Stmt *SubStmt) {
assert(SubStmt && "sub statement can't be null!");
+ CleanupVarDeclMarking();
+
if (!ExprNeedsCleanups)
return SubStmt;
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp (revision 149586)
+++ lib/Sema/SemaDecl.cpp (working copy)
@@ -7299,6 +7299,8 @@
assert(ExprCleanupObjects.empty() && "Leftover temporaries in function");
assert(!ExprNeedsCleanups && "Unaccounted cleanups in function");
+ assert(MaybeODRUseExprs.empty() &&
+ "Leftover expressions for odr-use checking");
}
if (!IsInstantiation)
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 149586)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -404,6 +404,8 @@
if (T.hasQualifiers())
T = T.getUnqualifiedType();
+ UpdateMarkingForLValueToRValue(E);
+
ExprResult Res = Owned(ImplicitCastExpr::Create(Context, T, CK_LValueToRValue,
E, 0, VK_RValue));
@@ -9433,6 +9435,8 @@
ExprCleanupObjects.size(),
ExprNeedsCleanups));
ExprNeedsCleanups = false;
+ if (!MaybeODRUseExprs.empty())
+ std::swap(MaybeODRUseExprs, ExprEvalContexts.back().SavedMaybeODRUseExprs);
}
void Sema::PopExpressionEvaluationContext() {
@@ -9446,10 +9450,14 @@
ExprCleanupObjects.erase(ExprCleanupObjects.begin() + Rec.NumCleanupObjects,
ExprCleanupObjects.end());
ExprNeedsCleanups = Rec.ParentNeedsCleanups;
+ CleanupVarDeclMarking();
+ std::swap(MaybeODRUseExprs, Rec.SavedMaybeODRUseExprs);
// Otherwise, merge the contexts together.
} else {
ExprNeedsCleanups |= Rec.ParentNeedsCleanups;
+ MaybeODRUseExprs.insert(Rec.SavedMaybeODRUseExprs.begin(),
+ Rec.SavedMaybeODRUseExprs.end());
}
// Pop the current expression evaluation context off the stack.
@@ -9461,6 +9469,7 @@
ExprCleanupObjects.begin() + ExprEvalContexts.back().NumCleanupObjects,
ExprCleanupObjects.end());
ExprNeedsCleanups = false;
+ MaybeODRUseExprs.clear();
}
ExprResult Sema::HandleExprEvaluationContextForTypeof(Expr *E) {
@@ -9603,18 +9612,8 @@
Func->setUsed(true);
}
-/// \brief Mark a variable referenced, and check whether it is odr-used
-/// (C++ [basic.def.odr]p2, C99 6.9p3). Note that this should not be
-/// used directly for normal expressions referring to VarDecl.
-void Sema::MarkVariableReferenced(SourceLocation Loc, VarDecl *Var) {
- Var->setReferenced();
-
- if (Var->isUsed(false))
- return;
-
- if (!IsPotentiallyEvaluatedContext(*this))
- return;
-
+static void InstantiateVarDecl(Sema &SemaRef, VarDecl *Var,
+ SourceLocation Loc) {
// Implicit instantiation of static data members of class templates.
if (Var->isStaticDataMember() &&
Var->getInstantiatedFromStaticDataMember()) {
@@ -9624,35 +9623,108 @@
MSInfo->getTemplateSpecializationKind()== TSK_ImplicitInstantiation) {
MSInfo->setPointOfInstantiation(Loc);
// This is a modification of an existing AST node. Notify listeners.
- if (ASTMutationListener *L = getASTMutationListener())
+ if (ASTMutationListener *L = SemaRef.getASTMutationListener())
L->StaticDataMemberInstantiated(Var);
if (Var->isUsableInConstantExpressions())
// Do not defer instantiations of variables which could be used in a
// constant expression.
- InstantiateStaticDataMemberDefinition(Loc, Var);
+ SemaRef.InstantiateStaticDataMemberDefinition(Loc, Var);
else
- PendingInstantiations.push_back(std::make_pair(Var, Loc));
+ SemaRef.PendingInstantiations.push_back(std::make_pair(Var, Loc));
}
}
+}
- // 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()];
+static void MarkVarDeclODRUsed(Sema &SemaRef, VarDecl *Var,
+ SourceLocation Loc) {
+ // Keep track of used but undefined variables.
+ if (Var->hasDefinition() == VarDecl::DeclarationOnly &&
+ Var->getLinkage() != ExternalLinkage) {
+ SourceLocation &old = SemaRef.UndefinedInternals[Var->getCanonicalDecl()];
if (old.isInvalid()) old = Loc;
}
Var->setUsed(true);
}
+void Sema::UpdateMarkingForLValueToRValue(Expr *E) {
+ // Per C++11 [basic.def.odr], a variable 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." This function handles the lvalue-to-rvalue
+ // conversion part.
+ MaybeODRUseExprs.erase(E->IgnoreParens());
+}
+
+void Sema::CleanupVarDeclMarking() {
+ for (llvm::SmallPtrSetIterator<Expr*> i = MaybeODRUseExprs.begin(),
+ e = MaybeODRUseExprs.end();
+ i != e; ++i) {
+ VarDecl *Var;
+ SourceLocation Loc;
+ if (BlockDeclRefExpr *BDRE = dyn_cast<BlockDeclRefExpr>(*i)) {
+ Var = BDRE->getDecl();
+ Loc = BDRE->getLocation();
+ } else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(*i)) {
+ Var = cast<VarDecl>(DRE->getDecl());
+ Loc = DRE->getLocation();
+ } else if (MemberExpr *ME = dyn_cast<MemberExpr>(*i)) {
+ Var = cast<VarDecl>(ME->getMemberDecl());
+ Loc = ME->getMemberLoc();
+ } else {
+ llvm_unreachable("Unexpcted expression");
+ }
+
+ MarkVarDeclODRUsed(*this, Var, Loc);
+ }
+
+ MaybeODRUseExprs.clear();
+}
+
+// Mark a VarDecl referenced, and perform the necessary handling to compute
+// odr-uses.
+static void DoMarkVarDeclReferenced(Sema &SemaRef, SourceLocation Loc,
+ VarDecl *Var, Expr *E) {
+ Var->setReferenced();
+
+ if (Var->isUsed(false))
+ return;
+
+ if (!IsPotentiallyEvaluatedContext(SemaRef))
+ return;
+
+ InstantiateVarDecl(SemaRef, Var, Loc);
+
+ // Per C++11 [basic.def.odr], a variable 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 check the first part here, and
+ // Sema::UpdateMarkingForLValueToRValue deals with the second part.
+ // Note that we use the C++11 definition everywhere because nothing in
+ // C++03 depends on whether we get the C++03 version correct.
+ const VarDecl *DefVD;
+ if (E && !isa<ParmVarDecl>(Var) &&
+ Var->isUsableInConstantExpressions() &&
+ Var->getAnyInitializer(DefVD) && DefVD->checkInitIsICE())
+ SemaRef.MaybeODRUseExprs.insert(E);
+ else
+ MarkVarDeclODRUsed(SemaRef, Var, Loc);
+}
+
+/// \brief Mark a variable referenced, and check whether it is odr-used
+/// (C++ [basic.def.odr]p2, C99 6.9p3). Note that this should not be
+/// used directly for normal expressions referring to VarDecl.
+void Sema::MarkVariableReferenced(SourceLocation Loc, VarDecl *Var) {
+ DoMarkVarDeclReferenced(*this, Loc, Var, 0);
+}
+
static void MarkExprReferenced(Sema &SemaRef, SourceLocation Loc,
Decl *D, Expr *E) {
- // TODO: Add special handling for variables
+ if (VarDecl *Var = dyn_cast<VarDecl>(D)) {
+ DoMarkVarDeclReferenced(SemaRef, Loc, Var, E);
+ return;
+ }
+
SemaRef.MarkAnyDeclReferenced(Loc, D);
}
@@ -9788,6 +9860,13 @@
void VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
Visit(E->getExpr());
}
+
+ void VisitImplicitCastExpr(ImplicitCastExpr *E) {
+ Inherited::VisitImplicitCastExpr(E);
+
+ if (E->getCastKind() == CK_LValueToRValue)
+ S.UpdateMarkingForLValueToRValue(E->getSubExpr());
+ }
};
}
More information about the cfe-commits
mailing list