Index: clang/docs/ReleaseNotes.rst =================================================================== --- clang/docs/ReleaseNotes.rst +++ clang/docs/ReleaseNotes.rst @@ -453,6 +453,42 @@ - Clang now emits ``-Wconstant-logical-operand`` warning even when constant logical operand is on left side. (`#37919 `_) +- Added a new warning group ``-Windeterminately-sequenced`` to diagnose indeterminately + sequenced accesses amongst the initializations of each function parameter, as is the + case from C++17 onwards. Accordingly, ``-Wunsequenced`` no longer falsely diagnoses + such accesses as unsequenced. + (`#63935 `_) + + *Example C++23 Code*: + + .. code-block:: c++ + + struct S { int operator[](auto...); }; + int c{}, d{}, f(int, int = {}), + a = f(c++, (++d, f)(!c, !d)), + b = S()[f(f(c++)), c++]; + + *BEFORE*: + + .. code-block:: text + + :3:12: warning: unsequenced modification and access to 'c' [-Wunsequenced] + 3 | a = f(c++, (++d, f)(!c, !d)), + | ^ ~ + :4:18: warning: multiple unsequenced modifications to 'c' [-Wunsequenced] + 4 | b = S()[f(f(c++)), c++]; + | ^ ~~ + + *AFTER*: + + .. code-block:: text + + :3:12: warning: indeterminately sequenced modification and access to 'c' [-Windeterminately-sequenced] + 3 | a = f(c++, (++d, f)(!c, !d)), + | ^ ~ + :4:18: warning: multiple indeterminately sequenced modifications to 'c' [-Windeterminately-sequenced] + 4 | b = S()[f(f(c++)), c++]; + | ^ ~~ Bug Fixes in This Version ------------------------- Index: clang/include/clang/Basic/DiagnosticGroups.td =================================================================== --- clang/include/clang/Basic/DiagnosticGroups.td +++ clang/include/clang/Basic/DiagnosticGroups.td @@ -688,6 +688,7 @@ def XorUsedAsPow : DiagGroup<"xor-used-as-pow">; def Unsequenced : DiagGroup<"unsequenced">; +def IndeterminatelySequenced : DiagGroup<"indeterminately-sequenced">; // GCC name for -Wunsequenced def : DiagGroup<"sequence-point", [Unsequenced]>; Index: clang/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- clang/include/clang/Basic/DiagnosticSemaKinds.td +++ clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2291,6 +2291,12 @@ "multiple unsequenced modifications to %0">, InGroup; def warn_unsequenced_mod_use : Warning< "unsequenced modification and access to %0">, InGroup; +def warn_indeterminately_sequenced_mod_mod : Warning< + "multiple indeterminately sequenced modifications to %0">, + InGroup; +def warn_indeterminately_sequenced_mod_use : Warning< + "indeterminately sequenced modification and access to %0">, + InGroup; def select_initialized_entity_kind : TextSubstitution< "%select{copying variable|copying parameter|initializing template parameter|" Index: clang/lib/Sema/SemaChecking.cpp =================================================================== --- clang/lib/Sema/SemaChecking.cpp +++ clang/lib/Sema/SemaChecking.cpp @@ -15692,6 +15692,8 @@ }; SmallVector Values; + const SequenceChecker &SC; + public: /// A region within an expression which may be sequenced with respect /// to some other region. @@ -15706,7 +15708,9 @@ Seq() : Index(0) {} }; - SequenceTree() { Values.push_back(Value(0)); } + SequenceTree(const SequenceChecker &SC) : SC(SC) { + Values.push_back(Value(0)); + } Seq root() const { return Seq(0); } /// Create a new sequence of operations, which is an unsequenced @@ -15736,6 +15740,29 @@ return false; } + /// Determine whether two operations are indeterminately sequenced. This + /// operation is asymmetric: \p Cur should be the more recent sequence, and + /// \p Old should have been merged into its parent as appropriate. + bool isIndeterminatelySequenced(Seq Cur, Seq Old) { + unsigned C = representative(Cur.Index); + unsigned Target = representative(Old.Index); + for (Seq seq : llvm::iterator_range(SC.CallPostfixExprs.rbegin(), + SC.CallPostfixExprs.rend())) { + unsigned Sibling = seq.Index; + unsigned Parent = Values[Sibling].Parent; + if (Target <= Sibling) + continue; + if ((Target = Values[Target].Parent) != Parent) + continue; + while (C >= Target) { + if (C == Target) + return true; + C = Values[C].Parent; + } + } + return false; + } + private: /// Pick a representative for a sequence. unsigned representative(unsigned K) { @@ -15788,7 +15815,7 @@ Sema &SemaRef; /// Sequenced regions within the expression. - SequenceTree Tree; + SequenceTree Tree = *this; /// Declaration modifications and references which we have seen. UsageInfoMap UsageMap; @@ -15800,10 +15827,6 @@ /// (that is, post-increment operations). SmallVectorImpl> *ModAsSideEffect = nullptr; - /// Expressions to check later. We defer checking these to reduce - /// stack usage. - SmallVectorImpl &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 /// become sequenced with respect to the value computation of the result, so @@ -15864,6 +15887,22 @@ bool EvalOK = true; } *EvalTracker = nullptr; + /// RAII object tracking the visitation of CallExpr and that of its + /// subexpressions. + struct InCallExprArgs { + InCallExprArgs(SequenceChecker &SC) : SC(SC) { + SC.CallPostfixExprs.push_back(SC.Region); + }; + + ~InCallExprArgs() { SC.CallPostfixExprs.pop_back(); } + + private: + SequenceChecker &SC; + }; + + /// Only modified by InCallExprArgs + SmallVector CallPostfixExprs; + /// Find the object which is produced by the specified expression, /// if any. Object getObject(const Expr *E, bool Mod) const { @@ -15892,7 +15931,11 @@ 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.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) { + bool FoundUnsequenced = U.UsageExpr && Tree.isUnsequenced(Region, U.Seq); + bool FoundIndeterminatelySequenced = + U.UsageExpr && SemaRef.LangOpts.CPlusPlus17 && + Tree.isIndeterminatelySequenced(Region, U.Seq); + if (!FoundUnsequenced && !FoundIndeterminatelySequenced) { // 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. @@ -15915,7 +15958,11 @@ return; const Usage &U = UI.Uses[OtherKind]; - if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) + bool FoundUnsequenced = U.UsageExpr && Tree.isUnsequenced(Region, U.Seq); + bool FoundIndeterminatelySequenced = + U.UsageExpr && SemaRef.LangOpts.CPlusPlus17 && + Tree.isIndeterminatelySequenced(Region, U.Seq); + if (!FoundUnsequenced && !FoundIndeterminatelySequenced) return; const Expr *Mod = U.UsageExpr; @@ -15925,8 +15972,12 @@ SemaRef.DiagRuntimeBehavior( Mod->getExprLoc(), {Mod, ModOrUse}, - SemaRef.PDiag(IsModMod ? diag::warn_unsequenced_mod_mod - : diag::warn_unsequenced_mod_use) + SemaRef.PDiag(!FoundUnsequenced && FoundIndeterminatelySequenced + ? IsModMod + ? diag::warn_indeterminately_sequenced_mod_mod + : diag::warn_indeterminately_sequenced_mod_use + : IsModMod ? diag::warn_unsequenced_mod_mod + : diag::warn_unsequenced_mod_use) << O << SourceRange(ModOrUse->getExprLoc())); UI.Diagnosed = true; } @@ -15985,13 +16036,9 @@ } public: - SequenceChecker(Sema &S, const Expr *E, - SmallVectorImpl &WorkList) - : Base(S.Context), SemaRef(S), Region(Tree.root()), WorkList(WorkList) { + SequenceChecker(Sema &S, const Expr *E) + : Base(S.Context), SemaRef(S), Region(Tree.root()) { Visit(E); - // Silence a -Wunused-private-field since WorkList is now unused. - // TODO: Evaluate if it can be used, and if not remove it. - (void)this->WorkList; } void VisitStmt(const Stmt *S) { @@ -16332,43 +16379,48 @@ // the value computation of its result]. SequencedSubexpression Sequenced(*this); SemaRef.runWithSufficientStackSpace(CE->getExprLoc(), [&] { + if (!SemaRef.getLangOpts().CPlusPlus17) + return Base::VisitCallExpr(CE); + // C++17 [expr.call]p5 // The postfix-expression is sequenced before each expression in the // expression-list and any default argument. [...] - SequenceTree::Seq CalleeRegion; - SequenceTree::Seq OtherRegion; - if (SemaRef.getLangOpts().CPlusPlus17) { - CalleeRegion = Tree.allocate(Region); - OtherRegion = Tree.allocate(Region); - } else { - CalleeRegion = Region; - OtherRegion = Region; - } - SequenceTree::Seq OldRegion = Region; - - // Visit the callee expression first. + SequenceTree::Seq Parent = Region; + SequenceTree::Seq CalleeRegion = Tree.allocate(Parent); Region = CalleeRegion; - if (SemaRef.getLangOpts().CPlusPlus17) { + { SequencedSubexpression Sequenced(*this); - Visit(CE->getCallee()); - } else { - Visit(CE->getCallee()); + Visit(isa(CE) ? CE->getArg(0) : CE->getCallee()); } - // Then visit the argument expressions. - Region = OtherRegion; - for (const Expr *Argument : CE->arguments()) + // C++17 [expr.call]p5 + // [...] The initialization of a parameter, including every associated + // value computation and side effect, is indeterminately sequenced with + // respect to that of any other parameter. + SmallVector Args; + auto ArgRange = + isa(CE) + ? llvm::iterator_range(CE->arg_begin() + 1, CE->arg_end()) + : CE->arguments(); + InCallExprArgs _(*this); + for (const Expr *Argument : ArgRange) { + SequencedSubexpression Sequenced(*this); + Region = Tree.allocate(Parent); + Args.push_back(Region); Visit(Argument); - - Region = OldRegion; - if (SemaRef.getLangOpts().CPlusPlus17) { - Tree.merge(CalleeRegion); - Tree.merge(OtherRegion); } + + Region = Parent; + Tree.merge(CalleeRegion); + for (SequenceTree::Seq region : Args) + Tree.merge(region); }); } void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *CXXOCE) { + if (!SemaRef.getLangOpts().CPlusPlus17) + return VisitCallExpr(CXXOCE); + // C++17 [over.match.oper]p2: // [...] the operator notation is first transformed to the equivalent // function-call notation as summarized in Table 12 (where @ denotes one @@ -16379,15 +16431,11 @@ // From the above only overloaded binary operators and overloaded call // operators have sequencing rules in C++17 that we need to handle // separately. - if (!SemaRef.getLangOpts().CPlusPlus17 || - (CXXOCE->getNumArgs() != 2 && CXXOCE->getOperator() != OO_Call)) - return VisitCallExpr(CXXOCE); - enum { NoSequencing, LHSBeforeRHS, RHSBeforeLHS, - LHSBeforeRest + IndeterminatelySequenced } SequencingKind; switch (CXXOCE->getOperator()) { case OO_Equal: @@ -16403,28 +16451,49 @@ case OO_GreaterGreaterEqual: SequencingKind = RHSBeforeLHS; break; - case OO_LessLess: case OO_GreaterGreater: case OO_AmpAmp: case OO_PipePipe: case OO_Comma: case OO_ArrowStar: - case OO_Subscript: SequencingKind = LHSBeforeRHS; break; - + case OO_Subscript: + SequencingKind = SemaRef.getLangOpts().CPlusPlus23 + ? IndeterminatelySequenced // CWG2571 + : LHSBeforeRHS; + break; case OO_Call: - SequencingKind = LHSBeforeRest; + SequencingKind = IndeterminatelySequenced; break; - default: SequencingKind = NoSequencing; break; } - if (SequencingKind == NoSequencing) - return VisitCallExpr(CXXOCE); + if (SequencingKind == NoSequencing) { + SequenceTree::Seq Parent = Region; + SequenceTree::Seq CalleeRegion = Tree.allocate(Parent); + SequenceTree::Seq OtherRegion = Tree.allocate(Parent); + + // Visit the callee expression first. + Region = CalleeRegion; + { + SequencedSubexpression Sequenced(*this); + Visit(CXXOCE->getCallee()); + } + + // Then visit the argument expressions. + Region = OtherRegion; + for (const Expr *Argument : CXXOCE->arguments()) + Visit(Argument); + + Region = Parent; + Tree.merge(CalleeRegion); + Tree.merge(OtherRegion); + return; + } // This is a call, so all subexpressions are sequenced before the result. SequencedSubexpression Sequenced(*this); @@ -16432,63 +16501,31 @@ SemaRef.runWithSufficientStackSpace(CXXOCE->getExprLoc(), [&] { assert(SemaRef.getLangOpts().CPlusPlus17 && "Should only get there with C++17 and above!"); - assert((CXXOCE->getNumArgs() == 2 || CXXOCE->getOperator() == OO_Call) && - "Should only get there with an overloaded binary operator" - " or an overloaded call operator!"); - - if (SequencingKind == LHSBeforeRest) { - assert(CXXOCE->getOperator() == OO_Call && - "We should only have an overloaded call operator here!"); - - // This is very similar to VisitCallExpr, except that we only have the - // C++17 case. The postfix-expression is the first argument of the - // CXXOperatorCallExpr. The expressions in the expression-list, if any, - // are in the following arguments. - // - // Note that we intentionally do not visit the callee expression since - // it is just a decayed reference to a function. - SequenceTree::Seq PostfixExprRegion = Tree.allocate(Region); - SequenceTree::Seq ArgsRegion = Tree.allocate(Region); - SequenceTree::Seq OldRegion = Region; + if (SequencingKind == IndeterminatelySequenced) { + assert((CXXOCE->getOperator() == OO_Subscript || + CXXOCE->getOperator() == OO_Call) && + "We should only have an overloaded call or subscript operator " + "here!"); assert(CXXOCE->getNumArgs() >= 1 && - "An overloaded call operator must have at least one argument" - " for the postfix-expression!"); - const Expr *PostfixExpr = CXXOCE->getArgs()[0]; - llvm::ArrayRef Args(CXXOCE->getArgs() + 1, - CXXOCE->getNumArgs() - 1); - - // Visit the postfix-expression first. - { - Region = PostfixExprRegion; - SequencedSubexpression Sequenced(*this); - Visit(PostfixExpr); - } - - // Then visit the argument expressions. - Region = ArgsRegion; - for (const Expr *Arg : Args) - Visit(Arg); - - Region = OldRegion; - Tree.merge(PostfixExprRegion); - Tree.merge(ArgsRegion); - } else { - assert(CXXOCE->getNumArgs() == 2 && - "Should only have two arguments here!"); - assert((SequencingKind == LHSBeforeRHS || - SequencingKind == RHSBeforeLHS) && - "Unexpected sequencing kind!"); - - // We do not visit the callee expression since it is just a decayed - // reference to a function. - const Expr *E1 = CXXOCE->getArg(0); - const Expr *E2 = CXXOCE->getArg(1); - if (SequencingKind == RHSBeforeLHS) - std::swap(E1, E2); - - return VisitSequencedExpressions(E1, E2); + "An overloaded call or subscript operator must have at least " + "one argument for the postfix-expression!"); + return VisitCallExpr(CXXOCE); } + assert(CXXOCE->getNumArgs() == 2 && + "Should only have two arguments here!"); + assert( + (SequencingKind == LHSBeforeRHS || SequencingKind == RHSBeforeLHS) && + "Unexpected sequencing kind!"); + + // We do not visit the callee expression since it is just a decayed + // reference to a function. + const Expr *E1 = CXXOCE->getArg(0); + const Expr *E2 = CXXOCE->getArg(1); + if (SequencingKind == RHSBeforeLHS) + std::swap(E1, E2); + + return VisitSequencedExpressions(E1, E2); }); } @@ -16542,12 +16579,7 @@ } // namespace void Sema::CheckUnsequencedOperations(const Expr *E) { - SmallVector WorkList; - WorkList.push_back(E); - while (!WorkList.empty()) { - const Expr *Item = WorkList.pop_back_val(); - SequenceChecker(*this, Item, WorkList); - } + SequenceChecker(*this, E); } void Sema::CheckCompletedExpr(Expr *E, SourceLocation CheckLoc, Index: clang/test/SemaCXX/warn-unsequenced.cpp =================================================================== --- clang/test/SemaCXX/warn-unsequenced.cpp +++ clang/test/SemaCXX/warn-unsequenced.cpp @@ -1,7 +1,7 @@ // RUN: %clang_cc1 -fsyntax-only -verify=cxx11 -std=c++11 -Wno-unused -Wno-uninitialized \ // RUN: -Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s -// RUN: %clang_cc1 -fsyntax-only -verify=cxx17 -std=c++17 -Wno-unused -Wno-uninitialized \ -// RUN: -Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s +// RUN: %clang_cc1 -fsyntax-only -verify=cxx17 -std=c++23 -Wno-unused -Wno-uninitialized \ +// RUN: -Wunsequenced %s int f(int, int = 0); int g1(); @@ -40,15 +40,15 @@ a = (a++, a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}} f(a, a); // ok f(a = 0, a); // cxx11-warning {{unsequenced modification and access to 'a'}} - // cxx17-warning@-1 {{unsequenced modification and access to 'a'}} + // cxx17-warning@-1 {{indeterminately sequenced modification and access to 'a'}} f(a, a += 0); // cxx11-warning {{unsequenced modification and access to 'a'}} - // cxx17-warning@-1 {{unsequenced modification and access to 'a'}} + // cxx17-warning@-1 {{indeterminately sequenced modification and access to 'a'}} f(a = 0, a = 0); // cxx11-warning {{multiple unsequenced modifications to 'a'}} - // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}} + // cxx17-warning@-1 {{multiple indeterminately sequenced modifications to 'a'}} a = f(++a); // ok a = f(a++); // ok a = f(++a, a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}} - // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}} + // cxx17-warning@-1 {{multiple indeterminately sequenced modifications to 'a'}} // Compound assignment "A OP= B" is equivalent to "A = A OP B" except that A // is evaluated only once. @@ -279,15 +279,18 @@ // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} (i++, f)(i++, 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}} + f((++i, f)(i++, j++), ++j); // cxx11-warning {{multiple unsequenced modifications to 'i'}} + // cxx11-warning@-1 {{multiple unsequenced modifications to 'j'}} + // cxx17-warning@-2 {{multiple indeterminately sequenced modifications to 'j'}} (i++ + i++, f)(42, 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}} // cxx17-warning@-1 {{multiple unsequenced modifications to 'i'}} int (*pf)(int, int); (pf = f)(pf != nullptr, pf != nullptr); // cxx11-warning {{unsequenced modification and access to 'pf'}} pf((pf = f) != nullptr, 42); // cxx11-warning {{unsequenced modification and access to 'pf'}} f((pf = f, 42), (pf = f, 42)); // cxx11-warning {{multiple unsequenced modifications to 'pf'}} - // cxx17-warning@-1 {{multiple unsequenced modifications to 'pf'}} + // cxx17-warning@-1 {{multiple indeterminately sequenced modifications to 'pf'}} pf((pf = f) != nullptr, pf == nullptr); // cxx11-warning {{unsequenced modification and access to 'pf'}} - // cxx17-warning@-1 {{unsequenced modification and access to 'pf'}} + // cxx17-warning@-1 {{indeterminately sequenced modification and access to 'pf'}} } namespace PR20819 { @@ -305,7 +308,11 @@ E &operator=(E &); E operator()(E); E operator()(E, E); +#if __cplusplus >= 202302L + E operator[](auto...); +#else E operator[](E); +#endif } e; // Binary operators with unsequenced operands. E operator+(E,E); @@ -417,7 +424,7 @@ operator+=(((void)i++,e), ((void)i++,e)); // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} - // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}} + // cxx17-warning@-2 {{multiple indeterminately sequenced modifications to 'i'}} // Binary operators where the LHS is sequenced before the RHS in C++17. ((void)i++,e) << ((void)i++,e); @@ -435,16 +442,21 @@ operator<<(((void)i++,e), ((void)i++,e)); // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} - // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}} + // cxx17-warning@-2 {{multiple indeterminately sequenced modifications to 'i'}} +#if __cplusplus >= 202302L + ((void)i++,e)[((void)i++,e), ++i]; + // cxx17-warning@-1 {{multiple indeterminately sequenced modifications to 'i'}} +#else ((void)i++,e)[((void)i++,e)]; // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} +#endif ((void)i++,e)(((void)i++,e)); // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} e(((void)i++,e), ((void)i++,e)); // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} - // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}} + // cxx17-warning@-2 {{multiple indeterminately sequenced modifications to 'i'}} ((void)i++,e).operator()(((void)i++,e)); // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} @@ -464,7 +476,7 @@ operator<<(operator<<(s, i++), i++); // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}} - // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}} + // cxx17-warning@-2 {{multiple indeterminately sequenced modifications to 'i'}} } } @@ -805,7 +817,7 @@ foo(num++, num++); // cxx11-warning@-1 {{multiple unsequenced modifications to 'num'}} - // cxx17-warning@-2 {{multiple unsequenced modifications to 'num'}} + // cxx17-warning@-2 {{multiple indeterminately sequenced modifications to 'num'}} return 1; }