The only method that I think can be realistically modelled is == (and thus !=). If both the operands refer to the same unique_ptr, we know == returns true. If they are not the same, the only way == can return true if the two smart pointers were initialized from the same raw pointer. This is of course a fatal bug in itself. So perhaps we can ignore this case and only consider the first case.
The ordering operators I guess can't be handled because there is no way to statically tell in general the address of some value. We have the following deductions, nevertheless, mathematically:
Let ptr1 and ptr2 be two std::unique_ptr objects.
If (ptr1 == ptr2) is true:

• ptr1 < ptr2 is false
• ptr1 > ptr2 is false
• ptr1 <= ptr2 is true
• ptr1 >= ptr2 is true

If (ptr1 == ptr2) is false, we can't say anything really.

In D103750#2827548, @RedDocMD wrote:

In D103750#2825342, @NoQ wrote:

Do the above tests pass when your new evalCall modeling is enabled?

The analyzer doesn't seem to be able to make up its mind.

member-constructor.cpp:15:5: warning: FALSE [debug.ExprInspection]
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
member-constructor.cpp:15:25: note: Assuming the condition is false
    clang_analyzer_eval(*P->p == 0);
                        ^~~~~~~~~~
member-constructor.cpp:15:5: note: FALSE
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
member-constructor.cpp:15:5: warning: TRUE [debug.ExprInspection]
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
member-constructor.cpp:15:25: note: Assuming the condition is true
    clang_analyzer_eval(*P->p == 0);
                        ^~~~~~~~~~
member-constructor.cpp:15:5: note: TRUE
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.

I am closing this since it has been addressed much better by the patches from @vsavchenko.

Some more refactoring

In D103750#2825823, @xazax.hun wrote:

I believe there are a couple of comments that are done but not marked accordingly.
I agree with Artem, if we could craft code that fails due to not calling ctors, we should probably include them in the tests, even if they don't reflect the desired behavior they are a great source of documentation what is missing.

Little changes, a failing test

In D103750#2825342, @NoQ wrote:

In D103750#2823741, @RedDocMD wrote:

I would suppose that constructor calls are properly handled. (ie, member constructors are called properly).

Do the above tests pass when your new evalCall modeling is enabled?

Marking and un-marking interestingness

I would suppose that constructor calls are properly handled. (ie, member constructors are called properly).
As for modelling destructors, there is a separate problem - since we don't have a Stmt, the postCall handler keeps on crashing.

Added more info to the TODO

I have gotten rid of notes for the time being.
The trouble is that, we were not figuring out whether the null-pointer de-reference was occurring due to the null value being swapped with the current pointer.
We just assumed that. So I am guessing we would need a visitor to figure that out? (Hooray for Valeriy!)
Or is there a simpler solution?

Refactored common code, removed note emission

The current implementation of how notes are emitted in handleSwap is broken. Consider the following code:

#include <memory>

I am not entirely satisfied with the note that is being emitted currently from the std::swap handling (its ripped off from the note emitted for std::unique_ptr::swap).
Any suggestions?

Fixed up meaning of make_unique_for_overwrite, use getConjuredHeapSymbolVal.

Fixed up technique used to find inner pointer type, put TODO's

Removed un-necessary check

Fixed up tests, now also runnning on C++20

How do I set the C++ standard while running a test?

Added tests

Put changes discussed in the meeting, tests to come in next revision

Nice. Looks like I have something to read up on. Turns out, I end up learning something new in C++ every now and then. 😃

In D103750#2803499, @NoQ wrote:

Ugh, this entire checkBind hack was so unexpected that I didn't even recognize evalCall. What prevents you from doing everything in evalCall? No state traits, no nothing, just directly take the this-region and attach the value to it?

In D103750#2801555, @NoQ wrote:

Yes I think you should totally do an evalCall() here. The function has no other side effects apart from making a pointer so it's valuable to fully model it so that to avoid unnecessary store invalidations.

Made stylistic refactors

Fixed git history

Reformatted code

In D103750#2801248, @xazax.hun wrote:

You can always create a new symbol to represent the inner pointer. Something like this already happens, when you have a unique_ptr formal parameter and call get on it.

The drawback of the current approach is that we are not using the following piece of information:

std::unique_ptr created from std::make_unique is not null (to begin with)

I am not being able to use this info since I don't have access to the raw pointer, so cannot create a SVal and then constrain the SVal to non-null.
Any suggestions @NoQ, @vsavchenko , @xazax.hun, @teemperor?

Fixed binding of SVal to variable

Accounting for std::make_unique_for_overwrite

In D98726#2719100, @RedDocMD wrote:

Judging by this line in the LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor() method, it seems that the bug report is squelched when the visitor encounters an ExplodedNode which corresponds to a LocationContext, whose associated Decl lies in std namespace. I guess, by default, the option to suppress warnings from the std library is enabled. Which makes sense, except in this case since unique_ptr is in std and it is being used in that function, the bug report is suppressed.

