[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
This revision was automatically updated to reflect the committed changes. Closed by commit rC339087: [analyzer] NFC: Document that we support implicit argument constructors. (authored by dergachev, committed by ). Repository: rC Clang https://reviews.llvm.org/D49627 Files: include/clang/Analysis/ConstructionContext.h test/Analysis/cfg-rich-constructors.cpp test/Analysis/temporaries.cpp Index: include/clang/Analysis/ConstructionContext.h === --- include/clang/Analysis/ConstructionContext.h +++ include/clang/Analysis/ConstructionContext.h @@ -623,9 +623,16 @@ }; class ArgumentConstructionContext : public ConstructionContext { - const Expr *CE; // The call of which the context is an argument. - unsigned Index; // Which argument we're constructing. - const CXXBindTemporaryExpr *BTE; // Whether the object needs to be destroyed. + // The call of which the context is an argument. + const Expr *CE; + + // Which argument we're constructing. Note that when numbering between + // arguments and parameters is inconsistent (eg., operator calls), + // this is the index of the argument, not of the parameter. + unsigned Index; + + // Whether the object needs to be destroyed. + const CXXBindTemporaryExpr *BTE; friend class ConstructionContext; // Allows to create<>() itself. Index: test/Analysis/temporaries.cpp === --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -986,3 +986,21 @@ *i = 99; // no-warning } } // namespace ctor_argument + +namespace operator_implicit_argument { +struct S { + bool x; + S(bool x): x(x) {} + operator bool() const { return x; } +}; + +void foo() { + if (S(false)) { +clang_analyzer_warnIfReached(); // no-warning + } + if (S(true)) { +clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + } +} +} // namespace operator_implicit_argument + Index: test/Analysis/cfg-rich-constructors.cpp === --- test/Analysis/cfg-rich-constructors.cpp +++ test/Analysis/cfg-rich-constructors.cpp @@ -963,3 +963,35 @@ C c = C(); } } // namespace copy_elision_with_extra_arguments + + +namespace operators { +class C { +public: + C(int); + C &operator+(C Other); +}; + +// FIXME: Find construction context for the this-argument of the operator. +// CHECK: void testOperators() +// CHECK:[B1] +// CHECK-NEXT: 1: operator+ +// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class operators::C &(*)(class o +// CHECK-NEXT: 3: 1 +// CHECK-NEXT: 4: [B1.3] (CXXConstructExpr, [B1.6], class operators::C) +// CHECK-NEXT: 5: operators::C([B1.4]) (CXXFunctionalCastExpr, ConstructorConversion, class operato +// CHECK-NEXT: 6: [B1.5] +// CHECK-NEXT: 7: 2 +// CXX11-ELIDE-NEXT: 8: [B1.7] (CXXConstructExpr, [B1.10], [B1.11], class operators::C) +// CXX11-NOELIDE-NEXT: 8: [B1.7] (CXXConstructExpr, [B1.10], class operators::C) +// CXX11-NEXT: 9: operators::C([B1.8]) (CXXFunctionalCastExpr, ConstructorConversion, class operato +// CXX11-NEXT:10: [B1.9] +// CXX11-NEXT:11: [B1.10] (CXXConstructExpr, [B1.12]+1, class operators::C) +// CXX11-NEXT:12: [B1.6] + [B1.11] (OperatorCall) +// CXX17-NEXT: 8: [B1.7] (CXXConstructExpr, [B1.10]+1, class operators::C) +// CXX17-NEXT: 9: operators::C([B1.8]) (CXXFunctionalCastExpr, ConstructorConversion, class operato +// CXX17-NEXT:10: [B1.6] + [B1.9] (OperatorCall) +void testOperators() { + C(1) + C(2); +} +} // namespace operators Index: include/clang/Analysis/ConstructionContext.h === --- include/clang/Analysis/ConstructionContext.h +++ include/clang/Analysis/ConstructionContext.h @@ -623,9 +623,16 @@ }; class ArgumentConstructionContext : public ConstructionContext { - const Expr *CE; // The call of which the context is an argument. - unsigned Index; // Which argument we're constructing. - const CXXBindTemporaryExpr *BTE; // Whether the object needs to be destroyed. + // The call of which the context is an argument. + const Expr *CE; + + // Which argument we're constructing. Note that when numbering between + // arguments and parameters is inconsistent (eg., operator calls), + // this is the index of the argument, not of the parameter. + unsigned Index; + + // Whether the object needs to be destroyed. + const CXXBindTemporaryExpr *BTE; friend class ConstructionContext; // Allows to create<>() itself. Index: test/Analysis/temporaries.cpp === --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -986,3 +986,21 @@ *i = 99; // no-warning } } // namespace ctor_argument + +namespace operator_implicit_argument { +struct S { + bool x; + S(bool x): x(x) {} + operator b
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
NoQ updated this revision to Diff 158668. NoQ added a comment. Ok, so with https://reviews.llvm.org/rC338135 operator this-argument constructors do indeed work exactly like temporaries. This patch is not necessary anymore. I'd still prefer to add tests and i've added a path-sensitive test as well to demonstrate that the constructor works correctly. https://reviews.llvm.org/D49627 Files: include/clang/Analysis/ConstructionContext.h test/Analysis/cfg-rich-constructors.cpp test/Analysis/temporaries.cpp Index: test/Analysis/temporaries.cpp === --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -986,3 +986,21 @@ *i = 99; // no-warning } } // namespace ctor_argument + +namespace operator_implicit_argument { +struct S { + bool x; + S(bool x): x(x) {} + operator bool() const { return x; } +}; + +void foo() { + if (S(false)) { +clang_analyzer_warnIfReached(); // no-warning + } + if (S(true)) { +clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + } +} +} // namespace operator_implicit_argument + Index: test/Analysis/cfg-rich-constructors.cpp === --- test/Analysis/cfg-rich-constructors.cpp +++ test/Analysis/cfg-rich-constructors.cpp @@ -963,3 +963,35 @@ C c = C(); } } // namespace copy_elision_with_extra_arguments + + +namespace operators { +class C { +public: + C(int); + C &operator+(C Other); +}; + +// FIXME: Find construction context for the this-argument of the operator. +// CHECK: void testOperators() +// CHECK:[B1] +// CHECK-NEXT: 1: operator+ +// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class operators::C &(*)(class o +// CHECK-NEXT: 3: 1 +// CHECK-NEXT: 4: [B1.3] (CXXConstructExpr, [B1.6], class operators::C) +// CHECK-NEXT: 5: operators::C([B1.4]) (CXXFunctionalCastExpr, ConstructorConversion, class operato +// CHECK-NEXT: 6: [B1.5] +// CHECK-NEXT: 7: 2 +// CXX11-ELIDE-NEXT: 8: [B1.7] (CXXConstructExpr, [B1.10], [B1.11], class operators::C) +// CXX11-NOELIDE-NEXT: 8: [B1.7] (CXXConstructExpr, [B1.10], class operators::C) +// CXX11-NEXT: 9: operators::C([B1.8]) (CXXFunctionalCastExpr, ConstructorConversion, class operato +// CXX11-NEXT:10: [B1.9] +// CXX11-NEXT:11: [B1.10] (CXXConstructExpr, [B1.12]+1, class operators::C) +// CXX11-NEXT:12: [B1.6] + [B1.11] (OperatorCall) +// CXX17-NEXT: 8: [B1.7] (CXXConstructExpr, [B1.10]+1, class operators::C) +// CXX17-NEXT: 9: operators::C([B1.8]) (CXXFunctionalCastExpr, ConstructorConversion, class operato +// CXX17-NEXT:10: [B1.6] + [B1.9] (OperatorCall) +void testOperators() { + C(1) + C(2); +} +} // namespace operators Index: include/clang/Analysis/ConstructionContext.h === --- include/clang/Analysis/ConstructionContext.h +++ include/clang/Analysis/ConstructionContext.h @@ -623,9 +623,16 @@ }; class ArgumentConstructionContext : public ConstructionContext { - const Expr *CE; // The call of which the context is an argument. - unsigned Index; // Which argument we're constructing. - const CXXBindTemporaryExpr *BTE; // Whether the object needs to be destroyed. + // The call of which the context is an argument. + const Expr *CE; + + // Which argument we're constructing. Note that when numbering between + // arguments and parameters is inconsistent (eg., operator calls), + // this is the index of the argument, not of the parameter. + unsigned Index; + + // Whether the object needs to be destroyed. + const CXXBindTemporaryExpr *BTE; friend class ConstructionContext; // Allows to create<>() itself. Index: test/Analysis/temporaries.cpp === --- test/Analysis/temporaries.cpp +++ test/Analysis/temporaries.cpp @@ -986,3 +986,21 @@ *i = 99; // no-warning } } // namespace ctor_argument + +namespace operator_implicit_argument { +struct S { + bool x; + S(bool x): x(x) {} + operator bool() const { return x; } +}; + +void foo() { + if (S(false)) { +clang_analyzer_warnIfReached(); // no-warning + } + if (S(true)) { +clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + } +} +} // namespace operator_implicit_argument + Index: test/Analysis/cfg-rich-constructors.cpp === --- test/Analysis/cfg-rich-constructors.cpp +++ test/Analysis/cfg-rich-constructors.cpp @@ -963,3 +963,35 @@ C c = C(); } } // namespace copy_elision_with_extra_arguments + + +namespace operators { +class C { +public: + C(int); + C &operator+(C Other); +}; + +// FIXME: Find construction context for the this-argument of the operator. +// CHECK: void testOperators() +// CHECK:[B1] +// CHECK-NEXT: 1: operator+ +// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, Fu
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
NoQ added a comment. Hmm, AST has just changed in https://reviews.llvm.org/rC338135. While the current patch is still relevant, it might be that we'll need to treat this construction context as if it's a simple temporary now. https://reviews.llvm.org/D49627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
NoQ added a comment. In https://reviews.llvm.org/D49627#1176326, @baloghadamsoftware wrote: > How much different is a correct `this`-argument construction context from > real argument construction contexts? It's identified in a similar manner; this patch pretty much already shows how to identify it. It carries the same amount of information: what call, what argument. The tricky part is to come up with a good description for the target region and make sure it's actually used when the operator call is inlined. It would probably be a simple `CXXTempObjectRegion` because we don't have a `VarDecl` to make a `VarRegion`. Then we might need to proactively bind in in the Store to `CXXThisRegion` of the call, and for that purpose we might need a crystal ball to guess the future `StackFrameContext`, like in case of arguments. We still need to invalidate the target region if operator call is conservatively evaluated, like in case of arguments, because the destructor will use the updated object. https://reviews.llvm.org/D49627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
baloghadamsoftware added a comment. How much different is a correct `this`-argument construction context from real argument construction contexts? https://reviews.llvm.org/D49627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
baloghadamsoftware added a comment. "Actually supporting such this-argument construction contexts would address the FIXME that we've added in https://reviews.llvm.org/D32642 but this patch doesn't go that far." Maybe a FIXME here so we do not forget to continue it in the future? https://reviews.llvm.org/D49627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
NoQ added a comment. What i'm fixing here is a false detection of construction context of a certain kind. These are very bad in general, because they cause us to inline the constructor without knowing what object we're constructing or how to handle this object. https://reviews.llvm.org/D49627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
NoQ updated this revision to Diff 156827. NoQ marked an inline comment as done. NoQ added a comment. In https://reviews.llvm.org/D49627#1172023, @george.karpenkov wrote: > I take it we are crashing otherwise? Not yet, but i'm about to commit code that would (https://reviews.llvm.org/D49443). https://reviews.llvm.org/D49627 Files: include/clang/Analysis/ConstructionContext.h lib/Analysis/CFG.cpp test/Analysis/cfg-rich-constructors.cpp Index: test/Analysis/cfg-rich-constructors.cpp === --- test/Analysis/cfg-rich-constructors.cpp +++ test/Analysis/cfg-rich-constructors.cpp @@ -960,3 +960,37 @@ C c = C(); } } // namespace copy_elision_with_extra_arguments + + +namespace operators { +class C { +public: + C(int); + C &operator+(C Other); +}; + +// FIXME: Find construction context for the this-argument of the operator. +// CHECK: void testOperators() +// CHECK:[B1] +// CHECK-NEXT: 1: operator+ +// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class operators::C &(*)(class operators::C)) +// CHECK-NEXT: 3: 1 +// CXX11-NEXT: 4: [B1.3] (CXXConstructExpr, class operators::C) +// CXX11-NEXT: 5: operators::C([B1.4]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX11-NEXT: 6: 2 +// CXX11-ELIDE-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.9], [B1.10], class operators::C) +// CXX11-NOELIDE-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.9], class operators::C) +// CXX11-NEXT: 8: operators::C([B1.7]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX11-NEXT: 9: [B1.8] +// CXX11-NEXT:10: [B1.9] (CXXConstructExpr, [B1.11]+1, class operators::C) +// CXX11-NEXT:11: [B1.5] + [B1.10] (OperatorCall) +// CXX17-NEXT: 4: [B1.3] (CXXConstructExpr, class operators::C) +// CXX17-NEXT: 5: operators::C([B1.4]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX17-NEXT: 6: 2 +// CXX17-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.9]+1, class operators::C) +// CXX17-NEXT: 8: operators::C([B1.7]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX17-NEXT: 9: [B1.5] + [B1.8] (OperatorCall) +void testOperators() { + C(1) + C(2); +} +} // namespace operators Index: lib/Analysis/CFG.cpp === --- lib/Analysis/CFG.cpp +++ lib/Analysis/CFG.cpp @@ -684,7 +684,7 @@ void findConstructionContexts(const ConstructionContextLayer *Layer, Stmt *Child); - // Scan all arguments of a call expression for a construction context. + // Scan all arguments of a call-like expression for a construction context. // These sorts of call expressions don't have a common superclass, // hence strict duck-typing. template ::value>> void findConstructionContextsForArguments(CallLikeExpr *E) { for (unsigned i = 0, e = E->getNumArgs(); i != e; ++i) { + if (i == 0) { +// FIXME: This check will run for non-CallExpr variants as well, +// which is redundant. +if (const auto *OE = dyn_cast(E)) + if (dyn_cast_or_null(OE->getDirectCallee())) { +// It's an operator's this-argument that's mistaken for argument 0 +// due to an AST inconsistency. +// FIXME: Actually introduce the respective construction context. +continue; + } + } + Expr *Arg = E->getArg(i); if (Arg->getType()->getAsCXXRecordDecl() && !Arg->isGLValue()) findConstructionContexts( Index: include/clang/Analysis/ConstructionContext.h === --- include/clang/Analysis/ConstructionContext.h +++ include/clang/Analysis/ConstructionContext.h @@ -597,9 +597,16 @@ }; class ArgumentConstructionContext : public ConstructionContext { - const Expr *CE; // The call of which the context is an argument. - unsigned Index; // Which argument we're constructing. - const CXXBindTemporaryExpr *BTE; // Whether the object needs to be destroyed. + // The call of which the context is an argument. + const Expr *CE; + + // Which argument we're constructing. Note that when numbering between + // arguments and parameters is inconsistent (eg., operator calls), + // this is the index of the argument, not of the parameter. + unsigned Index; + + // Whether the object needs to be destroyed. + const CXXBindTemporaryExpr *BTE; friend class ConstructionContext; // Allows to create<>() itself. Index: test/Analysis/cfg-rich-constructors.cpp === --- test/Analysis/cfg-rich-constructors.cpp +++ test/Analysis/cfg-rich-constructors.cpp @@ -960,3 +960,37 @@ C c = C(); } } // namespace copy_elision_with_extra_arguments + + +namespace operators { +class C { +public: + C(int); + C &operator+(C Other); +}; + +// FIXME
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
george.karpenkov accepted this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. I take it we are crashing otherwise? Comment at: include/clang/Analysis/ConstructionContext.h:606 + unsigned Index; + // Whether the object needs to be destroyed. + const CXXBindTemporaryExpr *BTE; optional nit: I think comments are more readable when there's a blank line before each comment block. Repository: rC Clang https://reviews.llvm.org/D49627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49627: [CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.
NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, baloghadamsoftware. Herald added subscribers: cfe-commits, mikhail.ramalho. Because `CXXOperatorCallExpr`'s argument 0 is the `this`-argument of the operator if the operator is a member. This doesn't correspond to operator declaration parameters. Do not provide argument construction context for such arguments. For the remaining arguments, provide a context, even though argument index still doesn't match parameter index; the user would, unfortunately, need to work around that, as we can't satisfy both. Actually supporting such this-argument construction contexts would address the FIXME that we've added in https://reviews.llvm.org/D32642?id=153023#inline-426950 but this patch doesn't go that far. Repository: rC Clang https://reviews.llvm.org/D49627 Files: include/clang/Analysis/ConstructionContext.h lib/Analysis/CFG.cpp test/Analysis/cfg-rich-constructors.cpp Index: test/Analysis/cfg-rich-constructors.cpp === --- test/Analysis/cfg-rich-constructors.cpp +++ test/Analysis/cfg-rich-constructors.cpp @@ -960,3 +960,37 @@ C c = C(); } } // namespace copy_elision_with_extra_arguments + + +namespace operators { +class C { +public: + C(int); + C &operator+(C Other); +}; + +// FIXME: Find construction context for the this-argument of the operator. +// CHECK: void testOperators() +// CHECK:[B1] +// CHECK-NEXT: 1: operator+ +// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class operators::C &(*)(class operators::C)) +// CHECK-NEXT: 3: 1 +// CXX11-NEXT: 4: [B1.3] (CXXConstructExpr, class operators::C) +// CXX11-NEXT: 5: operators::C([B1.4]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX11-NEXT: 6: 2 +// CXX11-ELIDE-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.9], [B1.10], class operators::C) +// CXX11-NOELIDE-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.9], class operators::C) +// CXX11-NEXT: 8: operators::C([B1.7]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX11-NEXT: 9: [B1.8] +// CXX11-NEXT:10: [B1.9] (CXXConstructExpr, [B1.11]+1, class operators::C) +// CXX11-NEXT:11: [B1.5] + [B1.10] (OperatorCall) +// CXX17-NEXT: 4: [B1.3] (CXXConstructExpr, class operators::C) +// CXX17-NEXT: 5: operators::C([B1.4]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX17-NEXT: 6: 2 +// CXX17-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.9]+1, class operators::C) +// CXX17-NEXT: 8: operators::C([B1.7]) (CXXFunctionalCastExpr, ConstructorConversion, class operators::C) +// CXX17-NEXT: 9: [B1.5] + [B1.8] (OperatorCall) +void testOperators() { + C(1) + C(2); +} +} // namespace operators Index: lib/Analysis/CFG.cpp === --- lib/Analysis/CFG.cpp +++ lib/Analysis/CFG.cpp @@ -684,7 +684,7 @@ void findConstructionContexts(const ConstructionContextLayer *Layer, Stmt *Child); - // Scan all arguments of a call expression for a construction context. + // Scan all arguments of a call-like expression for a construction context. // These sorts of call expressions don't have a common superclass, // hence strict duck-typing. template ::value>> void findConstructionContextsForArguments(CallLikeExpr *E) { for (unsigned i = 0, e = E->getNumArgs(); i != e; ++i) { + if (i == 0) { +// FIXME: This check will run for non-CallExpr variants as well, +// which is redundant. +if (const auto *OE = dyn_cast(E)) + if (dyn_cast_or_null(OE->getDirectCallee())) { +// It's an operator's this-argument that's mistaken for argument 0 +// due to an AST inconsistency. +// FIXME: Actually introduce the respective construction context. +continue; + } + } + Expr *Arg = E->getArg(i); if (Arg->getType()->getAsCXXRecordDecl() && !Arg->isGLValue()) findConstructionContexts( Index: include/clang/Analysis/ConstructionContext.h === --- include/clang/Analysis/ConstructionContext.h +++ include/clang/Analysis/ConstructionContext.h @@ -597,9 +597,14 @@ }; class ArgumentConstructionContext : public ConstructionContext { - const Expr *CE; // The call of which the context is an argument. - unsigned Index; // Which argument we're constructing. - const CXXBindTemporaryExpr *BTE; // Whether the object needs to be destroyed. + // The call of which the context is an argument. + const Expr *CE; + // Which argument we're constructing. Note that when numbering between + // arguments and parameters is inconsistent (eg., operator calls), + // this is the index of the argument, not of the parameter. + unsigned I