riccibruno updated this revision to Diff 234326.
riccibruno edited the summary of this revision.
riccibruno added a comment.
I have factored out various NFCs which were present in this patch. This should
make review easier.
Also addressed some inline comments.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57660/new/
https://reviews.llvm.org/D57660
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/SemaCXX/warn-unsequenced.cpp
Index: clang/test/SemaCXX/warn-unsequenced.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsequenced.cpp
+++ clang/test/SemaCXX/warn-unsequenced.cpp
@@ -279,17 +279,21 @@
void member_f(S1 &s);
};
+class SomeClass { public: static int x; };
+union SomeUnion { public: static int x; };
+
void S1::member_f(S1 &s) {
- ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
- a + ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+ ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to member 'a'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'a'}}
+ a + ++a; // cxx11-warning {{unsequenced modification and access to member 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'a'}}
++a + ++b; // no-warning
a + ++b; // no-warning
- // TODO: Warn here.
- ++s.a + ++s.a; // no-warning TODO {{multiple unsequenced modifications to}}
- s.a + ++s.a; // no-warning TODO {{unsequenced modification and access to}}
+ ++s.a + ++s.a; // cxx11-warning {{multiple unsequenced modifications to member 'a' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'a' of 's'}}
+ s.a + ++s.a; // cxx11-warning {{unsequenced modification and access to member 'a' of 's'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'a' of 's'}}
++s.a + ++s.b; // no-warning
s.a + ++s.b; // no-warning
@@ -299,16 +303,18 @@
a + ++s.b; // no-warning
// TODO Warn here for bit-fields in the same memory location.
- ++bf1 + ++bf1; // cxx11-warning {{multiple unsequenced modifications to 'bf1'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'bf1'}}
- bf1 + ++bf1; // cxx11-warning {{unsequenced modification and access to 'bf1'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'bf1'}}
+ ++bf1 + ++bf1; // cxx11-warning {{multiple unsequenced modifications to member 'bf1'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'bf1'}}
+ bf1 + ++bf1; // cxx11-warning {{unsequenced modification and access to member 'bf1'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'bf1'}}
++bf1 + ++bf2; // no-warning TODO {{multiple unsequenced modifications to}}
bf1 + ++bf2; // no-warning TODO {{unsequenced modification and access to}}
// TODO Warn here for bit-fields in the same memory location.
- ++s.bf1 + ++s.bf1; // no-warning TODO {{multiple unsequenced modifications to}}
- s.bf1 + ++s.bf1; // no-warning TODO {{unsequenced modification and access to}}
+ ++s.bf1 + ++s.bf1; // cxx11-warning {{multiple unsequenced modifications to member 'bf1' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'bf1' of 's'}}
+ s.bf1 + ++s.bf1; // cxx11-warning {{unsequenced modification and access to member 'bf1' of 's'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'bf1' of 's'}}
++s.bf1 + ++s.bf2; // no-warning TODO {{multiple unsequenced modifications to}}
s.bf1 + ++s.bf2; // no-warning TODO {{unsequenced modification and access to}}
@@ -322,19 +328,29 @@
Der &d_ref = d;
S1 &s1_ref = d_ref;
- ++s1_ref.a + ++d_ref.a; // no-warning TODO {{multiple unsequenced modifications to member 'a' of 'd'}}
- ++s1_ref.a + d_ref.a; // no-warning TODO {{unsequenced modification and access to member 'a' of 'd'}}
+ ++s1_ref.a + ++d_ref.a; // cxx11-warning {{multiple unsequenced modifications to member 'a' of 'd'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'a' of 'd'}}
+ ++s1_ref.a + d_ref.a; // cxx11-warning {{unsequenced modification and access to member 'a' of 'd'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'a' of 'd'}}
++s1_ref.a + ++d_ref.b; // no-warning
++s1_ref.a + d_ref.b; // no-warning
- ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
- ++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
- ++s.x + x; // no-warning TODO {{unsequenced modification and access to static member 'x' of 'S1'}}
- ++this->x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
- ++d_ref.x + ++S1::x; // no-warning TODO {{unsequenced modification and access to static member 'x' of 'S1'}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to static member 'x' of struct 'S1'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to static member 'x' of struct 'S1'}}
+ ++x + x; // cxx11-warning {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ ++s.x + x; // cxx11-warning {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ ++this->x + x; // cxx11-warning {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ ++d_ref.x + S1::x; // cxx11-warning {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to static member 'x' of struct 'S1'}}
+
+ SomeClass::x++ + SomeClass::x; // cxx11-warning {{unsequenced modification and access to static member 'x' of class 'SomeClass'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to static member 'x' of class 'SomeClass'}}
+ SomeUnion::x++ + SomeUnion::x; // cxx11-warning {{unsequenced modification and access to static member 'x' of union 'SomeUnion'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to static member 'x' of union 'SomeUnion'}}
+ SomeClass::x++ + SomeUnion::x++; // no-warning
}
struct S2 {
@@ -343,15 +359,19 @@
};
void S2::f2() {
- ++x + ++x; // no-warning TODO {{multiple unsequenced modifications to}}
- x + ++x; // no-warning TODO {{unsequenced modification and access to}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x'}}
+ x + ++x; // cxx11-warning {{unsequenced modification and access to member 'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x'}}
++x + ++y; // no-warning
x + ++y; // no-warning
}
void f2(S2 &s) {
- ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
- s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+ ++s.x + ++s.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 's'}}
+ s.x + ++s.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 's'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 's'}}
++s.x + ++s.y; // no-warning
s.x + ++s.y; // no-warning
}
@@ -367,15 +387,19 @@
};
void S3::f3() {
- ++x + ++x; // no-warning TODO {{multiple unsequenced modifications to}}
- x + ++x; // no-warning TODO {{unsequenced modification and access to}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x'}}
+ x + ++x; // cxx11-warning {{unsequenced modification and access to member 'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x'}}
++x + ++y; // no-warning
x + ++y; // no-warning
}
void f3(S3 &s) {
- ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
- s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+ ++s.x + ++s.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 's'}}
+ s.x + ++s.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 's'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 's'}}
++s.x + ++s.y; // no-warning
s.x + ++s.y; // no-warning
}
@@ -386,8 +410,10 @@
};
void S4::f4() {
- ++x + ++x; // no-warning TODO {{multiple unsequenced modifications to}}
- x + ++x; // no-warning TODO {{unsequenced modification and access to}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x'}}
+ x + ++x; // cxx11-warning {{unsequenced modification and access to member 'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x'}}
++x + ++y; // no-warning
x + ++y; // no-warning
++S3::y + ++y; // no-warning
@@ -395,8 +421,10 @@
}
void f4(S4 &s) {
- ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
- s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+ ++s.x + ++s.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 's'}}
+ s.x + ++s.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 's'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 's'}}
++s.x + ++s.y; // no-warning
s.x + ++s.y; // no-warning
++s.S3::y + ++s.y; // no-warning
@@ -409,22 +437,28 @@
};
void f5() {
- ++Ux + ++Ux; // no-warning TODO {{multiple unsequenced modifications to}}
- Ux + ++Ux; // no-warning TODO {{unsequenced modification and access to}}
+ ++Ux + ++Ux; // cxx11-warning {{multiple unsequenced modifications to member 'Ux' of ''}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'Ux' of ''}}
+ Ux + ++Ux; // cxx11-warning {{unsequenced modification and access to member 'Ux' of ''}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'Ux' of ''}}
++Ux + ++Uy; // no-warning
Ux + ++Uy; // no-warning
}
void f6() {
struct S { unsigned x, y; } s;
- ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
- s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+ ++s.x + ++s.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 's'}}
+ s.x + ++s.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 's'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 's'}}
++s.x + ++s.y; // no-warning
s.x + ++s.y; // no-warning
struct { unsigned x, y; } t;
- ++t.x + ++t.x; // no-warning TODO {{multiple unsequenced modifications to}}
- t.x + ++t.x; // no-warning TODO {{unsequenced modification and access to}}
+ ++t.x + ++t.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 't'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 't'}}
+ t.x + ++t.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 't'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 't'}}
++t.x + ++t.y; // no-warning
t.x + ++t.y; // no-warning
}
@@ -433,18 +467,18 @@
namespace references {
void reference_f() {
- // TODO: Check that we can see through references.
- // For now this is completely unhandled.
+ // Check that we can see through references.
int a;
- int xs[10];
int &b = a;
int &c = b;
int &ra1 = c;
int &ra2 = b;
int other;
- ++ra1 + ++ra2; // no-warning TODO {{multiple unsequenced modifications to}}
- ra1 + ++ra2; // no-warning TODO {{unsequenced modification and access to}}
+ ++ra1 + ++ra2; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
+ ra1 + ++ra2; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
++ra1 + ++other; // no-warning
ra1 + ++other; // no-warning
@@ -476,10 +510,10 @@
A a;
{
auto [x, y] = a;
- ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
- ++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of structured binding '[x, y]'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of structured binding '[x, y]'}}
+ ++x + x; // cxx11-warning {{unsequenced modification and access to member 'x' of structured binding '[x, y]'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of structured binding '[x, y]'}}
++x + ++y; // no-warning
++x + y; // no-warning
++x + ++a.x; // no-warning
@@ -487,16 +521,19 @@
}
{
auto &[x, y] = a;
- ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
- ++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 'a'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 'a'}}
+ ++x + x; // cxx11-warning {{unsequenced modification and access to member 'x' of 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 'a'}}
++x + ++y; // no-warning
++x + y; // no-warning
- ++x + ++a.x; // no-warning TODO
- ++x + a.x; // no-warning TODO
+ ++x + ++a.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 'a'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 'a'}}
+ ++x + a.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 'a'}}
}
}
+
void testb() {
B b;
{
@@ -518,6 +555,8 @@
// cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
++x + ++y; // no-warning
++x + y; // no-warning
+ // TODO: We don't handle these cases because we don't track memory locations
+ // through array subscript expressions for now.
++x + ++b[0]; // no-warning TODO
++x + b[0]; // no-warning TODO
}
@@ -547,10 +586,10 @@
D d;
{
auto [x, y] = d;
- ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
- ++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of structured binding '[x, y]'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of structured binding '[x, y]'}}
+ ++x + x; // cxx11-warning {{unsequenced modification and access to member 'x' of structured binding '[x, y]'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of structured binding '[x, y]'}}
++x + ++y; // no-warning
++x + y; // no-warning
++x + ++d.x; // no-warning
@@ -558,14 +597,16 @@
}
{
auto &[x, y] = d;
- ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
- ++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
- // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+ ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 'd'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 'd'}}
+ ++x + x; // cxx11-warning {{unsequenced modification and access to member 'x' of 'd'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 'd'}}
++x + ++y; // no-warning
++x + y; // no-warning
- ++x + ++d.x; // no-warning TODO
- ++x + d.x; // no-warning TODO
+ ++x + ++d.x; // cxx11-warning {{multiple unsequenced modifications to member 'x' of 'd'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'x' of 'd'}}
+ ++x + d.x; // cxx11-warning {{unsequenced modification and access to member 'x' of 'd'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to member 'x' of 'd'}}
}
}
} // namespace bindings
@@ -635,7 +676,8 @@
var<int>++ + var<int>; // cxx11-warning {{unsequenced modification and access to 'var<int>'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'var<int>'}}
int &r = var<int>;
- r++ + var<int>++; // no-warning TODO {{multiple unsequenced modifications to 'var<int>'}}
+ r++ + var<int>++; // cxx11-warning {{multiple unsequenced modifications to 'var<int>'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to 'var<int>'}}
r++ + var<long>++; // no-warning
}
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -12501,8 +12501,85 @@
namespace {
+/// An approximation of a C++ memory location used by SequenceChecker.
+/// TODO: Handle bit-fields.
+/// TODO: Give better info with references.
+class MemoryLocation {
+ friend struct llvm::DenseMapInfo<MemoryLocation>;
+ /// An object. As a special case this represent the C++ implicit object
+ /// if the ValueDecl * is null and the flag is true.
+ llvm::PointerIntPair<const ValueDecl *, 1> ObjectOrCXXThis;
+ /// If non-null, a field in a record type.
+ const ValueDecl *Field = nullptr;
+
+public:
+ struct CXXThisTag {};
+ MemoryLocation() = default;
+ MemoryLocation(const ValueDecl *Object, const ValueDecl *Field)
+ : ObjectOrCXXThis(Object, /*IsCXXThis=*/false), Field(Field) {}
+ MemoryLocation(CXXThisTag, const ValueDecl *Field)
+ : ObjectOrCXXThis(nullptr, /*IsCXXThis=*/true), Field(Field) {}
+ MemoryLocation(void *OpaqueObject, const ValueDecl *Field) : Field(Field) {
+ ObjectOrCXXThis.setFromOpaqueValue(OpaqueObject);
+ }
+
+ explicit operator bool() const { return getObject() || isCXXThis(); }
+ const ValueDecl *getObject() const { return ObjectOrCXXThis.getPointer(); }
+ bool isCXXThis() const { return ObjectOrCXXThis.getInt(); }
+ const ValueDecl *getField() const { return Field; }
+ void *getOpaqueObject() const { return ObjectOrCXXThis.getOpaqueValue(); }
+
+ friend bool operator==(const MemoryLocation &MemoryLoc1,
+ const MemoryLocation &MemoryLoc2) {
+ return !(MemoryLoc1 != MemoryLoc2);
+ }
+ friend bool operator!=(const MemoryLocation &MemoryLoc1,
+ const MemoryLocation &MemoryLoc2) {
+ return (MemoryLoc1.ObjectOrCXXThis != MemoryLoc2.ObjectOrCXXThis) ||
+ (MemoryLoc1.Field != MemoryLoc2.Field);
+ }
+}; // class MemoryLocation
+
+} // namespace
+
+namespace llvm {
+
+template <> struct llvm::DenseMapInfo<MemoryLocation> {
+ using FirstTy = llvm::PointerIntPair<const ValueDecl *, 1>;
+ using SecondTy = const ValueDecl *;
+ using FirstInfo = llvm::DenseMapInfo<FirstTy>;
+ using SecondInfo = llvm::DenseMapInfo<SecondTy>;
+
+ static MemoryLocation getEmptyKey() {
+ return MemoryLocation(FirstInfo::getEmptyKey().getOpaqueValue(),
+ SecondInfo::getEmptyKey());
+ }
+
+ static MemoryLocation getTombstoneKey() {
+ return MemoryLocation(FirstInfo::getTombstoneKey().getOpaqueValue(),
+ SecondInfo::getTombstoneKey());
+ }
+
+ static unsigned getHashValue(const MemoryLocation &MemoryLoc) {
+ unsigned H1 = FirstInfo::getHashValue(MemoryLoc.ObjectOrCXXThis);
+ unsigned H2 = SecondInfo::getHashValue(MemoryLoc.Field);
+ // Taken from Boost hash_combine. llvm::DenseMapInfo<std::pair>
+ // is significantly slower.
+ return H2 ^ (H1 + 0x9e3779b9 + (H2 << 6) + (H2 >> 2));
+ }
+
+ static bool isEqual(const MemoryLocation &MemoryLoc1,
+ const MemoryLocation &MemoryLoc2) {
+ return MemoryLoc1 == MemoryLoc2;
+ }
+};
+
+} // namespace llvm
+
+namespace {
+
/// Visitor for expressions which looks for unsequenced operations on the
-/// same object.
+/// same memory location.
class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
using Base = ConstEvaluatedExprVisitor<SequenceChecker>;
@@ -12573,21 +12650,18 @@
}
};
- /// An object for which we can track unsequenced uses.
- using Object = const NamedDecl *;
-
- /// Different flavors of object usage which we track. We only track the
- /// least-sequenced usage of each kind.
+ /// Different flavors of memory location usage which we track.
+ /// We only track the least-sequenced usage of each kind.
enum UsageKind {
- /// A read of an object. Multiple unsequenced reads are OK.
+ /// A read of a memory location. Multiple unsequenced reads are OK.
UK_Use,
- /// A modification of an object which is sequenced before the value
+ /// A modification of a memory location which is sequenced before the value
/// computation of the expression, such as ++n in C++.
UK_ModAsValue,
- /// A modification of an object which is not sequenced before the value
- /// computation of the expression, such as n++.
+ /// A modification of a memory location which is not sequenced before
+ /// the value computation of the expression, such as n++.
UK_ModAsSideEffect,
UK_Count = UK_ModAsSideEffect + 1
@@ -12605,12 +12679,12 @@
struct UsageInfo {
Usage Uses[UK_Count];
- /// Have we issued a diagnostic for this object already?
+ /// Have we issued a diagnostic for this memory location already?
bool Diagnosed;
UsageInfo() : Uses(), Diagnosed(false) {}
};
- using UsageInfoMap = llvm::SmallDenseMap<Object, UsageInfo, 16>;
+ using UsageInfoMap = llvm::SmallDenseMap<MemoryLocation, UsageInfo, 16>;
Sema &SemaRef;
@@ -12625,7 +12699,7 @@
/// Filled in with declarations which were modified as a side-effect
/// (that is, post-increment operations).
- SmallVectorImpl<std::pair<Object, Usage>> *ModAsSideEffect = nullptr;
+ SmallVectorImpl<std::pair<MemoryLocation, Usage>> *ModAsSideEffect = nullptr;
/// Expressions to check later. We defer checking these to reduce
/// stack usage.
@@ -12643,7 +12717,8 @@
}
~SequencedSubexpression() {
- for (const std::pair<Object, Usage> &M : llvm::reverse(ModAsSideEffect)) {
+ for (const std::pair<MemoryLocation, 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).
@@ -12656,8 +12731,8 @@
}
SequenceChecker &Self;
- SmallVector<std::pair<Object, Usage>, 4> ModAsSideEffect;
- SmallVectorImpl<std::pair<Object, Usage>> *OldModAsSideEffect;
+ SmallVector<std::pair<MemoryLocation, Usage>, 4> ModAsSideEffect;
+ SmallVectorImpl<std::pair<MemoryLocation, Usage>> *OldModAsSideEffect;
};
/// RAII object wrapping the visitation of a subexpression which we might
@@ -12691,32 +12766,105 @@
bool EvalOK = true;
} *EvalTracker = nullptr;
- /// Find the object which is produced by the specified expression,
- /// if any.
- Object getObject(const Expr *E, bool Mod) const {
+ /// Find the memory location which is produced by the specified
+ /// expression \p E if any.
+ static MemoryLocation getMemoryLocation(const Expr *E, bool Mod) {
+ return getMemoryLocationImpl(E, Mod, /*RefsSeenPtr=*/nullptr);
+ }
+
+ /// Implementation of \p getMemoryLocation.
+ /// \p RefsSeenPtr is used to avoid reference cycles. When such a cycle
+ /// is possible we check first if \p RefsSeenPtr is non-null. If it is
+ /// non-null we use the pointed \p SmallPtrSet and if null we create one on
+ /// the stack. This allow us to avoid creating the \p SmallPtrSet when we
+ /// don't strictly have to.
+ //
+ // FIXME: Is this duplicating some code which already exists somewhere else ?
+ // If not, should this be moved somewhere where it can be reused ?
+ static MemoryLocation
+ getMemoryLocationImpl(const Expr *E, bool Mod,
+ llvm::SmallPtrSetImpl<const VarDecl *> *RefsSeenPtr) {
E = E->IgnoreParenCasts();
- if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
+ if (const auto *UO = dyn_cast<UnaryOperator>(E)) {
if (Mod && (UO->getOpcode() == UO_PreInc || UO->getOpcode() == UO_PreDec))
- return getObject(UO->getSubExpr(), Mod);
- } else if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+ return getMemoryLocationImpl(UO->getSubExpr(), Mod, RefsSeenPtr);
+ }
+
+ else if (const auto *BO = dyn_cast<BinaryOperator>(E)) {
if (BO->getOpcode() == BO_Comma)
- return getObject(BO->getRHS(), Mod);
+ return getMemoryLocationImpl(BO->getRHS(), Mod, RefsSeenPtr);
if (Mod && BO->isAssignmentOp())
- return getObject(BO->getLHS(), Mod);
- } 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 (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
- // FIXME: If this is a reference, map through to its value.
- return DRE->getDecl();
- return nullptr;
- }
+ return getMemoryLocationImpl(BO->getLHS(), Mod, RefsSeenPtr);
+ }
- /// 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) {
+ else if (const auto *ME = dyn_cast<MemberExpr>(E)) {
+ if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl()))
+ return MemoryLocation(
+ /*Base=*/getMemoryLocationImpl(ME->getBase(), Mod, RefsSeenPtr)
+ .getOpaqueObject(),
+ /*Field=*/FD);
+
+ if (const auto *VD = dyn_cast<VarDecl>(ME->getMemberDecl())) {
+ if (!RefsSeenPtr) {
+ llvm::SmallPtrSet<const VarDecl *, 1> RefsSeen;
+ return maybeLookThroughRefs(VD, Mod, RefsSeen);
+ } else {
+ return maybeLookThroughRefs(VD, Mod, *RefsSeenPtr);
+ }
+ }
+ }
+
+ else if (const auto *CXXTE = dyn_cast<CXXThisExpr>(E)) {
+ return MemoryLocation(MemoryLocation::CXXThisTag(), /*Field=*/nullptr);
+ }
+
+ else if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+ const ValueDecl *ReferencedDecl = DRE->getDecl();
+ if (const auto *VD = dyn_cast<VarDecl>(ReferencedDecl)) {
+ if (!RefsSeenPtr) {
+ llvm::SmallPtrSet<const VarDecl *, 1> RefsSeen;
+ return maybeLookThroughRefs(VD, Mod, RefsSeen);
+ } else {
+ return maybeLookThroughRefs(VD, Mod, *RefsSeenPtr);
+ }
+ }
+
+ if (const auto *BD = dyn_cast<BindingDecl>(ReferencedDecl)) {
+ MemoryLocation MemoryLoc =
+ getMemoryLocationImpl(BD->getBinding(), Mod, RefsSeenPtr);
+ // If we found a memory location, return it. Otherwise pretend that the
+ // binding is the memory location since this is better than not finding
+ // anything.
+ return MemoryLoc ? MemoryLoc : MemoryLocation(BD, /*Field=*/nullptr);
+ }
+ }
+ return MemoryLocation();
+ }
+
+ static MemoryLocation
+ maybeLookThroughRefs(const VarDecl *VD, bool Mod,
+ llvm::SmallPtrSetImpl<const VarDecl *> &RefsSeen) {
+ MemoryLocation MemoryLoc;
+ // If this is a reference, look through it.
+ if (VD->getType()->isReferenceType() && VD->hasInit()) {
+ // But only if we don't have a cycle.
+ bool Inserted = RefsSeen.insert(VD).second;
+ if (Inserted)
+ MemoryLoc = getMemoryLocationImpl(VD->getInit(), Mod, &RefsSeen);
+ }
+ // If we found a memory location, return it. Otherwise pretend that the
+ // given VarDecl is the memory location since this is better than not
+ // finding anything.
+ return MemoryLoc ? MemoryLoc : MemoryLocation(VD, /*Field=*/nullptr);
+ }
+
+ /// Note that a memory location \p MemoryLoc was modified or used by
+ /// an expression \p UsageExpr with usage kind \p UK. \p UI is
+ /// the \p UsageInfo for the memory location \p MemoryLoc as obtained
+ /// via the \p UsageMap.
+ void addUsage(MemoryLocation MemoryLoc, UsageInfo &UI, const Expr *UsageExpr,
+ UsageKind UK) {
+ assert(MemoryLoc && "addUsage requires a valid MemoryLocation!");
// Get the old usage for the given object and usage kind.
Usage &U = UI.Uses[UK];
if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) {
@@ -12724,20 +12872,22 @@
// 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));
+ ModAsSideEffect->push_back(std::make_pair(MemoryLoc, U));
// Then record the new usage with the current sequencing region.
U.UsageExpr = UsageExpr;
U.Seq = Region;
}
}
- /// 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) {
+ /// Check whether a modification or use of a memory location \p MemoryLoc in
+ /// an expression \p UsageExpr conflicts with a prior usage of kind
+ /// \p OtherKind. \p UI is the \p UsageInfo for the memory location
+ /// \p MemoryLoc as obtained via the \p UsageMap. \p IsModMod is true when we
+ /// are checking for a mod-mod unsequenced usage and false when we are
+ /// checking for a mod-use unsequenced usage.
+ void checkUsage(MemoryLocation MemoryLoc, UsageInfo &UI,
+ const Expr *UsageExpr, UsageKind OtherKind, bool IsModMod) {
+ assert(MemoryLoc && "checkUsage requires a valid MemoryLocation!");
if (UI.Diagnosed)
return;
@@ -12750,11 +12900,65 @@
if (OtherKind == UK_Use)
std::swap(Mod, ModOrUse);
+ enum MemberDiagKind { MDK_NotMember, MDK_Member, MDK_StaticMember };
+ enum OfDiagKind {
+ ODK_NoOf,
+ ODK_Of,
+ ODK_OfStruct,
+ ODK_OfUnion,
+ ODK_OfClass,
+ ODK_OfStructuredBinding
+ };
+ const ValueDecl *Object = MemoryLoc.getObject();
+ const ValueDecl *Field = MemoryLoc.getField();
+ const TagDecl *TD = nullptr;
+
+ MemberDiagKind MemberDiag =
+ (Field != nullptr) ? MDK_Member
+ : (Object && isa<VarDecl>(Object) &&
+ cast<VarDecl>(Object)->isStaticDataMember())
+ ? MDK_StaticMember
+ : MDK_NotMember;
+ OfDiagKind OfDiag = ODK_NoOf;
+ if (MemberDiag == MDK_Member) {
+ if (Object) {
+ if (isa<DecompositionDecl>(Object))
+ OfDiag = ODK_OfStructuredBinding;
+ else
+ OfDiag = ODK_Of;
+ }
+ } else if (MemberDiag == MDK_StaticMember) {
+ TD = dyn_cast<TagDecl>(cast<VarDecl>(Object)->getDeclContext());
+ if (TD) {
+ switch (TD->getTagKind()) {
+ case TTK_Struct:
+ OfDiag = ODK_OfStruct;
+ break;
+ case TTK_Class:
+ OfDiag = ODK_OfClass;
+ break;
+ case TTK_Union:
+ OfDiag = ODK_OfUnion;
+ break;
+ default:
+ break;
+ }
+ }
+ }
+
+ const ValueDecl *FirstDiagElement =
+ MemberDiag == MDK_Member ? Field : Object;
+ const ValueDecl *SecondDiagElement =
+ MemberDiag == MDK_Member ? Object : nullptr;
+
SemaRef.DiagRuntimeBehavior(
Mod->getExprLoc(), {Mod, ModOrUse},
SemaRef.PDiag(IsModMod ? diag::warn_unsequenced_mod_mod
: diag::warn_unsequenced_mod_use)
- << O << SourceRange(ModOrUse->getExprLoc()));
+ << static_cast<unsigned>(MemberDiag)
+ << static_cast<unsigned>(OfDiag) << FirstDiagElement
+ << SecondDiagElement << TD << SourceRange(ModOrUse->getExprLoc()));
+
UI.Diagnosed = true;
}
@@ -12764,11 +12968,11 @@
// "((++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.
+ // When visiting a node which uses/modify a memory location 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
@@ -12784,31 +12988,39 @@
// modification as side effect) when exiting the scope of the sequenced
// subexpression.
- void notePreUse(Object O, const Expr *UseExpr) {
- UsageInfo &UI = UsageMap[O];
+ void notePreUse(MemoryLocation MemoryLoc, const Expr *UseExpr) {
+ assert(MemoryLoc && "notePreUse requires a valid memory location!");
+ UsageInfo &UI = UsageMap[MemoryLoc];
// Uses conflict with other modifications.
- checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/false);
+ checkUsage(MemoryLoc, UI, UseExpr, /*OtherKind=*/UK_ModAsValue,
+ /*IsModMod=*/false);
}
- void notePostUse(Object O, const Expr *UseExpr) {
- UsageInfo &UI = UsageMap[O];
- checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsSideEffect,
+ void notePostUse(MemoryLocation MemoryLoc, const Expr *UseExpr) {
+ assert(MemoryLoc && "notePostUse requires a valid memory location!");
+ UsageInfo &UI = UsageMap[MemoryLoc];
+ checkUsage(MemoryLoc, UI, UseExpr, /*OtherKind=*/UK_ModAsSideEffect,
/*IsModMod=*/false);
- addUsage(O, UI, UseExpr, /*UsageKind=*/UK_Use);
+ addUsage(MemoryLoc, UI, UseExpr, /*UsageKind=*/UK_Use);
}
- void notePreMod(Object O, const Expr *ModExpr) {
- UsageInfo &UI = UsageMap[O];
+ void notePreMod(MemoryLocation MemoryLoc, const Expr *ModExpr) {
+ assert(MemoryLoc && "notePreMod requires a valid memory location!");
+ UsageInfo &UI = UsageMap[MemoryLoc];
// Modifications conflict with other modifications and with uses.
- checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/true);
- checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_Use, /*IsModMod=*/false);
+ checkUsage(MemoryLoc, UI, ModExpr, /*OtherKind=*/UK_ModAsValue,
+ /*IsModMod=*/true);
+ checkUsage(MemoryLoc, UI, ModExpr, /*OtherKind=*/UK_Use,
+ /*IsModMod=*/false);
}
- void notePostMod(Object O, const Expr *ModExpr, UsageKind UK) {
- UsageInfo &UI = UsageMap[O];
- checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsSideEffect,
+ void notePostMod(MemoryLocation MemoryLoc, const Expr *ModExpr,
+ UsageKind UK) {
+ assert(MemoryLoc && "notePostMod requires a valid memory location!");
+ UsageInfo &UI = UsageMap[MemoryLoc];
+ checkUsage(MemoryLoc, UI, ModExpr, /*OtherKind=*/UK_ModAsSideEffect,
/*IsModMod=*/true);
- addUsage(O, UI, ModExpr, /*UsageKind=*/UK);
+ addUsage(MemoryLoc, UI, ModExpr, /*UsageKind=*/UK);
}
public:
@@ -12831,15 +13043,17 @@
}
void VisitCastExpr(const CastExpr *E) {
- Object O = Object();
+ MemoryLocation MemoryLoc{};
if (E->getCastKind() == CK_LValueToRValue)
- O = getObject(E->getSubExpr(), false);
+ MemoryLoc = getMemoryLocation(E->getSubExpr(), /*Mod=*/false);
+
+ if (MemoryLoc)
+ notePreUse(MemoryLoc, E);
- if (O)
- notePreUse(O, E);
VisitExpr(E);
- if (O)
- notePostUse(O, E);
+
+ if (MemoryLoc)
+ notePostUse(MemoryLoc, E);
}
void VisitSequencedExpressions(const Expr *SequencedBefore,
@@ -12928,9 +13142,9 @@
//
// so check it before inspecting the operands and update the
// map afterwards.
- Object O = getObject(BO->getLHS(), /*Mod=*/true);
- if (O)
- notePreMod(O, BO);
+ MemoryLocation MemoryLoc = getMemoryLocation(BO->getLHS(), /*Mod=*/true);
+ if (MemoryLoc)
+ notePreMod(MemoryLoc, BO);
if (SemaRef.getLangOpts().CPlusPlus17) {
// C++17 [expr.ass]p1:
@@ -12944,16 +13158,16 @@
Region = LHSRegion;
Visit(BO->getLHS());
- if (O && isa<CompoundAssignOperator>(BO))
- notePostUse(O, BO);
+ if (MemoryLoc && isa<CompoundAssignOperator>(BO))
+ notePostUse(MemoryLoc, BO);
} else {
// C++11 does not specify any sequencing between the LHS and RHS.
Region = LHSRegion;
Visit(BO->getLHS());
- if (O && isa<CompoundAssignOperator>(BO))
- notePostUse(O, BO);
+ if (MemoryLoc && isa<CompoundAssignOperator>(BO))
+ notePostUse(MemoryLoc, BO);
Region = RHSRegion;
Visit(BO->getRHS());
@@ -12964,8 +13178,8 @@
// assignment expression.
// C11 6.5.16/3 has no such rule.
Region = OldRegion;
- if (O)
- notePostMod(O, BO,
+ if (MemoryLoc)
+ notePostMod(MemoryLoc, BO,
SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
: UK_ModAsSideEffect);
if (SemaRef.getLangOpts().CPlusPlus17) {
@@ -12981,15 +13195,16 @@
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)
+ MemoryLocation MemoryLoc =
+ getMemoryLocation(UO->getSubExpr(), /*Mod=*/true);
+ if (!MemoryLoc)
return VisitExpr(UO);
- notePreMod(O, UO);
+ notePreMod(MemoryLoc, UO);
Visit(UO->getSubExpr());
// C++11 [expr.pre.incr]p1:
// the expression ++x is equivalent to x+=1
- notePostMod(O, UO,
+ notePostMod(MemoryLoc, UO,
SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
: UK_ModAsSideEffect);
}
@@ -12997,13 +13212,14 @@
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)
+ MemoryLocation MemoryLoc =
+ getMemoryLocation(UO->getSubExpr(), /*Mod=*/true);
+ if (!MemoryLoc)
return VisitExpr(UO);
- notePreMod(O, UO);
+ notePreMod(MemoryLoc, UO);
Visit(UO->getSubExpr());
- notePostMod(O, UO, UK_ModAsSideEffect);
+ notePostMod(MemoryLoc, UO, UK_ModAsSideEffect);
}
void VisitBinLOr(const BinaryOperator *BO) {
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2027,9 +2027,15 @@
"parenthesized initializer list">;
def warn_unsequenced_mod_mod : Warning<
- "multiple unsequenced modifications to %0">, InGroup<Unsequenced>;
+ "multiple unsequenced modifications to "
+ "%select{|member |static member }0%2"
+ "%select{| of %3| of struct %4| of union %4| of class %4"
+ "| of structured binding %3}1">, InGroup<Unsequenced>;
def warn_unsequenced_mod_use : Warning<
- "unsequenced modification and access to %0">, InGroup<Unsequenced>;
+ "unsequenced modification and access to "
+ "%select{|member |static member }0%2"
+ "%select{| of %3| of struct %4| of union %4| of class %4"
+ "| of structured binding %3}1">, InGroup<Unsequenced>;
def select_initialized_entity_kind : TextSubstitution<
"%select{copying variable|copying parameter|"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