In D103434#2795940, @vsavchenko wrote:

I was thinking a lot about this problem after our last call, and even though StoppableVisitor helps to solve the problem that we have, it is extremely unclear when this callback is called and should be called.
I decided to restore the balance (and rid of all younglings, maybe) and put together this interface: D103605. I still need to migrate the existing code into the new architecture, but it can be done incrementally.

Important question from @vsavchenko:

Moved visitor entirely to SmartPtrChecker.cpp, other refactors, better naming.

The following things are now done:

trackExpressionValue recieving callback, passing it to ReturnVisitor

Note: This patch is only partially done.

My bad! I forgot to submit the replies.

Removed extra include

More refactoring

I have put in the refactors, @teemperor.

Removed unnecessary include

Refactors to make code more stylistically accurate

Right, @teemperor, I will do the refactors once we figure out how to utilize trackExpressionValue() here (because that will eliminate quite a bit of code from GetNoteVisitor, so no point in refactoring those bits).

Removed un-necessary includes

Code clean up

Removed unnecessary includes

Added a test for multiple get's

@NoQ ?

In D98726#2721228, @NoQ wrote:

when the visitor encounters an ExplodedNode

Weird. finalizeVisitor() accepts not any node but the error node. Your screenshot suggests that the error node is not in the standard library but in user code. Might it be that there are multiple error nodes and you're looking at the wrong one? As usual, you can set conditional breakpoints by node IDs.

In D98726#2721228, @NoQ wrote:

when the visitor encounters an ExplodedNode

Weird. finalizeVisitor() accepts not any node but the error node. Your screenshot suggests that the error node is not in the standard library but in user code. Might it be that there are multiple error nodes and you're looking at the wrong one? As usual, you can set conditional breakpoints by node IDs.

Judging by this line in the LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor() method, it seems that the bug report is squelched when the visitor encounters an ExplodedNode which corresponds to a LocationContext, whose associated Decl lies in std namespace. I guess, by default, the option to suppress warnings from the std library is enabled. Which makes sense, except in this case since unique_ptr is in std and it is being used in that function, the bug report is suppressed.

The call to PathSensitiveBugReport::markInvalid() is triggered by LikelyFalsePositiveSuppressionBRVisitor::finalizeVisitor(). So I guess we need to see now why the visitor thinks this is a false positive.

As you can see here, the bug note is attached in the second case (please refer to my previous comment), but somehow, it doesn't finally show up as a bug.
Weird.

Okay, this is alarming.
The following code yields a leak warning:

#include <iostream>
#include <memory>

I think something is wrong about the way we are investigating this or I don't understand the MallocChecker.
The following doesn't yield a bug report! => https://godbolt.org/z/Y57G7zE5j

@NoQ?
(I actually should remove some extra includes and extra member fields)

@NoQ, I have changed to the visitor that you suggested.

Changed to a different visitor

It's important to realize that with pure static analysis it is absolutely impossible to reliably report a bug more severe than dead code. Any form of static analysis only ever finds code that doesn't make sense. It cannot make assumptions about how often the code is executed in practice or how severe and impactful the bug is to the users of the program under analysis. When we report anything that doesn't directly scream "dead code", like null dereference, we're still always implicitly saying "This code doesn't make sense because it either has dead parts or _____". In fact we should probably do a better job at managing expectations because users do become upset when we promise them use-after-frees but in reality only find dead code that "would have caused use-after-frees if it was ever run".

For the following function:

void foo(std::unique_ptr<A> P) {
  A* praw = P.get();
  A* other = praw;
  if (other) {}
  P->foo();
}

Where do we expect a note? Where praw is initialized, where other is initialized or both?

Right, sorry for the late reply, @NoQ.
I will get to it once I get these assignments off my head.

@NoQ, what do you think?

@steakhal?

@NoQ, I have taken a different approach this time. I have used a visitor and am storing some more data in the GDM. Together, they distinguish between the following three cases:

1. If the raw pointer obtained from get() is constrained to null in a path which leads to a Node (and thus State) where a smart-pointer-null-deref bug occurs.
2. If the raw pointer was null to begin with (because the smart-pointer was null)
3. If the raw pointer was not null to begin with but the smart-ptr became null after that.

Only in the first case should the note be emitted. I have added some more tests to that effect.
Can you please have a look at this?

Removed extra includes

Repaired git history

Changed approach to use visitor

@NoQ, why does the following trigger a null-dereference warning? (https://godbolt.org/z/Kxox8qd16)

void g(std::unique_ptr<A> a) {
  A *aptr = a.get();
  if (!aptr) {}
  a->foo();
}

When a->foo() is called, the constraint !aptr is no longer valid and so InnerPointerVal corresponding to a is no longer constrained to be null.
Am I missing something?

Added some more tests

Re-formatted file

Fixed up the git history

Added some more positive tests