[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

All right, it seems that i'm completely misunderstanding this problem and we've been talking past each other this whole time.

The problem is not that we need to skip the `CXXConstructExpr`. The problem is that we need to skip `CXXNewExpr` (!!). CFG elements for an operator-new call go like this:

1. CFGAllocatorCall
2. CXXConstructExpr
3. CXXNewExpr

We're staring at element 3. The original loop says "hey, that's our statement! we've found it!". However, even though this is the statement that's taken as the call site, the respective ExplodedNode does not correspond to the end of the call. Instead, it corresponds to a moment of time after construction has finished and the new-expression is fully evaluated. So we need to ignore element 3. Then, element 2 is ignored automatically because the statements don't match. Then, we need to learn to recognize that element 1 is the right element, which is going to be either a `CallExitEnd` (if the allocator call was inlined) or dunno-but-definitely-not-`PostStmt` if the allocator was evaluated conservatively.

Bonus: the constructor is never inlined in our scenario, so it's easy to skip element 2, as it's going to be just PreStmt/PostStmt and no inlined call within it. Why? Because how the hell do you inline a constructor of a null pointer. The Standard guarantees that when the allocator returns a null pointer, the constructor is gracefully omitted. We currently have a FIXME in `prepareForObjectConstruction()` to implement this behavior:

  179         // TODO: Detect when the allocator returns a null pointer.
  180         // Constructor shall not be called in this case.

The current incorrect behavior is to instead evaluate the constructor conservatively, with a fake temporary region instead of the null pointer (this is generally the conservative approach to evaluating the constructor when object-under-construction is not known).

So my request to come up with a test case in which the constructor is going to be inlined was completely pointless. But in the general case we aren't really sure that it's going to be a null pointer (we may want to track an arbitrary value), so we shouldn't rely on that.



================
Comment at: clang/test/Analysis/new-ctor-null.cpp:37
   S *s = new S();
   // FIXME: Should be FALSE - we should not invalidate globals.
   clang_analyzer_eval(global); // expected-warning{{UNKNOWN}}
----------------
This is essentially the same FIXME: the constructor should not have been evaluated, so in particular we should not invalidate globals.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62926/new/

https://reviews.llvm.org/D62926