[analyzer] Track trivial copy/move constructors and initializer lists in the BugReporter

[isuckatcs]

Summary

1.) Constructors

Prior to this patch, if an object had a trivial copy/move constructor we couldn't track
a value further than the copy constructor invocation. The problem is that in such cases
the analyzer doesn't inline the constructor, but performs a trivial copy instead. A trivial
copy means, we just copy the values from one place to another.

This patch handles this case by matching the regions of the 2 objects involved in the
construction, and tracks the appropriate region.

2.) Initializer lists

Prior to this patch we ignored initializer lists.

This patch handles them up to some extent.

[NoQ]

Great! Sounds like the conspiracy to not work with constructors goes deeper than I thought.

The new behavior looks lovely, I heavily recommend adding these examples as -analyzer-output=text tests.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2434

Hmmm so you're tracking *all* arguments. I'm worried that this will cause you to track values that aren't necessarily relevant to the report.

These notes are difficult to judge because it's usually important to maintain balance, too little information and the report becomes incomprehensible, too much information and people won't be able to navigate it. So we're only tracking things that are directly relevant (and we're trying our best to identify them).

For example, in this slightly more complicated case

int foo() {
    int *x = nullptr, *y = nullptr;
    S s{x, y};
    auto [a, b] = s;

    return *b;
}

we should ideally track the null pointer back to declaration of y but not x. A good long-term solution for that would probably be to track the individual *field* inside s (until we reach the field's initializer). I'm not sure how good are we at that right now, but I also don't immediately see any shortcuts that we could take here hmmm.

[isuckatcs]

• Addressed a comment
• Handling initializer lists.

I will add tests later.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2434

What I can do is finding the field the value belongs to and extract it's initializer expression. Then mark that as the initializer instead of the constructor.

The problem we face here is that the bug reporter checks the statements inside the exploded nodes. If the field initializer is not in the egraph, the bug reporter won't be able to track it.

In the snippet above ExprEngine::performTrivialCopy() is called, so the copy constructor is not inlined, hence the field initializer expressions are not in the CFG.

As a result if the constructor is not inlined, there's no way to tell which argument to track. It's either all or none. Also initializer lists aren't handled either, so in case of S s{x, y} neither x nor y would be tracked.

[isuckatcs]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2434

expressions are not in the CFG.

I meant egraph here instead of CFG.

[isuckatcs]

Updated

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2434

Aha interesting.

Every time BugReporter doesn't have enough information to reverse-engineer what ExprEngine has done, ExprEngine can leave breadcrumbs to help BugReporter figure out what happened. One mechanism we have for that is ProgramPointTag. You can either add a simple tag that BugReporter can pattern-match, or the modern solution is NoteTag that can replace entire bug visitors (but it's probably hard to integrate with an existing visitor).

Then again, performTrivialCopy() isn't the typical case. If we handle the typical case (i.e., add a non-trivial copy constructor to the test and handle that), that'd already be an improvement.

[isuckatcs]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2434

If we handle the typical case (i.e., add a non-trivial copy constructor to the test and handle that), that'd already be an improvement.

I'm handling one element copy constructors and initializer list, so for some simple snippets there are improvements.

E.g.:

struct S {
    int *a, *b;

    S(int *_a, int *_b) : a{_a}, b{_b} {}
};

void foo() {
    int *x = nullptr, *y = nullptr;

    S s{x, y};

    int i = *s.a;
}

The output before the patch:

test.cpp:12:7: note: Calling constructor for 'S'
    S s{x, y};
      ^~~~~~~
test.cpp:5:27: note: Null pointer value stored to 's.a'
    S(int *_a, int *_b) : a{_a}, b{_b} {}
                          ^
test.cpp:12:7: note: Returning from constructor for 'S'
    S s{x, y};
      ^~~~~~~
test.cpp:14:13: note: Dereference of null pointer (loaded from field 'a')
    int i = *s.a;
            ^  ~

The output after it:

test.cpp:10:10: note: 'x' initialized to a null pointer value
    int *x = nullptr, *y = nullptr;
         ^
test.cpp:12:9: note: Passing null pointer value via 1st parameter '_a'
    S s{x, y};
        ^
