[analyzer] Handle std::make_unique for SmartPtrModeling

[RedDocMD]

Summary

This patch models the std::make_unique and std::make_unique_for_overwrite methods.

[RedDocMD]

Accounting for std::make_unique_for_overwrite

[RedDocMD]

Fixed binding of SVal to variable

[RedDocMD]

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?

[xazax.hun]

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?

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.

[RedDocMD]

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 way handleGet obtains a new symbol cannot really be used here since we do not have an Expr for the inner pointer at hand. handleGet does the following:

C.getSValBuilder().conjureSymbolVal(
        CallExpr, C.getLocationContext(), Call.getResultType(), C.blockCount());

Since we have CallExpr, we can easily conjure up an SVal. But I don't see how I can do it similarly in this patch.

[RedDocMD]

Reformatted code

[RedDocMD]

Fixed git history

[xazax.hun]

Since we have CallExpr, we can easily conjure up an SVal. But I don't see how I can do it similarly in this patch.

You should have a CallExpr for std::make_unique too. I believe that expression is used to determine how the conjured symbol was created (to give it an identity). So probably it should be ok to use the make_unique call to create this symbol (but be careful to create the symbol with the right type).

[NoQ]

Tests pls!

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.

In D103750#2801320, @xazax.hun wrote:

Since we have CallExpr, we can easily conjure up an SVal. But I don't see how I can do it similarly in this patch.

You should have a CallExpr for std::make_unique too. I believe that expression is used to determine how the conjured symbol was created (to give it an identity). So probably it should be ok to use the make_unique call to create this symbol (but be careful to create the symbol with the right type).

That's right, a conjured symbol isn't necessarily the value of its respective expression. So you can conjure it and assume that it's non-null. Also consider using a heap conjured symbol (a-la getConjuredHeapSymbolVal()). That said, getConjuredHeapSymbolVal() doesn't provide an overload for overriding the type; there's no reason it shouldn't though, please feel free to extend it.

There's a separate story about SymbolConjured being the right class for the problem. I'm still in favor of having SymbolMetadata instead, so that to properly deduplicate symbols across different branches and merge more paths.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

245

Please use CallDescription for most of this stuff.

442

Please merge with isStdSmartPtrCall.

443–446

getTypePtr() is almost never necessary because QualType has an overloaded operator->().

[RedDocMD]

Made stylistic refactors

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

442

I don't think that can be done because isStdSmartPtrCall checks calls like a.foo(), checking if a is a smart-ptr. On the other hand I need to check for statements like a = foo() and ensure that a is a smart-ptr.

[RedDocMD]

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.

Am I not doing that now? I didn't get this point.

[NoQ]

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?

[RedDocMD]

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?

Because in evalCall, I don't have access to the ThisRegion. At least I think I don't.
So we have a statement like auto a = std::make_unique<int>(100). In evalCall I get a CallEvent corresponding to std::make_unique<int>(100). Using Call.getOriginExpr() I can get the LHS. But that's only the Expr, not the MemRegion. Unlike in a regular constructor call or method call, where ThisRegion can easily be obtained from the Call.

[xazax.hun]

In D103750#2803516, @RedDocMD wrote:

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?

Because in evalCall, I don't have access to the ThisRegion. At least I think I don't.

Did you consider trying to get it from CXXInstanceCall::getCXXThisVal? Usually, if you cannot access something you need, that is a bug in the analyzer. But I found that this is very rarely the case. Most of the callbacks are very well thought out and already gives access to most of the information you might want to access.

[NoQ]

In D103750#2803516, @RedDocMD wrote:

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?

Because in evalCall, I don't have access to the ThisRegion.

You do have access to it. That's how C++ works: the call constructs the value directly into the target this-region. In code

auto a = std::make_unique<int>(100)

it is known even before std::make_unique is invoked (i.e., even in checkPreCall) that the this-region for the call is going to be a. Because it's up to the call to invoke the constructor of the external object, and you can't invoke a constructor if you don't have a this to pass into it. You cannot ever have an object and not have this.

