comex created this revision.
comex added reviewers: delesley, dblaikie, rsmith.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.
- Treat arguments to constructor calls the same way as arguments to other calls
(fixes 42856). Previously, arguments to constructors were completely ignored –
even if explicitly marked `param_typestate` or `return_typestate` – except for
one special case to mark the argument to a move constructor as consumed on
return. Even that special case was broken, as it wouldn't apply if a move
constructor happened to be marked with `return_typestate`.
- Constructors are still special-cased when determining the default typestate
for the newly constructed object, unless the constructor is marked
`return_typestate`.
- Stop ignoring `return_typestate` on rvalue reference parameters.
- When a function takes a consumable object by value, treat that as consuming
by default, same as with an rvalue reference.
Note that the object being consumed is always a temporary. If you write:
`void a(X); a((X &&)x);`
...this first constructs a temporary X by moving from `x`, then calls `a`
with a pointer to the temporary. Before and after this change, the move copies
`x`'s typestate to the temporary and marks `x` itself as consumed. But before
this change, if the temporary started out unconsumed (because `x` was
unconsumed before the move), it would still be unconsumed when its destructor
was run after the call. Now it will be considered consumed at that point.
Note that currently, only parameters explicitly marked `return_typestate`
have their state-on-return checked on the callee side. Parameters which are
implicitly consuming due to being rvalue references – or, after this patch,
by-value – are checked only on the caller side. This discrepancy was very
surprising to me as a user. I wrote a fix, but in my codebase it broke some
code that was using `std::forward`. Therefore, I plan to hold off on
submitting the fix until a followup patch, which will generalize the current
`std::move` special case into an attribute that can be applied to any 'cast'
function like `std::move` and `std::forward`.
- Diagnose if a constructor or a static method is marked `set_typestate`,
instead of (respectively) ignoring it or crashing.
- Diagnose if a `param_typestate` or `return_typestate` attribute is attached
to a parameter of non-consumable type. The wording I came up with is a bit
fuzzy; suggestions welcome.
- Fix broken check that was trying to `dyn_cast` a `CXXOperatorCallExpr` to a
sibling class, `CXXMemberCallExpr` (that will never work). Instead, we need to
check whether the declaration is a `CXXMethodDecl`.
- This was harmless in the existing code, for somewhat complicated reasons,
but caused problems after I refactored `handleCall` to take the arguments as a
separate parameter.
Some of these changes have the potential to 'break' code (in the sense of
creating warnings where there were none before) because they check things that
were previously ignored.
One notable case, which showed up in the existing test code
(warn-consumed-analysis.cpp), is a copy constructor that takes its argument by
non-const reference. Previously, the argument would be ignored and thus
implicitly treated as leaving the state alone – just like all other constructor
arguments, except for the argument to a move constructor. Now it's given the
usual treatment for non-const reference parameters, which is to mark the
original object as being in `unknown` state, unless overridden with
`return_typestate`. I think the new behavior is more correct, and this isn't
removing a special case *per se*. However, since it is a "copy" constructor,
there might be some argument that the argument should be treated as
non-consuming instead.
Also, if anyone wants to test this on their code, note that I have a separate
patch pending to make the CFG more accurate with respect to temporary object
destructors (D66404 <https://reviews.llvm.org/D66404>).
Repository:
rC Clang
https://reviews.llvm.org/D66830
Files:
include/clang/Analysis/Analyses/Consumed.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/Analysis/Consumed.cpp
lib/Sema/AnalysisBasedWarnings.cpp
lib/Sema/SemaDeclAttr.cpp
test/SemaCXX/warn-consumed-analysis.cpp
test/SemaCXX/warn-consumed-parsing.cpp
Index: test/SemaCXX/warn-consumed-parsing.cpp
===================================================================
--- test/SemaCXX/warn-consumed-parsing.cpp
+++ test/SemaCXX/warn-consumed-parsing.cpp
@@ -62,5 +62,12 @@
Status {
};
-
+struct CONSUMABLE(unknown) UselessAttrs {
+ static void x() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+ UselessAttrs() SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+ UselessAttrs(int) CALLABLE_WHEN(consumed); // expected-warning {{consumed analysis attribute is attached to a constructor of class 'UselessAttrs'}}
+ void operator+(UselessAttrs) SET_TYPESTATE(consumed); // OK
+ static void *operator new(unsigned long) SET_TYPESTATE(consumed); // expected-warning {{consumed analysis attribute is attached to a static method of class 'UselessAttrs'}}
+};
+void operator-(UselessAttrs, UselessAttrs) SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' attribute only applies to functions}}
Index: test/SemaCXX/warn-consumed-analysis.cpp
===================================================================
--- test/SemaCXX/warn-consumed-analysis.cpp
+++ test/SemaCXX/warn-consumed-analysis.cpp
@@ -19,7 +19,7 @@
ConsumableClass();
ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);
ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);
- ConsumableClass(ConsumableClass<T> &other);
+ ConsumableClass(const ConsumableClass<T> &other);
ConsumableClass(ConsumableClass<T> &&other);
ConsumableClass<T>& operator=(ConsumableClass<T> &other);
@@ -53,10 +53,15 @@
public:
DestructorTester();
DestructorTester(int);
+ DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+ DestructorTester(DestructorTester &&);
void operator*() CALLABLE_WHEN("unconsumed");
~DestructorTester() CALLABLE_WHEN("consumed");
+
+ static void byVal(DestructorTester);
+ static void byValMarkUnconsumed(DestructorTester RETURN_TYPESTATE(unconsumed));
};
void baf0(const ConsumableClass<int> var);
@@ -120,6 +125,19 @@
expected-warning {{invalid invocation of method '~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
}
+void testDestructionByVal() {
+ {
+ // both the var and the temporary are consumed:
+ DestructorTester D0(nullptr);
+ DestructorTester::byVal((DestructorTester &&)D0);
+ }
+ {
+ // the var is consumed but the temporary isn't:
+ DestructorTester D1(nullptr);
+ DestructorTester::byValMarkUnconsumed((DestructorTester &&)D1); // expected-warning {{invalid invocation of method '~DestructorTester' on a temporary object while it is in the 'unconsumed' state}}
+ }
+}
+
void testTempValue() {
*ConsumableClass<int>(); // expected-warning {{invalid invocation of method 'operator*' on a temporary object while it is in the 'consumed' state}}
}
@@ -413,10 +431,15 @@
Param.consume();
}
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass<int> &&Param RETURN_TYPESTATE(unconsumed)) {
+ Param.unconsume();
+}
+
void testParamReturnTypestateCaller() {
ConsumableClass<int> var;
testParamReturnTypestateCallee(true, var);
+ testRvalueRefParamReturnTypestateCallee((ConsumableClass<int> &&)var);
*var;
}
@@ -480,6 +503,9 @@
baf2(&var);
*var;
+
+ baf3(var);
+ *var;
baf4(var);
*var; // expected-warning {{invalid invocation of method 'operator*' on object 'var' while it is in the 'unknown' state}}
@@ -950,3 +976,52 @@
}
} // end namespace PR18260
+namespace NonConstCopyConstructor {
+ struct CONSUMABLE(unknown) Foo {
+ Foo() RETURN_TYPESTATE(unconsumed);
+ Foo(Foo &);
+ void unconsumedCall() const CALLABLE_WHEN(unconsumed);
+ };
+ void test() {
+ Foo foo;
+ Foo bar(foo);
+ foo.unconsumedCall(); // expected-warning {{invalid invocation of method 'unconsumedCall' on object 'foo' while it is in the 'unknown' state}}
+ }
+} // end namespace NonConstCopyConstructor
+
+namespace ParamAttributesOnConstructor {
+ struct CONSUMABLE(unknown) Foo {
+ Foo() RETURN_TYPESTATE(consumed);
+ Foo(PARAM_TYPESTATE(consumed) RETURN_TYPESTATE(unconsumed) Foo &);
+ void unconsumedCall() const CALLABLE_WHEN(unconsumed);
+ };
+ void test() {
+ Foo foo;
+ Foo bar(foo);
+ foo.unconsumedCall();
+ Foo baz(foo); // expected-warning {{argument not in expected state; expected 'consumed', observed 'unconsumed'}}
+ }
+} // end namespace NonConstCopyConstructor
+
+namespace CustomConstructors {
+ struct Foo {
+ Foo(int, ConsumableClass<int> &&c);
+ Foo(int *, RETURN_TYPESTATE(unconsumed) ConsumableClass<int> &&c);
+ };
+
+ void test() {
+ ConsumableClass<int> i1(nullptr);
+ Foo j1(42, (ConsumableClass<int> &&)i1);
+ *i1; // expected-warning {{invalid invocation of method 'operator*' on object 'i1' while it is in the 'consumed' state}}
+ ConsumableClass<int> i2(nullptr);
+ Foo j2(nullptr, (ConsumableClass<int> &&)i2);
+ *i2;
+ }
+}
+
+// This needs to be here instead of warn-consumed-parsing.cpp because these
+// warnings come from analysis.
+struct CONSUMABLE(unknown) MoreUselessAttrs {
+ void a([[clang::param_typestate(consumed)]] int) {} // expected-warning {{consumed analysis attribute is attached to a parameter of type 'int': expected a type marked as consumable passed by value, reference, or rvalue reference}}
+ void b([[clang::return_typestate(consumed)]] MoreUselessAttrs *) {} // expected-warning {{consumed analysis attribute is attached to a parameter of type 'struct MoreUselessAttrs *': expected a type marked as consumable passed by value, reference, or rvalue reference}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===================================================================
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -1161,15 +1161,20 @@
static bool checkForConsumableClass(Sema &S, const CXXMethodDecl *MD,
const ParsedAttr &AL) {
- QualType ThisType = MD->getThisType()->getPointeeType();
+ const CXXRecordDecl *RD = MD->getParent();
- if (const CXXRecordDecl *RD = ThisType->getAsCXXRecordDecl()) {
- if (!RD->hasAttr<ConsumableAttr>()) {
- S.Diag(AL.getLoc(), diag::warn_attr_on_unconsumable_class) <<
- RD->getNameAsString();
+ if (!RD->hasAttr<ConsumableAttr>()) {
+ S.Diag(AL.getLoc(), diag::warn_attr_on_unconsumable_class) <<
+ RD->getNameAsString();
- return false;
- }
+ return false;
+ }
+
+ if (MD->isStatic() || isa<CXXConstructorDecl>(MD)) {
+ S.Diag(AL.getLoc(), diag::warn_consumable_attr_on_static) <<
+ (MD->isStatic() ? 0 : 1) <<
+ RD->getNameAsString();
+ return false;
}
return true;
Index: lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -1914,6 +1914,14 @@
Warnings.emplace_back(std::move(Warning), OptionalNotes());
}
+ void warnParamAttrForUnconsumableType(SourceLocation Loc,
+ StringRef TypeName) override {
+ PartialDiagnosticAt Warning(Loc, S.PDiag(
+ diag::warn_param_attr_for_unconsumable_type) << TypeName);
+
+ Warnings.emplace_back(std::move(Warning), OptionalNotes());
+ }
+
void warnReturnTypestateForUnconsumableType(SourceLocation Loc,
StringRef TypeName) override {
PartialDiagnosticAt Warning(Loc, S.PDiag(
Index: lib/Analysis/Consumed.cpp
===================================================================
--- lib/Analysis/Consumed.cpp
+++ lib/Analysis/Consumed.cpp
@@ -36,8 +36,6 @@
#include <memory>
#include <utility>
-// TODO: Adjust states of args to constructors in the same way that arguments to
-// function calls are handled.
// TODO: Use information from tests in for- and while-loop conditional.
// TODO: Add notes about the actual and expected state for
// TODO: Correctly identify unreachable blocks when chaining boolean operators.
@@ -494,8 +492,8 @@
void checkCallability(const PropagationInfo &PInfo,
const FunctionDecl *FunDecl,
SourceLocation BlameLoc);
- bool handleCall(const CallExpr *Call, const Expr *ObjArg,
- const FunctionDecl *FunD);
+ bool handleCall(const Expr *Call, const Expr *ObjArg,
+ ArrayRef<const Expr *> Args, const FunctionDecl *FunD);
void VisitBinaryOperator(const BinaryOperator *BinOp);
void VisitCallExpr(const CallExpr *Call);
@@ -608,22 +606,21 @@
// Factors out common behavior for function, method, and operator calls.
// Check parameters and set parameter state if necessary.
// Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+ const Expr *ObjArg,
+ ArrayRef<const Expr *> Args,
const FunctionDecl *FunD) {
- unsigned Offset = 0;
- if (isa<CXXOperatorCallExpr>(Call) && isa<CXXMethodDecl>(FunD))
- Offset = 1; // first argument is 'this'
-
// check explicit parameters
- for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+ unsigned Index = 0;
+ for (const Expr *Arg : Args) {
// Skip variable argument lists.
- if (Index - Offset >= FunD->getNumParams())
+ if (Index >= FunD->getNumParams())
break;
- const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+ const ParmVarDecl *Param = FunD->getParamDecl(Index++);
QualType ParamType = Param->getType();
- InfoEntry Entry = findInfo(Call->getArg(Index));
+ InfoEntry Entry = findInfo(Arg);
if (Entry == PropagationMap.end() || Entry->second.isTest())
continue;
@@ -636,7 +633,7 @@
if (ParamState != ExpectedState)
Analyzer.WarningsHandler.warnParamTypestateMismatch(
- Call->getArg(Index)->getExprLoc(),
+ Arg->getExprLoc(),
stateToString(ExpectedState), stateToString(ParamState));
}
@@ -644,10 +641,10 @@
continue;
// Adjust state on the caller side.
- if (isRValueRef(ParamType))
- setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
- else if (ReturnTypestateAttr *RT = Param->getAttr<ReturnTypestateAttr>())
+ if (ReturnTypestateAttr *RT = Param->getAttr<ReturnTypestateAttr>())
setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+ else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+ setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
else if (isPointerOrRef(ParamType) &&
(!ParamType->getPointeeType().isConstQualified() ||
isSetOnReadPtrType(ParamType)))
@@ -749,7 +746,9 @@
return;
}
- handleCall(Call, nullptr, FunDecl);
+ handleCall(Call, nullptr,
+ llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()),
+ FunDecl);
propagateReturnType(Call, FunDecl);
}
@@ -773,30 +772,25 @@
QualType ThisType = Constructor->getThisType()->getPointeeType();
- if (!isConsumableType(ThisType))
- return;
-
- // FIXME: What should happen if someone annotates the move constructor?
- if (ReturnTypestateAttr *RTA = Constructor->getAttr<ReturnTypestateAttr>()) {
- // TODO: Adjust state of args appropriately.
- ConsumedState RetState = mapReturnTypestateAttrState(RTA);
- PropagationMap.insert(PairType(Call, PropagationInfo(RetState)));
- } else if (Constructor->isDefaultConstructor()) {
- PropagationMap.insert(PairType(Call,
- PropagationInfo(consumed::CS_Consumed)));
- } else if (Constructor->isMoveConstructor()) {
- copyInfo(Call->getArg(0), Call, CS_Consumed);
- } else if (Constructor->isCopyConstructor()) {
- // Copy state from arg. If setStateOnRead then set arg to CS_Unknown.
- ConsumedState NS =
- isSetOnReadPtrType(Constructor->getThisType()) ?
- CS_Unknown : CS_None;
- copyInfo(Call->getArg(0), Call, NS);
- } else {
- // TODO: Adjust state of args appropriately.
- ConsumedState RetState = mapConsumableAttrState(ThisType);
- PropagationMap.insert(PairType(Call, PropagationInfo(RetState)));
+ if (isConsumableType(ThisType)) {
+ if (auto *RTA = Constructor->getAttr<ReturnTypestateAttr>()) {
+ ConsumedState RetState = mapReturnTypestateAttrState(RTA);
+ PropagationMap.insert(PairType(Call, PropagationInfo(RetState)));
+ } else if (Constructor->isDefaultConstructor()) {
+ PropagationMap.insert(PairType(Call,
+ PropagationInfo(consumed::CS_Consumed)));
+ } else if (Constructor->isMoveConstructor() ||
+ Constructor->isCopyConstructor()) {
+ copyInfo(Call->getArg(0), Call, CS_None);
+ } else {
+ ConsumedState RetState = mapConsumableAttrState(ThisType);
+ PropagationMap.insert(PairType(Call, PropagationInfo(RetState)));
+ }
}
+
+ handleCall(Call, nullptr,
+ llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()),
+ Constructor);
}
void ConsumedStmtVisitor::VisitCXXMemberCallExpr(
@@ -805,7 +799,8 @@
if (!MD)
return;
- handleCall(Call, Call->getImplicitObjectArgument(), MD);
+ handleCall(Call, Call->getImplicitObjectArgument(),
+ llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()), MD);
propagateReturnType(Call, MD);
}
@@ -813,18 +808,19 @@
const CXXOperatorCallExpr *Call) {
const auto *FunDecl = dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee());
if (!FunDecl) return;
+ ArrayRef<const Expr *> Args(Call->getArgs(), Call->getNumArgs());
if (Call->getOperator() == OO_Equal) {
- ConsumedState CS = getInfo(Call->getArg(1));
- if (!handleCall(Call, Call->getArg(0), FunDecl))
- setInfo(Call->getArg(0), CS);
+ ConsumedState CS = getInfo(Args[1]);
+ if (!handleCall(Call, Args[0], Args.drop_front(1), FunDecl))
+ setInfo(Args[0], CS);
return;
}
- if (const auto *MCall = dyn_cast<CXXMemberCallExpr>(Call))
- handleCall(MCall, MCall->getImplicitObjectArgument(), FunDecl);
+ if (isa<CXXMethodDecl>(FunDecl))
+ handleCall(Call, Args[0], Args.drop_front(1), FunDecl);
else
- handleCall(Call, Call->getArg(0), FunDecl);
+ handleCall(Call, nullptr, Args, FunDecl);
propagateReturnType(Call, FunDecl);
}
@@ -858,9 +854,7 @@
QualType ParamType = Param->getType();
ConsumedState ParamState = consumed::CS_None;
- if (const ParamTypestateAttr *PTA = Param->getAttr<ParamTypestateAttr>())
- ParamState = mapParamTypestateAttrState(PTA);
- else if (isConsumableType(ParamType))
+ if (isConsumableType(ParamType))
ParamState = mapConsumableAttrState(ParamType);
else if (isRValueRef(ParamType) &&
isConsumableType(ParamType->getPointeeType()))
@@ -869,6 +863,23 @@
isConsumableType(ParamType->getPointeeType()))
ParamState = consumed::CS_Unknown;
+ if (const ParamTypestateAttr *PTA = Param->getAttr<ParamTypestateAttr>()) {
+ if (ParamState == CS_None) {
+ // FIXME: This should be removed when template instantiation propagates
+ // attributes at template specialization definition, not
+ // declaration. When it is removed the test needs to be enabled
+ // in SemaDeclAttr.cpp.
+ Analyzer.WarningsHandler.warnParamAttrForUnconsumableType(
+ PTA->getLocation(), ParamType.getAsString());
+ }
+ ParamState = mapParamTypestateAttrState(PTA);
+ } else if (auto *RTA = Param->getAttr<ReturnTypestateAttr>()) {
+ if (ParamState == CS_None) {
+ Analyzer.WarningsHandler.warnParamAttrForUnconsumableType(
+ RTA->getLocation(), ParamType.getAsString());
+ }
+ }
+
if (ParamState != CS_None)
StateMap->setState(Param, ParamState);
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3185,6 +3185,15 @@
def warn_attr_on_unconsumable_class : Warning<
"consumed analysis attribute is attached to member of class '%0' which isn't "
"marked as consumable">, InGroup<Consumed>, DefaultIgnore;
+def warn_consumable_attr_on_static : Warning<
+ "consumed analysis attribute is attached to a "
+ "%select{static method|constructor}0 of class '%1'">,
+ InGroup<Consumed>, DefaultIgnore;
+def warn_param_attr_for_unconsumable_type : Warning<
+ "consumed analysis attribute is attached to a parameter of type '%0': "
+ "expected a type marked as consumable passed by value, reference, or "
+ "rvalue reference">,
+ InGroup<Consumed>, DefaultIgnore;
def warn_return_typestate_for_unconsumable_type : Warning<
"return state set for an unconsumable type '%0'">, InGroup<Consumed>,
DefaultIgnore;
Index: include/clang/Analysis/Analyses/Consumed.h
===================================================================
--- include/clang/Analysis/Analyses/Consumed.h
+++ include/clang/Analysis/Analyses/Consumed.h
@@ -89,6 +89,16 @@
StringRef ExpectedState,
StringRef ObservedState) {}
+ // FIXME: This can be removed when the attr propagation fix for templated
+ // classes lands.
+ /// Warn about return typestates set for unconsumable types.
+ ///
+ /// \param Loc -- The location of the attributes.
+ ///
+ /// \param TypeName -- The name of the unconsumable type.
+ virtual void warnParamAttrForUnconsumableType(SourceLocation Loc,
+ StringRef TypeName) {}
+
// FIXME: This can be removed when the attr propagation fix for templated
// classes lands.
/// Warn about return typestates set for unconsumable types.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