test.cpp:12:7: note: Calling constructor for 'S'
    S s{x, y};
      ^~~~~~~
test.cpp:5:27: note: Null pointer value stored to 's.a'
    S(int *_a, int *_b) : a{_a}, b{_b} {}
                          ^
test.cpp:12:7: note: Returning from constructor for 'S'
    S s{x, y};
      ^~~~~~~
test.cpp:14:13: note: Dereference of null pointer (loaded from field 'a')
    int i = *s.a;
            ^  ~

We can reach x because _a is tracked in the initializer list at a{_a}.

I think one solution would be to store somewhere where we started to track an expression and
where we ended it. Later when we encounter an initializer list or a constructor we can match the
parameters or the field initializers with the already tracked expressions and if we find a match
we know which value to track.

[isuckatcs]

New strategy for implicit copy/move ctor tracking.

Initializer lists are still not handled.

[isuckatcs]

• Progressed with initializer lists.
• Added more tests.

[xazax.hun]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1419

Nit: we have isCopyOrMoveConstructor.

1515

I think either the comment or the code might need to be updated. Trivial and user defined are not mutually exclusive terms.
Consider:

struct X {
  std::vector<int> v;
};

The copy constructor of X is neither trivial nor user provided.

[isuckatcs]

Rewrote from scratch

[isuckatcs]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1443–1444

This is still buggy and can result in false positives, so it's commented out for now. If we don't want to handle this situation, this will be removed.

[isuckatcs]

Updated

[isuckatcs]

Updated initializer list handling

[NoQ]

The current patch looks good to me, it matches my understanding of the problem. Excellent work!

There seem to be more loose ends to cover but I think it's better to commit this patch as-is, as a strict improvement, and follow-up later if you have time. Maybe leave comments for future generations to know where to look for trouble!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1437

I suspect that another possibility is ElementRegion for initializer lists that define arrays. Maybe leave a FIXME to look into that?

On the other hand, isa<InitListExpr>(E) sounds like it could become the type of variable E, to promote a runtime check to a compile-time check.

1453

Nitpick: The coding convention recommends against using auto when the type isn't obvious from the initializer. "Initializer" doesn't necessarily refer to an expression (could be like CXXCtorInitializer).

1490–1492

This probably needs another recursive descend, eg. if we track a.y.e and encounter S a = b then we need to track b.y.e.

Maybe leave a fixme?

[isuckatcs]

Addressed comments

[xazax.hun]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1451

I am a bit anxious about this matching. It probably works ok for most types, but consider a class like:

struct Common { int x; };

struct Inner {
  Common c;
};

struct S {
  Common c;
  Inner;
};

The type Common can be found at multiple levels. Would we ever confuse S::Common with S::Inner::Common? When we see a FieldRegion for Common:x, we would have a match twice. What guarantees that we always pick the right one? Or do I miss something?

Fortunately, picking the wrong one might not result in a wrong warning text, as subsequent matching might end up failing.

1473

I wonder whether top-level is ambiguous. Some people might think of s as the top level. How about innermost?

1475–1494

I feel like the code here is almost identical to the one at the beginning. Is it possible to extract this to a lambda and call it twice?

[isuckatcs]

Addressed comments

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1451

Would we ever confuse S::Common with S::Inner::Common?

Consider this snippet that uses the structs you created above with Inner i and int *x.

int *x = nullptr, *y = nullptr;
S s{x, y};

int a = *s.i.c.x;

The region we track in this case is s.i.c.x. The InitListExpr is

InitListExpr 'S':'struct S'
|-InitListExpr 'Common':'struct Common'
| `-ImplicitCastExpr 'int *' <LValueToRValue>
|   `-DeclRefExpr 'int *' lvalue Var  'x' 'int *'
`-InitListExpr 'Inner':'struct Inner'
  `-InitListExpr 'Common':'struct Common'
    `-ImplicitCastExpr 'int *' <LValueToRValue>
      `-DeclRefExpr 'int *' lvalue Var 'y' 'int *'