Even if it wasn't for RVO, you would have known the temporary region in which the smart pointer would originally reside.

This was the whole point of the CFG work described in https://www.youtube.com/watch?v=4n3l-ZcDJNY

You're looking in the wrong place though, as std::make_unique returns the structure by value (aka prvalue), so the value of the expression, even if available before the call, was never going to be this (which would have been the corresponding glvalue). What you're looking for is CallEvent::getReturnValueUnderConstruction().

[xazax.hun]

Whoops, I totally missed your point with my suggestion (should not read it while sitting at meetings). But Artem has a great answer already.

[NoQ]

Also congrats, sounds like you are to become the first actual user of this API! Hope it actually works 🤞🤞🤞

[RedDocMD]

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

[NoQ]

In D103750#2804438, @RedDocMD wrote:

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

Yeah, it's also pretty interesting ABI-wise. As you know, C++ doesn't have stable ABI. That said, one popular calling convention for functions that return C++ objects by value consists in passing a hidden argument to the function that contains the address at which to construct the return value. Like "this" is an implicit argument to methods, the return address is yet another implicit argument. So you can think of getReturnValueUnderConstruction() as of an accessor for that implicit argument value similarly to how getCXXThisVal() is the accessor for the implicit this-argument value.

This is completely different from C where you can return the structure on stack or even in registers (which can't be pointed to at all); this becomes possible in C because structures can be copied arbitrarily without invoking any sort of copy constructor.

---

Random general advice! When writing patches, you typically explore the design space of potential solutions - which you did great in this case. When you do so, it's a great idea to include the results of such exploration in the commit message (and/or in later comments). Especially if the first / most obvious thing to try didn't work. If you publish your second-choice solution, people will most likely ask you why you chose it instead of the obvious first-choice solution so it's an act of friendliness to anticipate this question. Similarly, if you have concerns about your solution, please communicate them openly; they'll either be discovered anyway but a lot more effort will be spent on it, or they won't be discovered during review which could result in future problems; both variants are worse than communicating openly. Ideally you carry more responsibility for proving that your approach is right than the reviewer has a responsibility to find problems in it. The reviewer is the last line of defense when it comes to applying critical thinking but the process really starts with you as you challenge your own assumptions and approaches by questioning both their correctness and their architectural harmony. Trying to find at least two solutions to each problem and compare them against each other is such a good exercise in critical thinking that almost every patch deserves such discussion.

[RedDocMD]

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

[RedDocMD]

Added tests

[RedDocMD]

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

[teemperor]

In D103750#2812613, @RedDocMD wrote:

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

If I understand your comment correctly, then see my inline comment.

clang/test/Analysis/smart-ptr-text-output.cpp

9

You set the standard here via the -std arg. If you want different standards you need to have multiple RUN: lines that all pass a different standard.

[RedDocMD]

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

[RedDocMD]

clang/test/Analysis/smart-ptr-text-output.cpp

9

Yup, thanks!

[teemperor]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

204

getParent() will assert when the MethodDecl is not a in a Record (which is the only way this can return null) so you can remove that check.

[RedDocMD]

Removed un-necessary check

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

280–281

Untyped region isn't a region without a type; everything has a type. Untyped region is when we don't know the type. A typical situation that produces untyped region is when the region comes in through a void pointer.

I vaguely remember that one way to trick your specific code may be to do

std::unique_ptr<int> foo() {
  return make_unique<int>(123);
}

which will RVO into an unknown region. I also wouldn't rely on it being typed in all other cases.

A much safer way to access the inner pointer type would be to query the function's template parameter.

293–296

I suggest a TODO: ExprEngine should do this for us..

298

Do we need a note here as well? I guess we don't because we'll never emit null dereference reports against a non-null pointer. But if we later emit more sophisticated bug reports, we might need one. Maybe leave a comment?

[RedDocMD]

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

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

278

Can you do a getConjuredHeapSymbolVal() instead? That'd give us the right memory space as well as the extra bit of information that the new symbol doesn't alias with any previous symbols.

I get it that it doesn't accept the type but it's perfectly ok for you to teach it how to accept the type.

clang/test/Analysis/smart-ptr-text-output.cpp

338

Mmm wait a sec, that doesn't look like what the spec says.

https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique:

Same as (1), except that the object is default-initialized. This overload participates in overload resolution only if T is not an array type. The function is equivalent to unique_ptr<T>(new T)

It zero-initializes the pointee, not the pointer.

The difference between std::make_unique<A>() and std::make_unique_for_overwrite<A>() is the difference between value-initialization (invoking the default constructor) and zero-initialization (simply filling the buffer with 0s).

[RedDocMD]

clang/test/Analysis/smart-ptr-text-output.cpp

338

Oops! Look like I completely misunderstood the spec.
So basically, the inner pointer is not null, and we have the same assumptions that apply for std::make_unique. Only problem is that, for primitive types, the value obtained on de-referencing is undefined.

[RedDocMD]

Fixed up meaning of make_unique_for_overwrite, use getConjuredHeapSymbolVal.

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

287–291

I think it's fine to have it here in this commit and change it in one of the future commits. But I think we should add a bit more description of 1) what's the problem, 2) how is this a solution, 3) how should it look when moved to the engine.

