riccibruno updated this revision to Diff 271464.
riccibruno retitled this revision from "[clang] SequenceChecker: Also visit
default arguments." to "[clang] SequenceChecker: Also visit default arguments
and default initializers.".
riccibruno edited the summary of this revision.
riccibruno added a comment.
Ah, I forgot about default initializers. Thanks!
I have updated the patch to track chains of default arguments and/or
initializers.
This shouldn't be too expensive since this will only be used when traversing a
`CXXDefaultArgExpr` or `CXXDefaultInitExpr`. Still I am running my usual
benchmark
(all of boost) and will report with the results.
I think that this uncovers another issue (`test_default_init` in the test):
`SequenceChecker::VisitCXXConstructExpr` does not visit the default arguments
of the constructor.
Because of this `SequenceChecker` misses the mutation in the example you gave,
but only in <c++17.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81003/new/
https://reviews.llvm.org/D81003
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
@@ -268,6 +268,118 @@
// cxx17-warning@-1 {{unsequenced modification and access to 'pf'}}
}
+namespace default_arg {
+ int a;
+ void f1(int = a, int = a++); // cxx11-warning 2{{unsequenced modification and access to 'a'}}
+ // cxx17-warning@-1 2{{unsequenced modification and access to 'a'}}
+
+ void f2(int = a++, int = a); // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+
+ void f3(int = a++, int = sizeof(a));
+
+ void test() {
+ int b;
+ f1(); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+ f1(a); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+ f1(a,a); // ok
+ f1(b); // ok
+ f1(b,a++); // ok
+
+ f2(); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+ f2(a); // ok
+ f2(a++); // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+
+ f3(); // ok
+ f3(a++); // ok
+ }
+}
+
+namespace default_init {
+ int a;
+ struct X { int b = ++a; };
+ // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+ void test_default_init() {
+ // FIXME: In C++17 the temporary is represented by
+ // a MaterializeTemporaryExpr wrapping a CXXFunctionalCastExpr.
+ //
+ // In C++11 the temporary is represented by a MaterializeTemporaryExpr
+ // wrapping a CXXTemporaryObjectExpr.
+ //
+ // However SequenceChecker::VisitCXXConstructExpr only visit the arguments
+ // of the CXXConstructExpr and is not visiting the arguments of
+ // the constructor.
+ int c = X{}.b + a;
+ // cxx17-note@-1 {{in default initializer used here}}
+ }
+}
+
+namespace default_arg_chain_0 {
+ int i;
+ int f0(int = i);
+ int f1(int = f0());
+ int f2(int = f1());
+ int f3(int, int = f2());
+
+ void test() {
+ f3(i); // ok
+ f3(i++); // cxx11-warning {{unsequenced modification and access to 'i'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'i'}}
+ }
+}
+
+namespace default_arg_chain_1 {
+ int i;
+ int f0(int = i++); // cxx11-warning {{unsequenced modification and access to 'i'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'i'}}
+
+ int f1(int = f0()); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+
+ int f2(int = f1()); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+
+ int f3(int, int = f2()); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+
+ void test() {
+ f3(0); // ok
+ f3(i); // cxx11-note {{in default argument used here}}
+ // cxx17-note@-1 {{in default argument used here}}
+ }
+}
+
+namespace default_arg_init_chain {
+ int i;
+ struct X { int b = ++i; }; // cxx17-warning 4{{unsequenced modification and access to 'i'}}
+
+ // FIXME: see note above in default_init.
+
+ int g0(int = X{}.b); // cxx17-note 4{{in default initializer used here}}
+ int g1(int = g0()); // cxx17-note 4{{in default argument used here}}
+ int g2(int = g1()); // cxx17-note 4{{in default argument used here}}
+ int g3(int = g2()); // cxx17-note 4{{in default argument used here}}
+
+ int g4a(int, int = g3() + i); // cxx17-note {{in default argument used here}}
+ int g4b(int, int = i + g3()); // cxx17-note {{in default argument used here}}
+
+ int g5a(int = 0, int = g3()); // cxx17-note {{in default argument used here}}
+ int g5b(int = g3(), int = 0);
+
+ void test() {
+ g5a(); // ok
+ g5b(); // ok
+ g5a(i); // cxx17-note {{in default argument used here}}
+ g5b(g3()); // ok
+ g5b(g3(), i); // cxx17-note {{in default argument used here}}
+ }
+}
+
+
namespace PR20819 {
struct foo { void bar(int); };
foo get_foo(int);
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -12434,9 +12434,28 @@
const Expr *UsageExpr;
SequenceTree::Seq Seq;
- Usage() : UsageExpr(nullptr), Seq() {}
+ /// The index in \p AdditionalSLData for the additional \p SourceLocation
+ /// data if >= 0, or -1 if there is no additional data (the common case).
+ int AdditionalSLDataIndex;
+
+ Usage() : UsageExpr(nullptr), Seq(), AdditionalSLDataIndex(-1) {}
+ };
+
+ /// A structure storing extra location data. These structures
+ /// form a chain in \p AdditionalSLData.
+ struct AdditionalSourceLocationData {
+ SourceLocation SL;
+ unsigned DiagID;
+ int PrevIndex;
+ AdditionalSourceLocationData(SourceLocation SL, unsigned DiagID,
+ int PrevIndex)
+ : SL(SL), DiagID(DiagID), PrevIndex(PrevIndex) {}
};
+ /// One \p AdditionalSourceLocationData is added to this vector
+ /// for each \p CXXDefaultArgExpr or \p CXXDefaultInitExpr.
+ SmallVector<AdditionalSourceLocationData, 2> AdditionalSLData;
+
struct UsageInfo {
Usage Uses[UK_Count];
@@ -12526,6 +12545,36 @@
bool EvalOK = true;
} *EvalTracker = nullptr;
+ /// RAII object used to wrap the visitation of an expression with additional
+ /// source location data. This is used to keep track of chains of default
+ /// argument and initializer.
+ class AdditionalSourceLocationTracker {
+ public:
+ AdditionalSourceLocationTracker(SequenceChecker &Self, SourceLocation SL,
+ unsigned DiagID)
+ : Self(Self), Prev(Self.AdditionalSLTracker), SL(SL), DiagID(DiagID),
+ AdditionalSLDataIndex(-1) {
+ Self.AdditionalSLTracker = this;
+ Self.AdditionalSLData.emplace_back(
+ SL, DiagID, Prev ? Prev->AdditionalSLDataIndex : -1);
+ AdditionalSLDataIndex = Self.AdditionalSLData.size() - 1;
+ }
+
+ ~AdditionalSourceLocationTracker() { Self.AdditionalSLTracker = Prev; }
+
+ AdditionalSourceLocationTracker *getPrev() const { return Prev; }
+ SourceLocation getLoc() const { return SL; }
+ unsigned getDiagID() const { return DiagID; }
+ int getAdditionalSLDataIndex() const { return AdditionalSLDataIndex; }
+
+ private:
+ SequenceChecker &Self;
+ AdditionalSourceLocationTracker *Prev;
+ SourceLocation SL;
+ unsigned DiagID;
+ int AdditionalSLDataIndex;
+ } *AdditionalSLTracker = nullptr;
+
/// Find the object which is produced by the specified expression,
/// if any.
Object getObject(const Expr *E, bool Mod) const {
@@ -12563,6 +12612,12 @@
// Then record the new usage with the current sequencing region.
U.UsageExpr = UsageExpr;
U.Seq = Region;
+
+ // If we have additional source location data, store the head of
+ // of the location data chain in \p U.
+ if (AdditionalSLTracker && AdditionalSLTracker->getLoc().isValid())
+ U.AdditionalSLDataIndex =
+ AdditionalSLTracker->getAdditionalSLDataIndex();
}
}
@@ -12590,6 +12645,26 @@
SemaRef.PDiag(IsModMod ? diag::warn_unsequenced_mod_mod
: diag::warn_unsequenced_mod_use)
<< O << SourceRange(ModOrUse->getExprLoc()));
+
+ if (OtherKind != UK_Use) {
+ // Walk back the stored chain.
+ int Index = U.AdditionalSLDataIndex;
+ while (Index != -1) {
+ const AdditionalSourceLocationData &ASLD = AdditionalSLData[Index];
+ SemaRef.DiagRuntimeBehavior(ASLD.SL, {Mod, ModOrUse},
+ SemaRef.PDiag(ASLD.DiagID));
+ Index = ASLD.PrevIndex;
+ }
+ } else {
+ // walk back the current chain.
+ const AdditionalSourceLocationTracker *SLTracker = AdditionalSLTracker;
+ while (SLTracker && SLTracker->getLoc().isValid()) {
+ SemaRef.DiagRuntimeBehavior(SLTracker->getLoc(), {Mod, ModOrUse},
+ SemaRef.PDiag(SLTracker->getDiagID()));
+ SLTracker = SLTracker->getPrev();
+ }
+ }
+
UI.Diagnosed = true;
}
@@ -13013,6 +13088,22 @@
});
}
+ void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *DIE) {
+ // Remember that we are visiting this default initializer.
+ AdditionalSourceLocationTracker SL(
+ *this, DIE->getUsedLocation(),
+ diag::note_in_default_initializer_used_here);
+ Visit(DIE->getExpr());
+ }
+
+ void VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *DAE) {
+ // Remember that we are visiting this default argument.
+ AdditionalSourceLocationTracker SL(
+ *this, DAE->getUsedLocation(),
+ diag::note_in_default_argument_used_here);
+ Visit(DAE->getExpr());
+ }
+
void VisitCXXConstructExpr(const CXXConstructExpr *CCE) {
// This is a call, so all subexpressions are sequenced before the result.
SequencedSubexpression Sequenced(*this);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2119,6 +2119,10 @@
"multiple unsequenced modifications to %0">, InGroup<Unsequenced>;
def warn_unsequenced_mod_use : Warning<
"unsequenced modification and access to %0">, InGroup<Unsequenced>;
+def note_in_default_argument_used_here : Note<
+ "in default argument used here">;
+def note_in_default_initializer_used_here : Note<
+ "in default initializer used here">;
def select_initialized_entity_kind : TextSubstitution<
"%select{copying variable|copying parameter|"
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