The type of this InitListExpr is RecordType 'struct S', so we first move up the regions
until we hit one with a matching type, which is s. Then we start moving back down the
regions and now the InitListExpr too, always choosing the sub-expression that matches
the index of the region.

So basically the functions always searches for the initializers top-down based on the field
indices, so I think if it confuses 2 regions, we made a mistake somewhere else.

Also I've reworked this function to be iterative and less confusing.

[NoQ]

LGTM as long as other reviewers are happy!

[martong]

Thanks, you have nice tests with good coverage! LGTM!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 }
 
 PathDiagnosticPieceRef StoreSiteFinder::VisitNode(const ExplodedNode *Succ,
                                                   BugReporterContext &BRC,
                                                   PathSensitiveBugReport &BR) {
   if (Satisfied)
     return nullptr;
 
   const ExplodedNode *StoreSite = nullptr;
   const ExplodedNode *Pred = Succ->getFirstPred();
   const Expr *InitE = nullptr;
   bool IsParam = false;
 
   // First see if we reached the declaration of the region.
   if (const auto *VR = dyn_cast<VarRegion>(R)) {
     if (isInitializationOfVar(Pred, VR)) {
       StoreSite = Pred;
       InitE = VR->getDecl()->getInit();
     }
   }
 
   // If this is a post initializer expression, initializing the region, we
   // should track the initializer expression.
   if (Optional<PostInitializer> PIP = Pred->getLocationAs<PostInitializer>()) {
     const MemRegion *FieldReg = (const MemRegion *)PIP->getLocationValue();
     if (FieldReg == R) {
       StoreSite = Pred;
       InitE = PIP->getInitializer()->getInit();
     }
   }
 
   // Otherwise, see if this is the store site:
   // (1) Succ has this binding and Pred does not, i.e. this is
   //     where the binding first occurred.
   // (2) Succ has this binding and is a PostStore node for this region, i.e.
   //     the same binding was re-assigned here.
   if (!StoreSite) {
     if (Succ->getState()->getSVal(R) != V)
       return nullptr;
 
     if (hasVisibleUpdate(Pred, Pred->getState()->getSVal(R), Succ, V)) {
       Optional<PostStore> PS = Succ->getLocationAs<PostStore>();
       if (!PS || PS->getLocationValue() != R)
         return nullptr;
     }
 
     StoreSite = Succ;
 
     // If this is an assignment expression, we can track the value
     // being assigned.
-    if (Optional<PostStmt> P = Succ->getLocationAs<PostStmt>())
+    if (Optional<PostStmt> P = Succ->getLocationAs<PostStmt>()) {
       if (const BinaryOperator *BO = P->getStmtAs<BinaryOperator>())
         if (BO->isAssignmentOp())
           InitE = BO->getRHS();
 
+      if (const CXXConstructExpr *CE = P->getStmtAs<CXXConstructExpr>()) {
+        InitE = CE;
+      }
+    }
+
     // If this is a call entry, the variable should be a parameter.
     // FIXME: Handle CXXThisRegion as well. (This is not a priority because
     // 'this' should never be NULL, but this visitor isn't just for NULL and
     // UndefinedVal.)
     if (Optional<CallEnter> CE = Succ->getLocationAs<CallEnter>()) {
       if (const auto *VR = dyn_cast<VarRegion>(R)) {
 
         if (const auto *Param = dyn_cast<ParmVarDecl>(VR->getDecl())) {
           ProgramStateManager &StateMgr = BRC.getStateManager();
           CallEventManager &CallMgr = StateMgr.getCallEventManager();
 
           CallEventRef<> Call = CallMgr.getCaller(CE->getCalleeContext(),
                                                   Succ->getState());
           InitE = Call->getArgExpr(Param->getFunctionScopeIndex());
         } else {
           // Handle Objective-C 'self'.
           assert(isa<ImplicitParamDecl>(VR->getDecl()));
           InitE = cast<ObjCMessageExpr>(CE->getCalleeContext()->getCallSite())
                       ->getInstanceReceiver()->IgnoreParenCasts();
         }
         IsParam = true;
       }
     }
 
     // If this is a CXXTempObjectRegion, the Expr responsible for its creation
     // is wrapped inside of it.
     if (const auto *TmpR = dyn_cast<CXXTempObjectRegion>(R))
       InitE = TmpR->getExpr();
   }
 
   if (!StoreSite)
     return nullptr;
 
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
   // came from.
   if (InitE) {
     if (!IsParam)
       InitE = InitE->IgnoreParenCasts();
 
     getParentTracker().track(InitE, StoreSite, Options);
   }
 
   // Let's try to find the region where the value came from.
   const MemRegion *OldRegion = nullptr;
 
   // If we have init expression, it might be simply a reference
   // to a variable, so we can use it.
   if (InitE) {
     // That region might still be not exactly what we are looking for.
     // In situations like `int &ref = val;`, we can't say that
     // `ref` is initialized with `val`, rather refers to `val`.
if (!E->isPRValue())
       return {};
 
     const ExplodedNode *RVNode = findNodeForExpression(ExprNode, E);
     if (!RVNode)
       return {};
 
     ProgramStateRef RVState = RVNode->getState();
     SVal V = RVState->getSValAsScalarOrLoc(E, RVNode->getLocationContext());
-    const auto *BO = dyn_cast<BinaryOperator>(E);
-
-    if (!BO || !BO->isMultiplicativeOp() || !V.isZeroConstant())
-      return {};
 
-    SVal RHSV = RVState->getSVal(BO->getRHS(), RVNode->getLocationContext());
-    SVal LHSV = RVState->getSVal(BO->getLHS(), RVNode->getLocationContext());
-
-    // Track both LHS and RHS of a multiplication.
     Tracker::Result CombinedResult;
     Tracker &Parent = getParentTracker();
-
-    const auto track = [&CombinedResult, &Parent, ExprNode, Opts](Expr *Inner) {
+    const auto track = [&CombinedResult, &Parent, ExprNode,
+                        Opts](const Expr *Inner) {
       CombinedResult.combineWith(Parent.track(Inner, ExprNode, Opts));
     };
 
+    const auto *BO = dyn_cast<BinaryOperator>(E);
+    if (BO && BO->isMultiplicativeOp() && V.isZeroConstant()) {
+      SVal RHSV = RVState->getSVal(BO->getRHS(), RVNode->getLocationContext());
+      SVal LHSV = RVState->getSVal(BO->getLHS(), RVNode->getLocationContext());
+
+      // Track both LHS and RHS of a multiplication.
       if (BO->getOpcode() == BO_Mul) {
         if (LHSV.isZeroConstant())
           track(BO->getLHS());
         if (RHSV.isZeroConstant())
           track(BO->getRHS());
       } else { // Track only the LHS of a division or a modulo.
         if (LHSV.isZeroConstant())
           track(BO->getLHS());
       }
 
       return CombinedResult;
     }
+
+    if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
+      for (auto &&arg : CE->arguments())
+        track(arg);
+
+      return CombinedResult;
+    }
+
+    return {};
+  }
 };
 
 Tracker::Tracker(PathSensitiveBugReport &Report) : Report(Report) {
   // Default expression handlers.
   addLowPriorityHandler<ControlDependencyHandler>();
   addLowPriorityHandler<NilReceiverHandler>();
   addLowPriorityHandler<ArrayIndexHandler>();
   addLowPriorityHandler<InterestingLValueHandler>();

clang/test/Analysis/osobject-retain-release.cpp

  // expected-note@-6{{Returning from '~smart_ptr'}}
   obj->release(); // expected-note{{Object released}}
   obj->release(); // expected-warning{{Reference-counted object is used after it is released}}
 // expected-note@-1{{Reference-counted object is used after it is released}}
 }
 
 void test_smart_ptr_leak() {
   OSObject *obj = new OSObject; // expected-note{{Operator 'new' returns an OSObject of type 'OSObject' with a +1 retain count}}
+  // expected-note@-1{{'obj' initialized here}}
   {
     OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr<OSObject>'}}
                         // expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
                         // expected-note@-2 {{'p' initialized here}}
     // expected-note@os_smart_ptr.h:13{{Field 'pointer' is non-null}}
     // expected-note@os_smart_ptr.h:13{{Taking true branch}}
     // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}}
     // expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}}