[CFG] [analyzer] Constructors of member CXXOperatorCallExpr's argument 0 are not argument constructors.

[NoQ]

Summary

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 D32642 but this patch doesn't go that far.

[george.karpenkov]

I take it we are crashing otherwise?

include/clang/Analysis/ConstructionContext.h

632

optional nit: I think comments are more readable when there's a blank line before each comment block.

[NoQ]

In D49627#1172023, @george.karpenkov wrote:

I take it we are crashing otherwise?

Not yet, but i'm about to commit code that would (D49443).

[NoQ]

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.

[baloghadamsoftware]

This comment was removed by baloghadamsoftware.

[baloghadamsoftware]

How much different is a correct this-argument construction context from real argument construction contexts?

[NoQ]

In 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.

[NoQ]

Hmm, AST has just changed in 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.

[NoQ]

Ok, so with 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.

include/clang/Analysis/ConstructionContext.h

public:
   const CXXBindTemporaryExpr *getCXXBindTemporaryExpr() const { return BTE; }
 
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == CXX17ElidedCopyReturnedValueKind;
   }
 };
 
 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.
 
   explicit ArgumentConstructionContext(const Expr *CE, unsigned Index,
                                        const CXXBindTemporaryExpr *BTE)
       : ConstructionContext(ArgumentKind), CE(CE),
         Index(Index), BTE(BTE) {
     assert(isa<CallExpr>(CE) || isa<CXXConstructExpr>(CE) ||

test/Analysis/cfg-rich-constructors.cpp

 // CXX11-NEXT:     5: [B1.3] (CXXConstructExpr, [B1.6], class copy_elision_with_extra_arguments::C)
 // CXX11-NEXT:     6: copy_elision_with_extra_arguments::C c = copy_elision_with_extra_arguments::C();
 // CXX17-NEXT:     1: copy_elision_with_extra_arguments::C() (CXXConstructExpr, [B1.2], class copy_elision_with_extra_arguments::C)
 // CXX17-NEXT:     2: copy_elision_with_extra_arguments::C c = copy_elision_with_extra_arguments::C();
 void testCopyElisionWhenCopyConstructorHasExtraArguments() {
   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

test/Analysis/temporaries.cpp

 
 void bar() {
   IntPtr ptr;
   int *i = ptr.i;
   Foo f(static_cast<IntPtr &&>(ptr));
   *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
+