[NoQ]

This looks great to me, I think the patch is good to go as long as Valeriy's comment is addressed :)

Speaking of specs, hold up, we're forgetting something very important.

make_unique() also has to call the constructor of the underlying value. We're supposed to model that.

The constructor may be inlined or evaluated conservatively.

~unique_ptr() is also supposed to call the destructor of the underlying value. We're supposed to model that.

I.e., you could imagine a bunch of tests like this

struct S {
  int *p;
  S(int *p): p(p) {}
  ~S() { *p = 0; }
};

void foo() {
  int x = 1;
  {
    auto P = std::make_unique<S>(&x);
    clang_analyzer_eval(*P->p == 1); // TRUE
  }
  clang_analyzer_eval(x == 0); // TRUE
}

We could stick to conservative evaluation of such constructors and destructors; at the very least, we should actively invalidate Store at the destructor. This patch can remain unchanged because the default values inside freshly conjured regions are unknown. But that'd still be a regression because the above test (presumably) passes with inlining.

Is now a good time to legalize modeling function calls inside checker callbacks?

clang/test/Analysis/smart-ptr-text-output.cpp

65

Could you double-check that this flag correctly disables all the newly introduced modeling so that it wasn't accidentally enabled for all users before it's ready?

[RedDocMD]

Added more info to the TODO

[RedDocMD]

clang/test/Analysis/smart-ptr-text-output.cpp

65

Yup. There are 133 notes and warnings (just search for "// expected-" and see the count). And on setting the flag to false, there are 133 errors.
So it works.

[RedDocMD]

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.

[NoQ]

clang/test/Analysis/smart-ptr-text-output.cpp

65

I mean, specifically the modeling added by this patch? I see you wrote an entire checker callback that doesn't seem to have an analyzer-config option check anywhere in it. Simply having different results isn't sufficient to verify this.

[NoQ]

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?

[xazax.hun]

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.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

194

Nit: consider TemplateArgs.empty().

[RedDocMD]

clang/test/Analysis/smart-ptr-text-output.cpp

65

Oh I see.

[RedDocMD]

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?

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.

[RedDocMD]

Little changes, a failing test

[RedDocMD]

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.

Do you want the new failing test to be marked expected to fail?

[xazax.hun]

In D103750#2827566, @RedDocMD wrote:

Do you want the new failing test to be marked expected to fail?

I usually just add the wrong expectation to make the test pass and add a TODO comment that explains why is this wrong and we should fix it in the future.

[NoQ]

In D103750#2827566, @RedDocMD wrote:

Do you want the new failing test to be marked expected to fail?

The line of thinking here is that tests are just something that gives us a signal when the behavior changes. They don't necessarily demonstrate the expected behavior. If they don't there's still value in knowing that the observed behavior on them has changed.

