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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
Hey!

This is short and sweet. MallocChecker uses both check::newAllocator and check::postStmt<CXXNewExpr> 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.

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

Cheers,
Husi

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
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.

On 2/26/20 5:02 PM, Kristóf Umann via cfe-dev wrote:

> Hey!
>
> This is short and sweet. MallocChecker uses both check::newAllocator
> and check::postStmt<CXXNewExpr> 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
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev


On 2/26/20 6:45 PM, Artem Dergachev wrote:

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

>
> On 2/26/20 5:02 PM, Kristóf Umann via cfe-dev wrote:
>> Hey!
>>
>> This is short and sweet. MallocChecker uses both check::newAllocator
>> and check::postStmt<CXXNewExpr> 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
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
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.

On Thu, 27 Feb 2020 at 15:53, Artem Dergachev <[hidden email]> wrote:


On 2/26/20 6:45 PM, Artem Dergachev wrote:
> 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.

>
> On 2/26/20 5:02 PM, Kristóf Umann via cfe-dev wrote:
>> Hey!
>>
>> This is short and sweet. MallocChecker uses both check::newAllocator
>> and check::postStmt<CXXNewExpr> 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
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev