Hi doug.gregor,
Previously, Sema was reusing parts of the AST when synthesizing an assignment
operator, turning it into a AS-dag. This caused problems for the static
analyzer, which assumed an expression appears in the tree only once.
Here I make sure to always create a fresh Expr, when inserting something into
the AST, fixing PR16745 in the process.
http://llvm-reviews.chandlerc.com/D1425
Files:
lib/Sema/SemaDeclCXX.cpp
test/Analysis/operator-calls.cpp
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8486,24 +8486,171 @@
// needs to be done somewhere else.
}
+namespace {
+/// \brief An abstract base class for all helper classes used in building the
+// copy/move operators. These classes serve as factory functions and help us
+// avoid using the same Expr* in the AST twice.
+class ExprBuilder {
+ ExprBuilder(const ExprBuilder&) LLVM_DELETED_FUNCTION;
+ ExprBuilder &operator =(const ExprBuilder&) LLVM_DELETED_FUNCTION;
+
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc) const = 0;
+
+public:
+ ExprBuilder() {}
+ virtual ~ExprBuilder() {}
+
+ Expr *Build(Sema &S, SourceLocation Loc) const {
+ Expr *E = DoBuild(S, Loc);
+ assert(E && "Expression construction cannot fail!");
+ return E;
+ }
+};
+
+class NestedExprBuilder: public ExprBuilder {
+ const ExprBuilder &Builder;
+
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const = 0;
+
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+ return DoBuild(S, Loc, Builder.Build(S, Loc));
+ }
+
+public:
+ NestedExprBuilder(const ExprBuilder &Builder) : Builder(Builder) {}
+};
+
+class RefBuilder: public ExprBuilder {
+ VarDecl *Var;
+ QualType VarType;
+
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+ return S.BuildDeclRefExpr(Var, VarType, VK_LValue, Loc).take();
+ }
+
+public:
+ RefBuilder(VarDecl *Var, QualType VarType)
+ : Var(Var), VarType(VarType) {}
+};
+
+class ThisBuilder: public ExprBuilder {
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+ return S.ActOnCXXThis(Loc).takeAs<Expr>();
+ }
+};
+
+class CastBuilder: public NestedExprBuilder {
+ QualType Type;
+ ExprValueKind Kind;
+ const CXXCastPath &Path;
+
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const
+ LLVM_OVERRIDE {
+ return S.ImpCastExprToType(E, Type, CK_UncheckedDerivedToBase, Kind,
+ &Path).take();
+ }
+
+public:
+ CastBuilder(const ExprBuilder &Builder, QualType Type, ExprValueKind Kind,
+ const CXXCastPath &Path)
+ : NestedExprBuilder(Builder), Type(Type), Kind(Kind), Path(Path) {}
+};
+
+class DerefBuilder: public NestedExprBuilder {
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const
+ LLVM_OVERRIDE {
+ return S.CreateBuiltinUnaryOp(Loc, UO_Deref, E).take();
+ }
+
+public:
+ DerefBuilder(const ExprBuilder &Builder) : NestedExprBuilder(Builder) {}
+};
+
+class MemberBuilder: public NestedExprBuilder {
+ QualType Type;
+ CXXScopeSpec SS;
+ bool IsArrow;
+ LookupResult &MemberLookup;
+
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const
+ LLVM_OVERRIDE {
+ return S.BuildMemberReferenceExpr(E, Type, Loc, IsArrow, SS,
+ SourceLocation(), 0, MemberLookup,
+ 0).take();
+ }
+
+public:
+ MemberBuilder(const ExprBuilder &Builder, QualType Type, bool IsArrow,
+ LookupResult &MemberLookup)
+ : NestedExprBuilder(Builder), Type(Type), IsArrow(IsArrow),
+ MemberLookup(MemberLookup) {}
+};
+
+class MoveCastBuilder: public NestedExprBuilder {
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const
+ LLVM_OVERRIDE {
+ return CastForMoving(S, E);
+ }
+
+public:
+ MoveCastBuilder(const ExprBuilder &Builder) : NestedExprBuilder(Builder) {}
+};
+
+class LvalueConvBuilder: public NestedExprBuilder {
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const
+ LLVM_OVERRIDE {
+ return S.DefaultLvalueConversion(E).take();
+ }
+
+public:
+ LvalueConvBuilder(const ExprBuilder &Builder) : NestedExprBuilder(Builder) {}
+};
+
+class SubscriptBuilder: public ExprBuilder {
+ const ExprBuilder &Base;
+ const ExprBuilder &Idx;
+protected:
+ virtual Expr *DoBuild(Sema &S, SourceLocation Loc) const
+ LLVM_OVERRIDE {
+ return S.CreateBuiltinArraySubscriptExpr(Base.Build(S, Loc), Loc,
+ Idx.Build(S, Loc), Loc).take();
+ }
+
+public:
+ SubscriptBuilder(const ExprBuilder &Base, const ExprBuilder &Idx)
+ : Base(Base), Idx(Idx) {}
+};
+
+} // end anonymous namespace
+
/// When generating a defaulted copy or move assignment operator, if a field
/// should be copied with __builtin_memcpy rather than via explicit assignments,
/// do so. This optimization only applies for arrays of scalars, and for arrays
/// of class type where the selected copy/move-assignment operator is trivial.
static StmtResult
buildMemcpyForAssignmentOp(Sema &S, SourceLocation Loc, QualType T,
- Expr *To, Expr *From) {
+ const ExprBuilder &ToB, const ExprBuilder &FromB) {
// Compute the size of the memory buffer to be copied.
QualType SizeType = S.Context.getSizeType();
llvm::APInt Size(S.Context.getTypeSize(SizeType),
S.Context.getTypeSizeInChars(T).getQuantity());
// Take the address of the field references for "from" and "to". We
// directly construct UnaryOperators here because semantic analysis
// does not permit us to take the address of an xvalue.
+ Expr *From = FromB.Build(S, Loc);
From = new (S.Context) UnaryOperator(From, UO_AddrOf,
S.Context.getPointerType(From->getType()),
VK_RValue, OK_Ordinary, Loc);
+ Expr *To = ToB.Build(S, Loc);
To = new (S.Context) UnaryOperator(To, UO_AddrOf,
S.Context.getPointerType(To->getType()),
VK_RValue, OK_Ordinary, Loc);
@@ -8569,7 +8716,7 @@
/// if a memcpy should be used instead.
static StmtResult
buildSingleCopyAssignRecursively(Sema &S, SourceLocation Loc, QualType T,
- Expr *To, Expr *From,
+ const ExprBuilder &To, const ExprBuilder &From,
bool CopyingBaseSubobject, bool Copying,
unsigned Depth = 0) {
// C++11 [class.copy]p28:
@@ -8642,8 +8789,8 @@
// Create the reference to operator=.
ExprResult OpEqualRef
- = S.BuildMemberReferenceExpr(To, T, Loc, /*isArrow=*/false, SS,
- /*TemplateKWLoc=*/SourceLocation(),
+ = S.BuildMemberReferenceExpr(To.Build(S, Loc), T, Loc, /*isArrow=*/false,
+ SS, /*TemplateKWLoc=*/SourceLocation(),
/*FirstQualifierInScope=*/0,
OpLookup,
/*TemplateArgs=*/0,
@@ -8653,9 +8800,10 @@
// Build the call to the assignment operator.
+ Expr *FromInst = From.Build(S, Loc);
ExprResult Call = S.BuildCallToMemberFunction(/*Scope=*/0,
OpEqualRef.takeAs<Expr>(),
- Loc, From, Loc);
+ Loc, FromInst, Loc);
if (Call.isInvalid())
return StmtError();
@@ -8674,7 +8822,8 @@
// operator is used.
const ConstantArrayType *ArrayTy = S.Context.getAsConstantArrayType(T);
if (!ArrayTy) {
- ExprResult Assignment = S.CreateBuiltinBinOp(Loc, BO_Assign, To, From);
+ ExprResult Assignment = S.CreateBuiltinBinOp(
+ Loc, BO_Assign, To.Build(S, Loc), From.Build(S, Loc));
if (Assignment.isInvalid())
return StmtError();
return S.ActOnExprStmt(Assignment);
@@ -8707,31 +8856,24 @@
llvm::APInt Zero(S.Context.getTypeSize(SizeType), 0);
IterationVar->setInit(IntegerLiteral::Create(S.Context, Zero, SizeType, Loc));
- // Create a reference to the iteration variable; we'll use this several
- // times throughout.
- Expr *IterationVarRef
- = S.BuildDeclRefExpr(IterationVar, SizeType, VK_LValue, Loc).take();
- assert(IterationVarRef && "Reference to invented variable cannot fail!");
- Expr *IterationVarRefRVal = S.DefaultLvalueConversion(IterationVarRef).take();
- assert(IterationVarRefRVal && "Conversion of invented variable cannot fail!");
+ // Creates a reference to the iteration variable.
+ RefBuilder IterationVarRef(IterationVar, SizeType);
+ LvalueConvBuilder IterationVarRefRVal(IterationVarRef);
// Create the DeclStmt that holds the iteration variable.
Stmt *InitStmt = new (S.Context) DeclStmt(DeclGroupRef(IterationVar),Loc,Loc);
// Subscript the "from" and "to" expressions with the iteration variable.
- From = AssertSuccess(S.CreateBuiltinArraySubscriptExpr(From, Loc,
- IterationVarRefRVal,
- Loc));
- To = AssertSuccess(S.CreateBuiltinArraySubscriptExpr(To, Loc,
- IterationVarRefRVal,
- Loc));
- if (!Copying) // Cast to rvalue
- From = CastForMoving(S, From);
+ SubscriptBuilder FromIdx(From, IterationVarRefRVal);
+ SubscriptBuilder ToIdx(To, IterationVarRefRVal);
+ const ExprBuilder &FromIdxCast =
+ Copying ? FromIdx
+ : static_cast<const ExprBuilder &>(MoveCastBuilder(FromIdx));
// Build the copy/move for an individual element of the array.
StmtResult Copy =
buildSingleCopyAssignRecursively(S, Loc, ArrayTy->getElementType(),
- To, From, CopyingBaseSubobject,
+ ToIdx, FromIdxCast, CopyingBaseSubobject,
Copying, Depth + 1);
// Bail out if copying fails or if we determined that we should use memcpy.
if (Copy.isInvalid() || !Copy.get())
@@ -8741,15 +8883,15 @@
llvm::APInt Upper
= ArrayTy->getSize().zextOrTrunc(S.Context.getTypeSize(SizeType));
Expr *Comparison
- = new (S.Context) BinaryOperator(IterationVarRefRVal,
+ = new (S.Context) BinaryOperator(IterationVarRefRVal.Build(S, Loc),
IntegerLiteral::Create(S.Context, Upper, SizeType, Loc),
BO_NE, S.Context.BoolTy,
VK_RValue, OK_Ordinary, Loc, false);
// Create the pre-increment of the iteration variable.
Expr *Increment
- = new (S.Context) UnaryOperator(IterationVarRef, UO_PreInc, SizeType,
- VK_LValue, OK_Ordinary, Loc);
+ = new (S.Context) UnaryOperator(IterationVarRef.Build(S, Loc), UO_PreInc,
+ SizeType, VK_LValue, OK_Ordinary, Loc);
// Construct the loop that copies all elements of this array.
return S.ActOnForStmt(Loc, Loc, InitStmt,
@@ -8760,7 +8902,7 @@
static StmtResult
buildSingleCopyAssign(Sema &S, SourceLocation Loc, QualType T,
- Expr *To, Expr *From,
+ const ExprBuilder &To, const ExprBuilder &From,
bool CopyingBaseSubobject, bool Copying) {
// Maybe we should use a memcpy?
if (T->isArrayType() && !T.isConstQualified() && !T.isVolatileQualified() &&
@@ -9018,15 +9160,11 @@
// Our location for everything implicitly-generated.
SourceLocation Loc = CopyAssignOperator->getLocation();
- // Construct a reference to the "other" object. We'll be using this
- // throughout the generated ASTs.
- Expr *OtherRef = BuildDeclRefExpr(Other, OtherRefType, VK_LValue, Loc).take();
- assert(OtherRef && "Reference to parameter cannot fail!");
-
- // Construct the "this" pointer. We'll be using this throughout the generated
- // ASTs.
- Expr *This = ActOnCXXThis(Loc).takeAs<Expr>();
- assert(This && "Reference to this cannot fail!");
+ // Builds a DeclRefExpr for the "other" object.
+ RefBuilder OtherRef(Other, OtherRefType);
+
+ // Builds the "this" pointer.
+ ThisBuilder This;
// Assign base classes.
bool Invalid = false;
@@ -9045,24 +9183,19 @@
// Construct the "from" expression, which is an implicit cast to the
// appropriately-qualified base type.
- Expr *From = OtherRef;
- From = ImpCastExprToType(From, Context.getQualifiedType(BaseType, OtherQuals),
- CK_UncheckedDerivedToBase,
- VK_LValue, &BasePath).take();
+ CastBuilder From(OtherRef, Context.getQualifiedType(BaseType, OtherQuals),
+ VK_LValue, BasePath);
// Dereference "this".
- ExprResult To = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
-
- // Implicitly cast "this" to the appropriately-qualified base type.
- To = ImpCastExprToType(To.take(),
- Context.getCVRQualifiedType(BaseType,
- CopyAssignOperator->getTypeQualifiers()),
- CK_UncheckedDerivedToBase,
- VK_LValue, &BasePath);
+ DerefBuilder DerefThis(This);
+ CastBuilder To(DerefThis,
+ Context.getCVRQualifiedType(
+ BaseType, CopyAssignOperator->getTypeQualifiers()),
+ VK_LValue, BasePath);
// Build the copy.
StmtResult Copy = buildSingleCopyAssign(*this, Loc, BaseType,
- To.get(), From,
+ To, From,
/*CopyingBaseSubobject=*/true,
/*Copying=*/true);
if (Copy.isInvalid()) {
@@ -9128,20 +9261,14 @@
LookupMemberName);
MemberLookup.addDecl(*Field);
MemberLookup.resolveKind();
- ExprResult From = BuildMemberReferenceExpr(OtherRef, OtherRefType,
- Loc, /*IsArrow=*/false,
- SS, SourceLocation(), 0,
- MemberLookup, 0);
- ExprResult To = BuildMemberReferenceExpr(This, This->getType(),
- Loc, /*IsArrow=*/true,
- SS, SourceLocation(), 0,
- MemberLookup, 0);
- assert(!From.isInvalid() && "Implicit field reference cannot fail");
- assert(!To.isInvalid() && "Implicit field reference cannot fail");
+
+ MemberBuilder From(OtherRef, OtherRefType, /*IsArrow=*/false, MemberLookup);
+
+ MemberBuilder To(This, getCurrentThisType(), /*IsArrow=*/true, MemberLookup);
// Build the copy of this field.
StmtResult Copy = buildSingleCopyAssign(*this, Loc, FieldType,
- To.get(), From.get(),
+ To, From,
/*CopyingBaseSubobject=*/false,
/*Copying=*/true);
if (Copy.isInvalid()) {
@@ -9157,7 +9284,7 @@
if (!Invalid) {
// Add a "return *this;"
- ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
+ ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This.Build(*this, Loc));
StmtResult Return = ActOnReturnStmt(Loc, ThisObj.get());
if (Return.isInvalid())
@@ -9476,17 +9603,13 @@
// Our location for everything implicitly-generated.
SourceLocation Loc = MoveAssignOperator->getLocation();
- // Construct a reference to the "other" object. We'll be using this
- // throughout the generated ASTs.
- Expr *OtherRef = BuildDeclRefExpr(Other, OtherRefType, VK_LValue, Loc).take();
- assert(OtherRef && "Reference to parameter cannot fail!");
+ // Builds a reference to the "other" object.
+ RefBuilder OtherRef(Other, OtherRefType);
// Cast to rvalue.
- OtherRef = CastForMoving(*this, OtherRef);
+ MoveCastBuilder MoveOther(OtherRef);
- // Construct the "this" pointer. We'll be using this throughout the generated
- // ASTs.
- Expr *This = ActOnCXXThis(Loc).takeAs<Expr>();
- assert(This && "Reference to this cannot fail!");
+ // Builds the "this" pointer.
+ ThisBuilder This;
// Assign base classes.
bool Invalid = false;
@@ -9505,23 +9628,20 @@
// Construct the "from" expression, which is an implicit cast to the
// appropriately-qualified base type.
- Expr *From = OtherRef;
- From = ImpCastExprToType(From, BaseType, CK_UncheckedDerivedToBase,
- VK_XValue, &BasePath).take();
+ CastBuilder From(OtherRef, BaseType, VK_XValue, BasePath);
// Dereference "this".
- ExprResult To = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
+ DerefBuilder DerefThis(This);
// Implicitly cast "this" to the appropriately-qualified base type.
- To = ImpCastExprToType(To.take(),
- Context.getCVRQualifiedType(BaseType,
- MoveAssignOperator->getTypeQualifiers()),
- CK_UncheckedDerivedToBase,
- VK_LValue, &BasePath);
+ CastBuilder To(DerefThis,
+ Context.getCVRQualifiedType(
+ BaseType, MoveAssignOperator->getTypeQualifiers()),
+ VK_LValue, BasePath);
// Build the move.
StmtResult Move = buildSingleCopyAssign(*this, Loc, BaseType,
- To.get(), From,
+ To, From,
/*CopyingBaseSubobject=*/true,
/*Copying=*/false);
if (Move.isInvalid()) {
@@ -9582,29 +9702,22 @@
}
// Build references to the field in the object we're copying from and to.
- CXXScopeSpec SS; // Intentionally empty
LookupResult MemberLookup(*this, Field->getDeclName(), Loc,
LookupMemberName);
MemberLookup.addDecl(*Field);
MemberLookup.resolveKind();
- ExprResult From = BuildMemberReferenceExpr(OtherRef, OtherRefType,
- Loc, /*IsArrow=*/false,
- SS, SourceLocation(), 0,
- MemberLookup, 0);
- ExprResult To = BuildMemberReferenceExpr(This, This->getType(),
- Loc, /*IsArrow=*/true,
- SS, SourceLocation(), 0,
- MemberLookup, 0);
- assert(!From.isInvalid() && "Implicit field reference cannot fail");
- assert(!To.isInvalid() && "Implicit field reference cannot fail");
-
- assert(!From.get()->isLValue() && // could be xvalue or prvalue
+ MemberBuilder From(MoveOther, OtherRefType,
+ /*IsArrow=*/false, MemberLookup);
+ MemberBuilder To(This, getCurrentThisType(),
+ /*IsArrow=*/true, MemberLookup);
+
+ assert(!From.Build(*this, Loc)->isLValue() && // could be xvalue or prvalue
"Member reference with rvalue base must be rvalue except for reference "
"members, which aren't allowed for move assignment.");
// Build the move of this field.
StmtResult Move = buildSingleCopyAssign(*this, Loc, FieldType,
- To.get(), From.get(),
+ To, From,
/*CopyingBaseSubobject=*/false,
/*Copying=*/false);
if (Move.isInvalid()) {
@@ -9620,7 +9733,7 @@
if (!Invalid) {
// Add a "return *this;"
- ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
+ ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This.Build(*this, Loc));
StmtResult Return = ActOnReturnStmt(Loc, ThisObj.get());
if (Return.isInvalid())
Index: test/Analysis/operator-calls.cpp
===================================================================
--- test/Analysis/operator-calls.cpp
+++ test/Analysis/operator-calls.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -verify %s
void clang_analyzer_eval(bool);
struct X0 { };
@@ -85,3 +85,48 @@
clang_analyzer_eval(+(coin ? getLargeOpaque() : getLargeOpaque())); // expected-warning{{UNKNOWN}}
}
}
+
+namespace SynthesizedAssignment {
+ struct A {
+ int a;
+ A& operator=(A& other) { a = -other.a; return *this; }
+ A& operator=(A&& other) { a = other.a+1; return *this; }
+ };
+
+ struct B {
+ int x;
+ A a[3];
+ B& operator=(B&) = default;
+ B& operator=(B&&) = default;
+ };
+
+ // This used to produce a warning about the iteration variable in the
+ // synthesized assignment operator being undefined.
+ void testNoWarning() {
+ B v, u;
+ u = v;
+ }
+
+ void testNoWarningMove() {
+ B v, u;
+ u = static_cast<B &&>(v);
+ }
+
+ void testConsistency() {
+ B v, u;
+ v.a[1].a = 47;
+ v.a[2].a = 42;
+ u = v;
+ clang_analyzer_eval(u.a[1].a == -47); // expected-warning{{TRUE}}
+ clang_analyzer_eval(u.a[2].a == -42); // expected-warning{{TRUE}}
+ }
+
+ void testConsistencyMove() {
+ B v, u;
+ v.a[1].a = 47;
+ v.a[2].a = 42;
+ u = static_cast<B &&>(v);
+ clang_analyzer_eval(u.a[1].a == 48); // expected-warning{{TRUE}}
+ clang_analyzer_eval(u.a[2].a == 43); // expected-warning{{TRUE}}
+ }
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