For example, if you believe that your commit shouldn't change the observed behavior ("NFC commit", eg. refactoring), any signal that the observed behavior has changed should raise concerns and probably cause you to rethink the commit, even if the behavior has in fact improved.

Similarly, if the functional change is intended, you may still find it suspicious if it suddenly fixes a test that it wasn't supposed to fix. This may help you discover a bug in your commit that would cause the behavior to regress in other related cases.

You don't want the buggy test to be silenced, you want it to keep actively verifying that the bug is not yet fixed.

Then, separately, for every test there's a requirement to keep it understandable, so that the reader would understand what exactly is tested and what are the consequences of breaking the test. It's often valuable to provide comments indicating your level of confidence - "this test is extremely important to pass", "we don't really care about our behavior on this test but it's still nice to know when that behavior changes", etc. - and "we definitely don't want this test to pass but for now it does" is a possible answer as well and it doesn't make the test any less valuable.

So basically

void foo() {
  // FIXME: Should be FALSE.
  clang_analyzer_eval(1 + 1 == 3); // expected-warning{{TRUE}}
  // FIXME: Should be TRUE.
  clang_analyzer_eval(1 + 1 == 2); // expected-warning{{FALSE}}
}

is a great test to have.

[RedDocMD]

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.

What about this?

[xazax.hun]

In D103750#2828995, @RedDocMD wrote:

What about this?

I believe that you might see analyzer-eagerly-assume in action. Basically, instead of keeping a constraint unknown, the analyzer can split the path eagerly. So you have one path where the constraint is assumed to be true and one where it is assumed to be false. I think the rationale was that the user is likely to branch on certain conditions and doing it eagerly can make the analysis more precise in some cases but I do not really remember the details. Hopefully Artem has some more contexts.

clang/test/Analysis/smart-ptr-text-output.cpp

378

Nit: we usually add new lines to the ends of the files.

[NoQ]

Yes, @xazax.hun is correct.

It's incorrect to say that the static analyzer "doesn't seem to be able to make up its mind". The analyzer gives perfectly clear and consistent answers for each execution path it explores and it's not surprising that the results are different on different execution paths. The presence of the FALSE path indicates indicates that the test indeed doesn't pass: an impossible execution path is being explored.

Eagerly-assume is a thing because it produces easier constraints for the solver to solve. Also the state split is pretty justified on any boolean expression with no side effects.

[RedDocMD]

Fixing up tests

[RedDocMD]

Is the syntax of specifying expected notes and warnings documented somewhere? I could not find the note-specific syntax.

[RedDocMD]

Post-rebase cleanup

[RedDocMD]

Fixed up tests

[RedDocMD]

Marked test with FIXME notes

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

public:
 
   /// Conjure a symbol representing heap allocated memory region.
   ///
   /// Note, the expression should represent a location.
   DefinedOrUnknownSVal getConjuredHeapSymbolVal(const Expr *E,
                                                 const LocationContext *LCtx,
                                                 unsigned Count);
 
