[PATCH] D40560: [analyzer] WIP: Get construction into `operator new` running in simple cases.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D40560#947514, @NoQ wrote:

> Replaced the live expression hack with a slightly better approach. It doesn't update the live variables analysis to take `CFGNewAllocator` into account, but at least tests now pass.
>
> In order to keep the return value produced by the `operator new()` call around until `CXXNewExpr` is evaluated, i added a program state trait, `CXXNewAllocatorValueStack`:
>
> 1. Upon evaluating `CFGNewAllocator`, the return `SVal` of the evaluated allocator call is put here;
> 2. Upon evaluating `CXXConstructExpr`, that return value is retrieved from here;
> 3. Upon evaluating `CXXNewExpr`, the return value is retrieved from here again and then wiped.
>
>   In order to support nested allocator calls, this state trait is organized as a stack/FIFO, with push in `CFGNewAllocator` and pop in `CXXNewExpr`. The `llvm::ImmutableList` thing offers some asserts for warning us when we popped more times than we pushed; we might want to add more asserts here to detect other mismatches, but i don't see a need for implementing this as an environment-like map from (`Expr`, `LocationContext`) to SVal.


I think it would be great to have tests for such cases to make it apparent it is not trivial to do this only based on the cfg.
Maybe something like:

  A *a = new A(new B, coin ? new C : new D, inlinedCallWithMoreAllocs()); 

Or in case it turns out to be an easy problem to match these from CFG, I prefer avoiding the state.

Also having a new expression within an overloaded operator new might be interesting.

> Why `SVal` and not `MemRegion`? Because i believe that ideally we want to produce constructor calls with null or undefined `this`-arguments. Constructors into null pointers should be skipped - this is how `operator new(std::nothrow)` works, for instance. Constructors into garbage pointers should be immediately caught by the checkers (to throw a warning and generate a sink), but we still need to produce them so that the checkers could catch them. But for now `CXXConstructorCall` has room only for one pointer, which is currently `const MemRegion *`, so this still remains to be tackled.

Are you sure we need to produce undef vals? Couldn't a checker subscribe to the post allocator call and check for the undefined value and generate a sink before the constructor call? I am not opposed to using SVals there, just curious.

> Also we need to make sure that not only the expression lives, but also its value lives, with all the various traits attached to it. Hence the new facility in `ExprEngine::removeDead()` to mark the values in `CXXNewAllocatorValueStack` as live. Test included in `new-ctor-inlined.cpp` (the one with `s != 0`) covers this situation.
> 
> Some doxygen comments updated.




https://reviews.llvm.org/D40560