[analyzer] What's up with c++-allocator-inlining?

Hey!

This is short and sweet. MallocChecker uses both check::newAllocator and check::postStmt to model aspects of operator new. Mind that these two are redundant not only with each other, but with the already used check::postCall. The reason I can see is handling all values of the analyzer config c+±allocator-inlining.

So, this flag has been true by default for a long-long time, and I personally never changed had the need to change it. Is there a need to keep tiptoeing around it? Here is a patch that tackles the issue, but it quite dated and I’m not too sure about the current state of things.

[analyzer] Add a new checker callback, check::NewAllocator.

https://reviews.llvm.org/D41406

Also, shouldn’t we make check::NewAllocator provide a CXXAllocatorCall rather then a CXXNewExpr and a related under-construction SVal?

Cheers,
Husi

Yes, we should remove the old code for c++-allocator-inlining=false. The worry we've had back then was that in the new mode we've disabled aggressive behavior of MallocChecker in which it reacted to some overloaded operator new invocations but i think this was the right thing to do and also nobody complained; also nothing prevents us from bringing back the old behavior in a much less confusing way.

Using a CXXAllocatorCall sounds wonderful as well. I'm afraid it might be technically difficult to do so because our Environment is screwed due to CXXNewExpr serving two different purposes, so there's a wrong SVal attached to it and CallEvent might be unable to retrieve the right SVal, so we're passing it separately. If this issue is solved, we might as well provide these callbacks as part of checkPreCall/checkPostCall and then abandon the check::NewAllocator callback entirely. More discussion on this in http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html

I think at this point we might actually do a good job sorting out this check::NewAllocator issue because we have a "separate" "Environment" to hold the other SVal, which is "objects under construction"! - so we should probably simply teach CXXAllocatorCall to extract the value from the objects-under-construction trait of the program state and we're good.

Yes, we should remove the old code for c++-allocator-inlining=false. The worry we've had back then was that in the new mode we've disabled aggressive behavior of MallocChecker in which it reacted to some overloaded operator new invocations but i think this was the right thing to do and also nobody complained; also nothing prevents us from bringing back the old behavior in a much less confusing way.

Using a CXXAllocatorCall sounds wonderful as well. I'm afraid it might be technically difficult to do so because our Environment is screwed due to CXXNewExpr serving two different purposes, so there's a wrong SVal attached to it and CallEvent might be unable to retrieve the right SVal, so we're passing it separately. If this issue is solved, we might as well provide these callbacks as part of checkPreCall/checkPostCall and then abandon the check::NewAllocator callback entirely. More discussion on this in http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html

I think at this point we might actually do a good job sorting out this check::NewAllocator issue because we have a "separate" "Environment" to hold the other SVal, which is "objects under construction"! - so we should probably simply teach CXXAllocatorCall to extract the value from the objects-under-construction trait of the program state and we're good.

Nvm, i'm wrong. CXXAllocatorCall should have the value before the cast but "objects under construction" and check::NewAllocator should have the value after the cast. So there's still no easy way to fix CXXAllocatorCall.

Ah, I see. Though, in the specific case of MallocChecker, I wonder whether there’s any point of using it once we touch postCall anyways.