+  /// Conjure a symbol representing heap allocated memory region.
+  ///
+  /// Note, now, the expression *doesn't* need to represent a location.
+  /// But the type need to!
+  DefinedOrUnknownSVal getConjuredHeapSymbolVal(const Expr *E,
+                                                const LocationContext *LCtx,
+                                                QualType type, unsigned Count);
+
   DefinedOrUnknownSVal getDerivedRegionValueSymbolVal(
       SymbolRef parentSymbol, const TypedValueRegion *region);
 
   DefinedSVal getMetadataSymbolVal(const void *symbolTag,
                                    const MemRegion *region,
                                    const Expr *expr, QualType type,
                                    const LocationContext *LCtx,
                                    unsigned count);

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <string>
 
 using namespace clang;
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
     : public Checker<eval::Call, check::DeadSymbols, check::RegionChanges,
                      check::LiveSymbols> {
 
   bool isBoolConversionMethod(const CallEvent &Call) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
private:
   using SmartPtrMethodHandlerFn =
       void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
   CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
       {{"reset"}, &SmartPtrModeling::handleReset},
       {{"release"}, &SmartPtrModeling::handleRelease},
       {{"swap", 1}, &SmartPtrModeling::handleSwapMethod},
       {{"get"}, &SmartPtrModeling::handleGet}};
   const CallDescription StdSwapCall{{"std", "swap"}, 2};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+      {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
 
 // Checks if RD has name in Names and is in std namespace
 static bool hasStdClassWithName(const CXXRecordDecl *RD,
                                 ArrayRef<llvm::StringLiteral> Names) {
static QualType getInnerPointerType(CheckerContext C, const CXXRecordDecl *RD) {
   if (!RD || !RD->isInStdNamespace())
     return {};
 
   const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RD);
   if (!TSD)
     return {};
 
   auto TemplateArgs = TSD->getTemplateArgs().asArray();
-  if (TemplateArgs.size() == 0)
+  if (TemplateArgs.empty())
     return {};
   auto InnerValueType = TemplateArgs[0].getAsType();
   return C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
 }
 
+// This is for use with standalone-functions like std::make_unique,
+// std::make_unique_for_overwrite, etc. It reads the template parameter and
+// returns the pointer type corresponding to it,
+static QualType getPointerTypeFromTemplateArg(const CallEvent &Call,
+                                              CheckerContext &C) {
+  const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+  if (!FD || !FD->isFunctionTemplateSpecialization())
+    return {};
+  const auto &TemplateArgs = FD->getTemplateSpecializationArgs()->asArray();
+  if (TemplateArgs.size() == 0)
+    return {};
+  auto ValueType = TemplateArgs[0].getAsType();
+  return C.getASTContext().getPointerType(ValueType.getCanonicalType());
+}
+
 // Helper method to get the inner pointer type of specialized smart pointer
 // Returns empty type if not found valid inner pointer type.
 static QualType getInnerPointerType(const CallEvent &Call, CheckerContext &C) {
   const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
     return {};
 
   const auto *RecordDecl = MethodDecl->getParent();
bool SmartPtrModeling::isBoolConversionMethod(const CallEvent &Call) const {
   // TODO: Handle other methods, such as .get() or .release().
   // But once we do, we'd need a visitor to explain null dereferences
   // that are found via such modeling.
   const auto *CD = dyn_cast_or_null<CXXConversionDecl>(Call.getDecl());
   return CD && CD->getConversionType()->isBooleanType();
 }
 
 constexpr llvm::StringLiteral BASIC_OSTREAM_NAMES[] = {"basic_ostream"};
 
 bool isStdBasicOstream(const Expr *E) {
   const auto *RD = E->getType()->getAsCXXRecordDecl();
   return hasStdClassWithName(RD, BASIC_OSTREAM_NAMES);
 }
 
 bool isStdOstreamOperatorCall(const CallEvent &Call) {
   if (Call.getNumArgs() != 2 ||
       !Call.getDecl()->getDeclContext()->isStdNamespace())
     return false;
   const auto *FC = dyn_cast<SimpleFunctionCall>(&Call);
   if (!FC)
     return false;
   const FunctionDecl *FD = FC->getDecl();
   if (!FD->isOverloadedOperator())
     return false;
   const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
   if (OOK != clang::OO_LessLess)
     return false;
   return isStdSmartPtr(Call.getArgExpr(1)) &&
          isStdBasicOstream(Call.getArgExpr(0));
 }
 
 bool SmartPtrModeling::evalCall(const CallEvent &Call,
                                 CheckerContext &C) const {
+
   ProgramStateRef State = C.getState();
 
   // If any one of the arg is a unique_ptr, then
   // we can try this function
   if (Call.getNumArgs() == 2 &&
       Call.getDecl()->getDeclContext()->isStdNamespace())
     if (smartptr::isStdSmartPtr(Call.getArgExpr(0)) ||
         smartptr::isStdSmartPtr(Call.getArgExpr(1)))
       if (handleComparisionOp(Call, C))
         return true;
 
   if (isStdOstreamOperatorCall(Call))
     return handleOstreamOperator(Call, C);
 
   if (Call.isCalled(StdSwapCall)) {
     // Check the first arg, if it is of std::unique_ptr type.
     assert(Call.getNumArgs() == 2 && "std::swap should have two arguments");
     const Expr *FirstArg = Call.getArgExpr(0);
     if (!smartptr::isStdSmartPtr(FirstArg->getType()->getAsCXXRecordDecl()))
       return false;
     return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);
   }
 
+  if (Call.isCalled(StdMakeUniqueCall) ||
+      Call.isCalled(StdMakeUniqueForOverwriteCall)) {
+    if (!ModelSmartPtrDereference)
+      return false;
+    
+    const Optional<SVal> ThisRegionOpt = Call.getReturnValueUnderConstruction();
+    if (!ThisRegionOpt)
+      return false;
+
+    const auto PtrVal = C.getSValBuilder().getConjuredHeapSymbolVal(
+        Call.getOriginExpr(), C.getLocationContext(),
+        getPointerTypeFromTemplateArg(Call, C), C.blockCount());
+
+    const MemRegion *ThisRegion = ThisRegionOpt->getAsRegion();
+    State = State->set<TrackedRegionMap>(ThisRegion, PtrVal);
+    State = State->assume(PtrVal, true);
+
+    // TODO: ExprEngine should do this for us.
+    // For a bit more context:
+    // 1) Why do we need this? Since we are modelling a "function"
+    // that returns a constructed object we need to store this information in
+    // the program state.
+    //
+    // 2) Why does this work?
+    // `updateObjectsUnderConstruction` does exactly as it sounds.
+    //
+    // 3) How should it look like when moved to the Engine?
+    // It would be nice if we can just
+    // pretend we don't need to know about this - ie, completely automatic work.
+    // However, realistically speaking, I think we would need to "signal" the
+    // ExprEngine evalCall handler that we are constructing an object with this
+    // function call (constructors obviously construct, hence can be
+    // automatically deduced).
+    auto &Engine = State->getStateManager().getOwningEngine();
+    State = Engine.updateObjectsUnderConstruction(
+        *ThisRegionOpt, nullptr, State, C.getLocationContext(),
+        Call.getConstructionContext(), {});
+
+    // We don't leave a note here since it is guaranteed the
+    // unique_ptr from this call is non-null (hence is safe to de-reference).
+    C.addTransition(State);
+    return true;
+  }
+
   if (!smartptr::isStdSmartPtrCall(Call))
     return false;
 
   if (isBoolConversionMethod(Call)) {
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
     if (ModelSmartPtrDereference) {
std::pair<SVal, ProgramStateRef> SmartPtrModeling::retrieveOrConjureInnerPtrVal(
     QualType Type, CheckerContext &C) const {
   const auto *Ptr = State->get<TrackedRegionMap>(ThisRegion);
   if (Ptr)
     return {*Ptr, State};
   auto Val = C.getSValBuilder().conjureSymbolVal(E, C.getLocationContext(),
                                                  Type, C.blockCount());
   State = State->set<TrackedRegionMap>(ThisRegion, Val);
   return {Val, State};
 }
 
 bool SmartPtrModeling::handleComparisionOp(const CallEvent &Call,
                                            CheckerContext &C) const {
   const auto *FC = dyn_cast<SimpleFunctionCall>(&Call);
   if (!FC)
     return false;
   const FunctionDecl *FD = FC->getDecl();
   if (!FD->isOverloadedOperator())
     return false;
   const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
   if (!(OOK == OO_EqualEqual || OOK == OO_ExclaimEqual || OOK == OO_Less ||
         OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

DefinedOrUnknownSVal SValBuilder::conjureSymbolVal(const Stmt *stmt,
   return nonloc::SymbolVal(sym);
 }
 
 DefinedOrUnknownSVal
 SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
                                       const LocationContext *LCtx,
                                       unsigned VisitCount) {
   QualType T = E->getType();
-  assert(Loc::isLocType(T));
-  assert(SymbolManager::canSymbolicate(T));
-  if (T->isNullPtrType())
-    return makeZeroVal(T);
+  return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
+}
+
+DefinedOrUnknownSVal
+SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
+                                      const LocationContext *LCtx,
+                                      QualType type, unsigned VisitCount) {
+  assert(Loc::isLocType(type));
+  assert(SymbolManager::canSymbolicate(type));
+  if (type->isNullPtrType())
+    return makeZeroVal(type);
 
-  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
+  SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
 DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag,
                                               const MemRegion *region,
                                               const Expr *expr, QualType type,
                                               const LocationContext *LCtx,
                                               unsigned count) {

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

 bool operator<(nullptr_t x, const unique_ptr<T> &y);
 
 template <typename T>
 bool operator>=(nullptr_t x, const unique_ptr<T> &y);
 
 template <typename T>
 bool operator<=(nullptr_t x, const unique_ptr<T> &y);
 
+template <class T, class... Args>
+unique_ptr<T> make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template <class T>
+unique_ptr<T> make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
 namespace std {
 template <class CharT>
 class basic_ostream;
 
 using ostream = basic_ostream<char>;

clang/test/Analysis/smart-ptr-text-output.cpp

 // RUN: %clang_analyze_cc1\
-// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
+
+// RUN: %clang_analyze_cc1\
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 
 #include "Inputs/system-header-simulator-cxx.h"
 
+void clang_analyzer_eval(bool);
+
 class A {
 public:
   A(){};
   void foo();
 };
 
 A *return_null() {
   return nullptr;
void derefAfterReset() {
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
 void derefAfterResetWithNull() {
   A *NullInnerPtr = nullptr; // expected-note {{'NullInnerPtr' initialized to a null pointer value}}
   std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
   P.reset(NullInnerPtr); // expected-note {{Smart pointer 'P' reset using a null value}}
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   // expected-note@-1{{Dereference of null smart pointer 'P'}}
 }
 
 // FIXME: Fix this test when support is added for tracking raw pointer
 // and mark the smart pointer as interesting based on that and add tags.
 void derefOnReleasedNullRawPtr() {
   std::unique_ptr<A> P; // FIXME: add note "Default constructed smart pointer 'P' is null"
   A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer value}}
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}

 void derefAfterBranchingOnUnknownInnerPtr(std::unique_ptr<A> P) {
   A *RP = P.get();
   if (!RP) { // expected-note {{Assuming 'RP' is null}}
     // expected-note@-1 {{Taking true branch}}
     P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
     // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique<A>();
+  if (!P) {   // expected-note {{Taking false branch}}
+    P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+    // expected-note@-1 {{Taking true branch}}
+    P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+    // expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite<A>();
+  if (!P) {   // expected-note {{Taking false branch}}
+    P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+    // expected-note@-1 {{Taking true branch}}
+    P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+    // expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#endif
+
+struct G {
+  int *p;
+  G(int *p): p(p) {}
+  ~G() { *p = 0; }
+};
+
+void foo() {
+  int x = 1;
+  {
+    auto P = std::make_unique<G>(&x);
+    // FIXME: There should not be a state split here, it should take the true path.
+    clang_analyzer_eval(*P->p == 1); // expected-warning {{TRUE}}
+    // expected-warning@-1 {{FALSE}}
+    // expected-note@-2 {{Assuming the condition is true}}
+    // expected-note@-3 {{Assuming the condition is false}}
+    // expected-note@-4 {{TRUE}}
+    // expected-note@-5 {{FALSE}}
+    // expected-note@-6 {{Assuming the condition is false}}
+  }
+  // FIXME: Should be fixed when unique_ptr desctructors are
+  // properly modelled. This includes modelling the call to
+  // the destructor of the inner pointer type.
+  clang_analyzer_eval(x == 0); // expected-warning {{FALSE}}
+  // expected-note@-1 {{FALSE}}
+}