[Sema] SequenceChecker: Add some comments + related small NFCs

NFCs factored out of the following patches:
- Change all of the `Expr *` to `const Expr *` in SequenceChecker for
  const-correctness. SequenceChecker should not modify AST nodes.
- Add some comments.
- clang-format

Differential Revision: https://reviews.llvm.org/D57659

Reviewed By: xbolva00
This commit is contained in:
Bruno Ricci 2019-12-22 12:07:26 +00:00
parent d3f5769d5e
commit b6eba31292
No known key found for this signature in database
GPG Key ID: D58C906B2F684D92
2 changed files with 123 additions and 74 deletions

View File

@ -11620,7 +11620,7 @@ private:
void CheckImplicitConversions(Expr *E, SourceLocation CC = SourceLocation());
void CheckBoolLikeConversion(Expr *E, SourceLocation CC);
void CheckForIntOverflow(Expr *E);
void CheckUnsequencedOperations(Expr *E);
void CheckUnsequencedOperations(const Expr *E);
/// Perform semantic checks on a completed expression. This will either
/// be a full-expression or a default argument expression.

View File

@ -12466,8 +12466,8 @@ namespace {
/// Visitor for expressions which looks for unsequenced operations on the
/// same object.
class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
using Base = EvaluatedExprVisitor<SequenceChecker>;
class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
using Base = ConstEvaluatedExprVisitor<SequenceChecker>;
/// A tree of sequenced regions within an expression. Two regions are
/// unsequenced if one is an ancestor or a descendent of the other. When we
@ -12537,7 +12537,7 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
};
/// An object for which we can track unsequenced uses.
using Object = NamedDecl *;
using Object = const NamedDecl *;
/// Different flavors of object usage which we track. We only track the
/// least-sequenced usage of each kind.
@ -12556,17 +12556,19 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
UK_Count = UK_ModAsSideEffect + 1
};
/// Bundle together a sequencing region and the expression corresponding
/// to a specific usage. One Usage is stored for each usage kind in UsageInfo.
struct Usage {
Expr *Use;
const Expr *UsageExpr;
SequenceTree::Seq Seq;
Usage() : Use(nullptr), Seq() {}
Usage() : UsageExpr(nullptr), Seq() {}
};
struct UsageInfo {
Usage Uses[UK_Count];
/// Have we issued a diagnostic for this variable already?
/// Have we issued a diagnostic for this object already?
bool Diagnosed;
UsageInfo() : Uses(), Diagnosed(false) {}
@ -12590,7 +12592,7 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
/// Expressions to check later. We defer checking these to reduce
/// stack usage.
SmallVectorImpl<Expr *> &WorkList;
SmallVectorImpl<const Expr *> &WorkList;
/// RAII object wrapping the visitation of a sequenced subexpression of an
/// expression. At the end of this process, the side-effects of the evaluation
@ -12604,10 +12606,13 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
}
~SequencedSubexpression() {
for (auto &M : llvm::reverse(ModAsSideEffect)) {
UsageInfo &U = Self.UsageMap[M.first];
auto &SideEffectUsage = U.Uses[UK_ModAsSideEffect];
Self.addUsage(U, M.first, SideEffectUsage.Use, UK_ModAsValue);
for (const std::pair<Object, Usage> &M : llvm::reverse(ModAsSideEffect)) {
// Add a new usage with usage kind UK_ModAsValue, and then restore
// the previous usage with UK_ModAsSideEffect (thus clearing it if
// the previous one was empty).
UsageInfo &UI = Self.UsageMap[M.first];
auto &SideEffectUsage = UI.Uses[UK_ModAsSideEffect];
Self.addUsage(M.first, UI, SideEffectUsage.UsageExpr, UK_ModAsValue);
SideEffectUsage = M.second;
}
Self.ModAsSideEffect = OldModAsSideEffect;
@ -12651,49 +12656,60 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
/// Find the object which is produced by the specified expression,
/// if any.
Object getObject(Expr *E, bool Mod) const {
Object getObject(const Expr *E, bool Mod) const {
E = E->IgnoreParenCasts();
if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
if (Mod && (UO->getOpcode() == UO_PreInc || UO->getOpcode() == UO_PreDec))
return getObject(UO->getSubExpr(), Mod);
} else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
} else if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
if (BO->getOpcode() == BO_Comma)
return getObject(BO->getRHS(), Mod);
if (Mod && BO->isAssignmentOp())
return getObject(BO->getLHS(), Mod);
} else if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
} else if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
// FIXME: Check for more interesting cases, like "x.n = ++x.n".
if (isa<CXXThisExpr>(ME->getBase()->IgnoreParenCasts()))
return ME->getMemberDecl();
} else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
} else if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
// FIXME: If this is a reference, map through to its value.
return DRE->getDecl();
return nullptr;
}
/// Note that an object was modified or used by an expression.
void addUsage(UsageInfo &UI, Object O, Expr *Ref, UsageKind UK) {
/// Note that an object \p O was modified or used by an expression
/// \p UsageExpr with usage kind \p UK. \p UI is the \p UsageInfo for
/// the object \p O as obtained via the \p UsageMap.
void addUsage(Object O, UsageInfo &UI, const Expr *UsageExpr, UsageKind UK) {
// Get the old usage for the given object and usage kind.
Usage &U = UI.Uses[UK];
if (!U.Use || !Tree.isUnsequenced(Region, U.Seq)) {
if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) {
// If we have a modification as side effect and are in a sequenced
// subexpression, save the old Usage so that we can restore it later
// in SequencedSubexpression::~SequencedSubexpression.
if (UK == UK_ModAsSideEffect && ModAsSideEffect)
ModAsSideEffect->push_back(std::make_pair(O, U));
U.Use = Ref;
// Then record the new usage with the current sequencing region.
U.UsageExpr = UsageExpr;
U.Seq = Region;
}
}
/// Check whether a modification or use conflicts with a prior usage.
void checkUsage(Object O, UsageInfo &UI, Expr *Ref, UsageKind OtherKind,
bool IsModMod) {
/// Check whether a modification or use of an object \p O in an expression
/// \p UsageExpr conflicts with a prior usage of kind \p OtherKind. \p UI is
/// the \p UsageInfo for the object \p O as obtained via the \p UsageMap.
/// \p IsModMod is true when we are checking for a mod-mod unsequenced
/// usage and false we are checking for a mod-use unsequenced usage.
void checkUsage(Object O, UsageInfo &UI, const Expr *UsageExpr,
UsageKind OtherKind, bool IsModMod) {
if (UI.Diagnosed)
return;
const Usage &U = UI.Uses[OtherKind];
if (!U.Use || !Tree.isUnsequenced(Region, U.Seq))
if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq))
return;
Expr *Mod = U.Use;
Expr *ModOrUse = Ref;
const Expr *Mod = U.UsageExpr;
const Expr *ModOrUse = UsageExpr;
if (OtherKind == UK_Use)
std::swap(Mod, ModOrUse);
@ -12705,47 +12721,76 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
UI.Diagnosed = true;
}
void notePreUse(Object O, Expr *Use) {
UsageInfo &U = UsageMap[O];
// A note on note{Pre, Post}{Use, Mod}:
//
// (It helps to follow the algorithm with an expression such as
// "((++k)++, k) = k" or "k = (k++, k++)". Both contain unsequenced
// operations before C++17 and both are well-defined in C++17).
//
// When visiting a node which uses/modify an object we first call notePreUse
// or notePreMod before visiting its sub-expression(s). At this point the
// children of the current node have not yet been visited and so the eventual
// uses/modifications resulting from the children of the current node have not
// been recorded yet.
//
// We then visit the children of the current node. After that notePostUse or
// notePostMod is called. These will 1) detect an unsequenced modification
// as side effect (as in "k++ + k") and 2) add a new usage with the
// appropriate usage kind.
//
// We also have to be careful that some operation sequences modification as
// side effect as well (for example: || or ,). To account for this we wrap
// the visitation of such a sub-expression (for example: the LHS of || or ,)
// with SequencedSubexpression. SequencedSubexpression is an RAII object
// which record usages which are modifications as side effect, and then
// downgrade them (or more accurately restore the previous usage which was a
// modification as side effect) when exiting the scope of the sequenced
// subexpression.
void notePreUse(Object O, const Expr *UseExpr) {
UsageInfo &UI = UsageMap[O];
// Uses conflict with other modifications.
checkUsage(O, U, Use, UK_ModAsValue, false);
checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/false);
}
void notePostUse(Object O, Expr *Use) {
UsageInfo &U = UsageMap[O];
checkUsage(O, U, Use, UK_ModAsSideEffect, false);
addUsage(U, O, Use, UK_Use);
void notePostUse(Object O, const Expr *UseExpr) {
UsageInfo &UI = UsageMap[O];
checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsSideEffect,
/*IsModMod=*/false);
addUsage(O, UI, UseExpr, /*UsageKind=*/UK_Use);
}
void notePreMod(Object O, Expr *Mod) {
UsageInfo &U = UsageMap[O];
void notePreMod(Object O, const Expr *ModExpr) {
UsageInfo &UI = UsageMap[O];
// Modifications conflict with other modifications and with uses.
checkUsage(O, U, Mod, UK_ModAsValue, true);
checkUsage(O, U, Mod, UK_Use, false);
checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/true);
checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_Use, /*IsModMod=*/false);
}
void notePostMod(Object O, Expr *Use, UsageKind UK) {
UsageInfo &U = UsageMap[O];
checkUsage(O, U, Use, UK_ModAsSideEffect, true);
addUsage(U, O, Use, UK);
void notePostMod(Object O, const Expr *ModExpr, UsageKind UK) {
UsageInfo &UI = UsageMap[O];
checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsSideEffect,
/*IsModMod=*/true);
addUsage(O, UI, ModExpr, /*UsageKind=*/UK);
}
public:
SequenceChecker(Sema &S, Expr *E, SmallVectorImpl<Expr *> &WorkList)
SequenceChecker(Sema &S, const Expr *E,
SmallVectorImpl<const Expr *> &WorkList)
: Base(S.Context), SemaRef(S), Region(Tree.root()), WorkList(WorkList) {
Visit(E);
}
void VisitStmt(Stmt *S) {
void VisitStmt(const Stmt *S) {
// Skip all statements which aren't expressions for now.
}
void VisitExpr(Expr *E) {
void VisitExpr(const Expr *E) {
// By default, just recurse to evaluated subexpressions.
Base::VisitStmt(E);
}
void VisitCastExpr(CastExpr *E) {
void VisitCastExpr(const CastExpr *E) {
Object O = Object();
if (E->getCastKind() == CK_LValueToRValue)
O = getObject(E->getSubExpr(), false);
@ -12757,7 +12802,8 @@ public:
notePostUse(O, E);
}
void VisitSequencedExpressions(Expr *SequencedBefore, Expr *SequencedAfter) {
void VisitSequencedExpressions(const Expr *SequencedBefore,
const Expr *SequencedAfter) {
SequenceTree::Seq BeforeRegion = Tree.allocate(Region);
SequenceTree::Seq AfterRegion = Tree.allocate(Region);
SequenceTree::Seq OldRegion = Region;
@ -12777,7 +12823,7 @@ public:
Tree.merge(AfterRegion);
}
void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE) {
void VisitArraySubscriptExpr(const ArraySubscriptExpr *ASE) {
// C++17 [expr.sub]p1:
// The expression E1[E2] is identical (by definition) to *((E1)+(E2)). The
// expression E1 is sequenced before the expression E2.
@ -12787,7 +12833,7 @@ public:
Base::VisitStmt(ASE);
}
void VisitBinComma(BinaryOperator *BO) {
void VisitBinComma(const BinaryOperator *BO) {
// C++11 [expr.comma]p1:
// Every value computation and side effect associated with the left
// expression is sequenced before every value computation and side
@ -12795,7 +12841,7 @@ public:
VisitSequencedExpressions(BO->getLHS(), BO->getRHS());
}
void VisitBinAssign(BinaryOperator *BO) {
void VisitBinAssign(const BinaryOperator *BO) {
// The modification is sequenced after the value computation of the LHS
// and RHS, so check it before inspecting the operands and update the
// map afterwards.
@ -12825,17 +12871,18 @@ public:
// the assignment is sequenced [...] before the value computation of the
// assignment expression.
// C11 6.5.16/3 has no such rule.
notePostMod(O, BO, SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
: UK_ModAsSideEffect);
notePostMod(O, BO,
SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
: UK_ModAsSideEffect);
}
void VisitCompoundAssignOperator(CompoundAssignOperator *CAO) {
void VisitCompoundAssignOperator(const CompoundAssignOperator *CAO) {
VisitBinAssign(CAO);
}
void VisitUnaryPreInc(UnaryOperator *UO) { VisitUnaryPreIncDec(UO); }
void VisitUnaryPreDec(UnaryOperator *UO) { VisitUnaryPreIncDec(UO); }
void VisitUnaryPreIncDec(UnaryOperator *UO) {
void VisitUnaryPreInc(const UnaryOperator *UO) { VisitUnaryPreIncDec(UO); }
void VisitUnaryPreDec(const UnaryOperator *UO) { VisitUnaryPreIncDec(UO); }
void VisitUnaryPreIncDec(const UnaryOperator *UO) {
Object O = getObject(UO->getSubExpr(), true);
if (!O)
return VisitExpr(UO);
@ -12844,13 +12891,14 @@ public:
Visit(UO->getSubExpr());
// C++11 [expr.pre.incr]p1:
// the expression ++x is equivalent to x+=1
notePostMod(O, UO, SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
: UK_ModAsSideEffect);
notePostMod(O, UO,
SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
: UK_ModAsSideEffect);
}
void VisitUnaryPostInc(UnaryOperator *UO) { VisitUnaryPostIncDec(UO); }
void VisitUnaryPostDec(UnaryOperator *UO) { VisitUnaryPostIncDec(UO); }
void VisitUnaryPostIncDec(UnaryOperator *UO) {
void VisitUnaryPostInc(const UnaryOperator *UO) { VisitUnaryPostIncDec(UO); }
void VisitUnaryPostDec(const UnaryOperator *UO) { VisitUnaryPostIncDec(UO); }
void VisitUnaryPostIncDec(const UnaryOperator *UO) {
Object O = getObject(UO->getSubExpr(), true);
if (!O)
return VisitExpr(UO);
@ -12861,7 +12909,7 @@ public:
}
/// Don't visit the RHS of '&&' or '||' if it might not be evaluated.
void VisitBinLOr(BinaryOperator *BO) {
void VisitBinLOr(const BinaryOperator *BO) {
// The side-effects of the LHS of an '&&' are sequenced before the
// value computation of the RHS, and hence before the value computation
// of the '&&' itself, unless the LHS evaluates to zero. We treat them
@ -12886,7 +12934,7 @@ public:
WorkList.push_back(BO->getRHS());
}
}
void VisitBinLAnd(BinaryOperator *BO) {
void VisitBinLAnd(const BinaryOperator *BO) {
EvaluationTracker Eval(*this);
{
SequencedSubexpression Sequenced(*this);
@ -12904,7 +12952,7 @@ public:
// Only visit the condition, unless we can be sure which subexpression will
// be chosen.
void VisitAbstractConditionalOperator(AbstractConditionalOperator *CO) {
void VisitAbstractConditionalOperator(const AbstractConditionalOperator *CO) {
EvaluationTracker Eval(*this);
{
SequencedSubexpression Sequenced(*this);
@ -12920,7 +12968,7 @@ public:
}
}
void VisitCallExpr(CallExpr *CE) {
void VisitCallExpr(const CallExpr *CE) {
// C++11 [intro.execution]p15:
// When calling a function [...], every value computation and side effect
// associated with any argument expression, or with the postfix expression
@ -12934,7 +12982,7 @@ public:
// FIXME: CXXNewExpr and CXXDeleteExpr implicitly call functions.
}
void VisitCXXConstructExpr(CXXConstructExpr *CCE) {
void VisitCXXConstructExpr(const CXXConstructExpr *CCE) {
// This is a call, so all subexpressions are sequenced before the result.
SequencedSubexpression Sequenced(*this);
@ -12944,8 +12992,8 @@ public:
// In C++11, list initializations are sequenced.
SmallVector<SequenceTree::Seq, 32> Elts;
SequenceTree::Seq Parent = Region;
for (CXXConstructExpr::arg_iterator I = CCE->arg_begin(),
E = CCE->arg_end();
for (CXXConstructExpr::const_arg_iterator I = CCE->arg_begin(),
E = CCE->arg_end();
I != E; ++I) {
Region = Tree.allocate(Parent);
Elts.push_back(Region);
@ -12958,7 +13006,7 @@ public:
Tree.merge(Elts[I]);
}
void VisitInitListExpr(InitListExpr *ILE) {
void VisitInitListExpr(const InitListExpr *ILE) {
if (!SemaRef.getLangOpts().CPlusPlus11)
return VisitExpr(ILE);
@ -12966,8 +13014,9 @@ public:
SmallVector<SequenceTree::Seq, 32> Elts;
SequenceTree::Seq Parent = Region;
for (unsigned I = 0; I < ILE->getNumInits(); ++I) {
Expr *E = ILE->getInit(I);
if (!E) continue;
const Expr *E = ILE->getInit(I);
if (!E)
continue;
Region = Tree.allocate(Parent);
Elts.push_back(Region);
Visit(E);
@ -12982,11 +13031,11 @@ public:
} // namespace
void Sema::CheckUnsequencedOperations(Expr *E) {
SmallVector<Expr *, 8> WorkList;
void Sema::CheckUnsequencedOperations(const Expr *E) {
SmallVector<const Expr *, 8> WorkList;
WorkList.push_back(E);
while (!WorkList.empty()) {
Expr *Item = WorkList.pop_back_val();
const Expr *Item = WorkList.pop_back_val();
SequenceChecker(*this, Item, WorkList);
}
}