[analyzer] Model comparision methods of std::unique_ptr

[RedDocMD]

Summary

This patch handles all the comparision methods (defined via overloaded
operators) on std::unique_ptr. These operators compare the underlying
pointers, which makes it difficult to model the result.

[RedDocMD]

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.

[vsavchenko]

In D104616#2829705, @RedDocMD wrote:

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.

Sounds good to me!

[xazax.hun]

In D104616#2829705, @RedDocMD wrote:

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

Well, I think it depends. If one of the pointers is null, for some platforms, we can. E.g. null < non-null is probably true on most architectures, and similarly non-null < null is false. Also null <= anyptr is probably true on the platforms we care about.

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.

Is it? E.g. two default constructed unique_ptrs both should have null as the underlying pointer, they should be considered equal, are not the same object, and this is not a fatal bug.

[RedDocMD]

Logic for handling special cases, when both are unique_ptr

[vsavchenko]

Looks like a solid start!

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

323

nit: variable names should be capitalized

356

nit: llvm_unreachable instead of assert(false)

395–413

This switch exactly repeats the previous switch.

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

343–345

Another idea here.
Instead of repeating:

State = State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(RetVal), VALUE);
addTransition = true;

and having boolean addTransition, you can have Optional<bool> CompResult and do:

CompResult = VALUE;

And do the actual assumption at the bottom section when CompResult.hasValue()

367–386

These are also symmetrical.

416–458

These are symmetrical, is there a way to implement it without this boilerplate?

[NoQ]

In D104616#2830349, @xazax.hun wrote:

In D104616#2829705, @RedDocMD wrote:

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

Well, I think it depends. If one of the pointers is null, for some platforms, we can. E.g. null < non-null is probably true on most architectures, and similarly non-null < null is false. Also null <= anyptr is probably true on the platforms we care about.

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.

Is it? E.g. two default constructed unique_ptrs both should have null as the underlying pointer, they should be considered equal, are not the same object, and this is not a fatal bug.

Why not simply delegate this job to assume(evalBinOp(...)) over raw pointer values, which already has all this logic written down nicely?

[xazax.hun]

In D104616#2834714, @NoQ wrote:

Why not simply delegate this job to assume(evalBinOp(...)) over raw pointer values, which already has all this logic written down nicely?

This is what I had in mind, I just did not want to spoil it :)

[RedDocMD]

In D104616#2834770, @xazax.hun wrote:

In D104616#2834714, @NoQ wrote:

Why not simply delegate this job to assume(evalBinOp(...)) over raw pointer values, which already has all this logic written down nicely?

This is what I had in mind, I just did not want to spoil it :)

Looks like I have wasted a good deal of effort. :(

[xazax.hun]

In D104616#2835030, @RedDocMD wrote:

Looks like I have wasted a good deal of effort. :(

Sorry about that! :( If we learned anything new in the process it was not wasted effort though.

[RedDocMD]

In D104616#2835061, @xazax.hun wrote:

In D104616#2835030, @RedDocMD wrote:

Looks like I have wasted a good deal of effort. :(

Sorry about that! :( If we learned anything new in the process it was not wasted effort though.

I know what I learnt - if I am writing too much code, there is probably a function to that. :)

[RedDocMD]

Removed re-invention, added tests

[RedDocMD]

We have a failing test here (test at line 473).
Which makes me wonder if the handleComparision function is at all called. This is something I need to check.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

312

So it looks like operator<< is the only operator we are not supporting. I think it might be good to include some test code to ensure that its use does not suppress warnings. (Also OK, if you decide to deal with this in a follow-up PR.)

345

I think in cases where we know that the result is true or false, the RetVal should probably be a constant instead of a conjured symbol with an assumption.

352

Instead of marking this unreachable, I'd suggest to just return a conjured symbol for now. Usually, we should not use asserts to test user input.

365

I am not sure if we actually need all this special casing. You could create an SVal that represents a nullpointer constant and use evalBinOp with that SVal when there is no symbol available.

405

cannot reach here is redundant information. That is already encoded in llvm_unreachable. I suggest including a message that tells the reader why is it unreachable. In this case it could be "unexpected overloaded operator kind".

clang/test/Analysis/smart-ptr.cpp

466

Putting tests like this on the same path can be risky. Tests might split the execution path (maybe not now, but in the future). I think it might be more future proof to have a large switch statement that switches on an unknown value and put the tests in separate cases.

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

312

Yes that's the next patch. (and one more for std::hash).

352

Ah yes that's what is happening now. RetVal is initialized to a conjured value. If we can conclude no further, then that is what is returned - which is what happens here. In other cases, constraints are applied or it is replaced by the output from evalBinOp.

365

Actually the naming is a bit misleading here, perhaps. FirstPtr is not the inner pointer itself but a pointer to the SVal. So the test FirstPtr && !SecondPtr checks that we know the SVal for the inner pointer of the first arg but we do not know that of the second arg. Using a null-pointer constant would not help.
However, we could use a conjured value to simplify some work.

[RedDocMD]

Refactored code, removed duplications, fixed tests, added some more

[RedDocMD]

clang/test/Analysis/smart-ptr.cpp

466

I didn't quite get you.

[RedDocMD]

Removed dump statement

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

336

In case we conjure a new symbol, we want this stored in the TrackedRegionMap. So the analyzer can correctly record the constraints between the two symbols.

clang/test/Analysis/smart-ptr.cpp

466

You remember this in the other patch:

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.

It looks like this does not happen for overloaded comparison operators at the moment. But we might want to do that in the future (@NoW what do you think). I was wondering, if we want to future proof these test cases for that behavior. But looking at the test cases again, you only have two, where the expected result is unknown, and they are at the very end. So feel free to ignore this and leave the code as is.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

343

Btw, if we do not have a helper yet to translate between these enums in the analyer, we should create one. Could you look for some other places in the analyzer code base to double check?

[RedDocMD]

clang/test/Analysis/smart-ptr.cpp

466

Then perhaps I should leave a comment to indicate this possibility.

[RedDocMD]

First try at implementing conversion function from OverloadedOperatorKind to BinaryOperatorKind

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

355–356

Optimization: if one of the lookups succeeds, the other is unnecessary. A bit premature but I think we shouldn't lose too much elegance over this.

392–393

This sounds like a super common functionality. Doesn't .get() do the same as well? I think it should be de-duplicated into a top-level method.

447–450

Because these operators are pure boolean functions, a state split would be justified. It's pointless to call the operator unless both return values are feasible. I think you should do an assume() and transition into both states.

It might also make sense to consult -analyzer-config eagerly-assume= before doing it but it sounds like this option will stays true forever and we might as well remove it.

The spaceship operator is obviously an exceptional case. Invocation of the spaceship operator isn't a good indication that all three return values are feasible, might be only two.

[RedDocMD]

Sorry for not updating. Was down with fever.
This patch does *not* work now. operationKindFromOverloadedOperator is broken because the maps don't get populated. I am not entirely sure why this is happening.
Will try to fix tomorrow. @NoQ, @vsavchenko, @xazax.hun, @teemperor do you have a hunch as to why this may be happening?

[NoQ]

I suspect that you might want to include OperationKinds.def instead of OperationKinds.h.

[RedDocMD]

Fixed bug in enum conversion

[RedDocMD]

Refactored out common block

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

447–450

I think this comment has got displaced from its original location from subsequent updates. Could you please clarify?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

447–450

You can always go back in time by clicking the |<< button in the top-left corner of the comment ;)

I'm trying to say that you should introduce a state split while evaluating the operators, not wait for the control flow operators to do that for you, exactly like the static analyzer currently does for the raw comparison operators.

[RedDocMD]

Performing state split on normal comparision ops

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

409

Do we want to create a new SVal in this case? Why returning S is insufficient?

417

While this is smart (overwriting the state here), I think this is hard to follow. I'd prefer this lambda returning a pair.

436

assume might not return a state (when the analyzer is smart enough to figure out one path is infeasible). While your code would probably work as is, as far as I remember we usually try to not call addTransition with null states.

clang/test/Analysis/smart-ptr.cpp

484

I do not see test cases where the path is actually split. Would you add some?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

432

Now that we've made a state split, it makes sense to bind a concrete true value or a concrete false value (depending on the state) instead of a symbolic value. This produces simpler symbolic equations for our constraint solver to handle.

I.e., something like

std::tie(TrueState, FalseState) = State->assume(...);
if (TrueState)
  C.addTransition(TrueState->BindExpr(E, LCtx, SVB.makeTruthVal(true));
if (FalseState)
  C.addTransition(FalseState->BindExpr(E, LCtx, SVB.makeTruthVal(false));

436

Yeah I have a lot of anxiety about passing nulls there as well, have to look up the source code of these functions every time :/

[RedDocMD]

clang/test/Analysis/smart-ptr.cpp

484

Oops! I removed two tests emitting UNKNOWN and forgot to put them back in.

[RedDocMD]

Simplify SVal on state split, other refactors

[NoQ]

I only have a few nits left.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

36

I think you're not using this anymore.

317

One good place to put this may be CheckerHelpers.h. This is where we dump all the stuff that's probably useful in multiple checkers but not in other places.

I also wonder if you plan to support unary operators. The interesting part about them is that they are sometimes ambiguous to binary operators in their string representation, eg. -.

436

C.getSValBuilder() is called three times now, you might want to stash it in a reference. There's probably not much performance benefit but the code should be easier to read this way.

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

983

I think it's a good idea to test these cross-type comparison operators as well. IIUC they're handled exactly the same as their same-type counterparts but it may uncover occasional assertion failures.

[RedDocMD]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

317

I guess passing a parameter to chose operator arity should work.

[RedDocMD]

Little refactors

[NoQ]

If everybody's happy let's commit :)

clang/test/Analysis/smart-ptr.cpp

467

487

494

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

351

Nit: couldn't you just return retrieveOrConjureInnerPtrVal(Reg, E, Type, C); instead of all the ceremony with std::tie?

366

I think we might end up losing some information here. Imagine both makeSValFor conjuring a new symbol. Only one of the symbols will be stored in the state since capturing happend once, when you created the lambda. Maybe it is better to ask for a state in makeSValFor as a parameter instead of doing a lambda capture.

Moreover, retrieveOrConjureInnerPtrVal will not even look at the state captured by makeSValFor. It will ask the CheckerContext to get the state, but that state is the state from the beginning of the function that does not have any of the modifications you made to it.

[RedDocMD]

Major bug fix

[xazax.hun]

The latest version looks good to me, let's land this!

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

 //== CheckerHelpers.h - Helper functions for checkers ------------*- C++ -*--=//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines CheckerVisitor.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
 #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
 
+#include "clang/AST/OperationKinds.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "llvm/ADT/Optional.h"
 #include <tuple>
 
 namespace clang {
 
 class Expr;
 class VarDecl;
 class QualType;

 /// Get nullability annotation for a given type.
 Nullability getNullabilityAnnotation(QualType Type);
 
 /// Try to parse the value of a defined preprocessor macro. We can only parse
 /// simple expressions that consist of an optional minus sign token and then a
 /// token for an integer. If we cannot parse the value then None is returned.
 llvm::Optional<int> tryExpandAsInteger(StringRef Macro, const Preprocessor &PP);
 
+class OperatorKind {
+  union {
+    BinaryOperatorKind Bin;
+    UnaryOperatorKind Un;
+  } Op;
+  bool IsBinary;
+
+public:
+  explicit OperatorKind(BinaryOperatorKind Bin) : Op{Bin}, IsBinary{true} {}
+  explicit OperatorKind(UnaryOperatorKind Un) : IsBinary{false} { Op.Un = Un; }
+  bool IsBinaryOp() const { return IsBinary; }
+
+  BinaryOperatorKind GetBinaryOpUnsafe() const {
+    assert(IsBinary && "cannot get binary operator - we have a unary operator");
+    return Op.Bin;
+  }
+
+  Optional<BinaryOperatorKind> GetBinaryOp() const {
+    if (IsBinary)
+      return Op.Bin;
+    return {};
+  }
+
+  UnaryOperatorKind GetUnaryOpUnsafe() const {
+    assert(!IsBinary &&
+           "cannot get unary operator - we have a binary operator");
+    return Op.Un;
+  }
+
+  Optional<UnaryOperatorKind> GetUnaryOp() const {
+    if (!IsBinary)
+      return Op.Un;
+    return {};
+  }
+};
+
+OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
+                                                 bool IsBinary);
+
 } // namespace ento
 
 } // namespace clang
 
 #endif

clang/lib/StaticAnalyzer/Checkers/SmartPtr.h

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 
 namespace clang {
 namespace ento {
 namespace smartptr {
 
 /// Returns true if the event call is on smart pointer.
 bool isStdSmartPtrCall(const CallEvent &Call);
+bool isStdSmartPtr(const CXXRecordDecl *RD);
+bool isStdSmartPtr(const Expr *E);
 
 /// Returns whether the smart pointer is null or not.
 bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion);
 
 const BugType *getNullDereferenceBugType();
 
 } // namespace smartptr
 } // namespace ento
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_SMARTPTR_H

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

 #include "clang/AST/Type.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#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> {
 
private:
   void handleSwap(const CallEvent &Call, CheckerContext &C) const;
   void handleGet(const CallEvent &Call, CheckerContext &C) const;
   bool handleAssignOp(const CallEvent &Call, CheckerContext &C) const;
   bool handleMoveCtr(const CallEvent &Call, CheckerContext &C,
                      const MemRegion *ThisRegion) const;
   bool updateMovedSmartPointers(CheckerContext &C, const MemRegion *ThisRegion,
                                 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent &Call, CheckerContext &C) const;
+  bool handleComparisionOp(const CallEvent &Call, CheckerContext &C) const;
+  std::pair<SVal, ProgramStateRef>
+  retrieveOrConjureInnerPtrVal(ProgramStateRef State,
+                               const MemRegion *ThisRegion, const Expr *E,
+                               QualType Type, CheckerContext &C) const;
 
   using SmartPtrMethodHandlerFn =
       void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
   CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
       {{"reset"}, &SmartPtrModeling::handleReset},
       {{"release"}, &SmartPtrModeling::handleRelease},
       {{"swap", 1}, &SmartPtrModeling::handleSwap},
       {{"get"}, &SmartPtrModeling::handleGet}};
 };
 } // end of anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
 
 // Define the inter-checker API.
 namespace clang {
 namespace ento {
 namespace smartptr {
 bool isStdSmartPtrCall(const CallEvent &Call) {
   const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
   if (!MethodDecl || !MethodDecl->getParent())
     return false;
+  return isStdSmartPtr(MethodDecl->getParent());
+}
 
-  const auto *RecordDecl = MethodDecl->getParent();
-  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+bool isStdSmartPtr(const CXXRecordDecl *RD) {
+  if (!RD || !RD->getDeclContext()->isStdNamespace())
     return false;
 
-  if (RecordDecl->getDeclName().isIdentifier()) {
-    StringRef Name = RecordDecl->getName();
+  if (RD->getDeclName().isIdentifier()) {
+    StringRef Name = RD->getName();
     return Name == "shared_ptr" || Name == "unique_ptr" || Name == "weak_ptr";
   }
   return false;
 }
 
+bool isStdSmartPtr(const Expr *E) {
+  return isStdSmartPtr(E->getType()->getAsCXXRecordDecl());
+}
+
 bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion) {
   const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
   return InnerPointVal &&
          !State->assume(InnerPointVal->castAs<DefinedOrUnknownSVal>(), true);
 }
 } // namespace smartptr
 } // namespace ento
 } // namespace clang
static ProgramStateRef updateSwappedRegion(ProgramStateRef State,
   if (RegionInnerPointerVal) {
     State = State->set<TrackedRegionMap>(Region, *RegionInnerPointerVal);
   } else {
     State = State->remove<TrackedRegionMap>(Region);
   }
   return State;
 }
 
-// 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();
-  if (!RecordDecl || !RecordDecl->isInStdNamespace())
+static QualType getInnerPointerType(CheckerContext C, const CXXRecordDecl *RD) {
+  if (!RD || !RD->isInStdNamespace())
     return {};
 
-  const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RecordDecl);
+  const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RD);
   if (!TSD)
     return {};
 
   auto TemplateArgs = TSD->getTemplateArgs().asArray();
   if (TemplateArgs.size() == 0)
     return {};
   auto InnerValueType = TemplateArgs[0].getAsType();
   return C.getASTContext().getPointerType(InnerValueType.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();
+  return getInnerPointerType(C, RecordDecl);
+}
+
 // Helper method to pretty print region and avoid extra spacing.
 static void checkAndPrettyPrintRegion(llvm::raw_ostream &OS,
                                       const MemRegion *Region) {
   if (Region->canPrintPretty()) {
     OS << " ";
     Region->printPretty(OS);
   }
 }
 
 bool SmartPtrModeling::isBoolConversionMethod(const CallEvent &Call) const {
   // TODO: Update CallDescription to support anonymous calls?
   // 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();
 }
 
 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 (!smartptr::isStdSmartPtrCall(Call))
     return false;
 
   if (isBoolConversionMethod(Call)) {
     const MemRegion *ThisR =
         cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
 
     if (ModelSmartPtrDereference) {
bool SmartPtrModeling::evalCall(const CallEvent &Call,
   const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
   if (!Handler)
     return false;
   (this->**Handler)(Call, C);
 
   return C.isDifferent();
 }
 
+std::pair<SVal, ProgramStateRef> SmartPtrModeling::retrieveOrConjureInnerPtrVal(
+    ProgramStateRef State, const MemRegion *ThisRegion, const Expr *E,
+    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 ||
+        OOK == OO_Spaceship))
+    return false;
+
+  // There are some special cases about which we can infer about
+  // the resulting answer.
+  // For reference, there is a discussion at https://reviews.llvm.org/D104616.
+  // Also, the cppreference page is good to look at
+  // https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_cmp.
+
+  auto makeSValFor = [&C, this](ProgramStateRef State, const Expr *E,
+                                SVal S) -> std::pair<SVal, ProgramStateRef> {
+    if (S.isZeroConstant()) {
+      return {S, State};
+    }
+    const MemRegion *Reg = S.getAsRegion();
+    assert(Reg &&
+           "this pointer of std::unique_ptr should be obtainable as MemRegion");
+    QualType Type = getInnerPointerType(C, E->getType()->getAsCXXRecordDecl());
+    return retrieveOrConjureInnerPtrVal(State, Reg, E, Type, C);
+  };
+
+  SVal First = Call.getArgSVal(0);
+  SVal Second = Call.getArgSVal(1);
+  const auto *FirstExpr = Call.getArgExpr(0);
+  const auto *SecondExpr = Call.getArgExpr(1);
+
+  const auto *ResultExpr = Call.getOriginExpr();
+  const auto *LCtx = C.getLocationContext();
+  auto &Bldr = C.getSValBuilder();
+  ProgramStateRef State = C.getState();
+
+  SVal FirstPtrVal, SecondPtrVal;
+  std::tie(FirstPtrVal, State) = makeSValFor(State, FirstExpr, First);
+  std::tie(SecondPtrVal, State) = makeSValFor(State, SecondExpr, Second);
+  BinaryOperatorKind BOK =
+      operationKindFromOverloadedOperator(OOK, true).GetBinaryOpUnsafe();
+  auto RetVal = Bldr.evalBinOp(State, BOK, FirstPtrVal, SecondPtrVal,
+                               Call.getResultType());
+
+  if (OOK != OO_Spaceship) {
+    ProgramStateRef TrueState, FalseState;
+    std::tie(TrueState, FalseState) =
+        State->assume(*RetVal.getAs<DefinedOrUnknownSVal>());
+    if (TrueState)
+      C.addTransition(
+          TrueState->BindExpr(ResultExpr, LCtx, Bldr.makeTruthVal(true)));
+    if (FalseState)
+      C.addTransition(
+          FalseState->BindExpr(ResultExpr, LCtx, Bldr.makeTruthVal(false)));
+  } else {
+    C.addTransition(State->BindExpr(ResultExpr, LCtx, RetVal));
+  }
+  return true;
+}
+
 void SmartPtrModeling::checkDeadSymbols(SymbolReaper &SymReaper,
                                         CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   // Clean up dead regions from the region map.
   TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
   for (auto E : TrackedRegions) {
     const MemRegion *Region = E.first;
     bool IsRegDead = !SymReaper.isLiveRegion(Region);
 
     if (IsRegDead)
       State = State->remove<TrackedRegionMap>(Region);
   }
   C.addTransition(State);
 }
 
 void SmartPtrModeling::printState(raw_ostream &Out, ProgramStateRef State,
                                   const char *NL, const char *Sep) const {
   TrackedRegionMapTy RS = State->get<TrackedRegionMap>();
 
   if (!RS.isEmpty()) {
     Out << Sep << "Smart ptr regions :" << NL;
     for (auto I : RS) {
       I.first->dumpToStream(Out);
       if (smartptr::isNullSmartPtr(State, I.first))
         Out << ": Null";
       else
         Out << ": Non Null";
       Out << NL;
     }
   }
 }
 
 ProgramStateRef SmartPtrModeling::checkRegionChanges(
     ProgramStateRef State, const InvalidatedSymbols *Invalidated,
     ArrayRef<const MemRegion *> ExplicitRegions,
     ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx,
     const CallEvent *Call) const {
   TrackedRegionMapTy RegionMap = State->get<TrackedRegionMap>();
   TrackedRegionMapTy::Factory &RegionMapFactory =
       State->get_context<TrackedRegionMap>();
   for (const auto *Region : Regions)
     RegionMap = removeTrackedSubregions(RegionMap, RegionMapFactory,
                                         Region->getBaseRegion());
   return State->set<TrackedRegionMap>(RegionMap);
 }
 
 void SmartPtrModeling::checkLiveSymbols(ProgramStateRef State,
                                         SymbolReaper &SR) const {
   // Marking tracked symbols alive
   TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
   for (auto I = TrackedRegions.begin(), E = TrackedRegions.end(); I != E; ++I) {
     SVal Val = I->second;
     for (auto si = Val.symbol_begin(), se = Val.symbol_end(); si != se; ++si) {
       SR.markLive(*si);
     }
   }
 }
 
 void SmartPtrModeling::handleReset(const CallEvent &Call,
                                    CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
 
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisRegion)
     return;
 
   assert(Call.getArgExpr(0)->getType()->isPointerType() &&
          "Adding a non pointer value to TrackedRegionMap");
   State = State->set<TrackedRegionMap>(ThisRegion, Call.getArgSVal(0));
   const auto *TrackingExpr = Call.getArgExpr(0);
   C.addTransition(
       State, C.getNoteTag([ThisRegion, TrackingExpr](PathSensitiveBugReport &BR,
                                                      llvm::raw_ostream &OS) {
         if (&BR.getBugType() != smartptr::getNullDereferenceBugType() ||
             !BR.isInteresting(ThisRegion))
           return;
         bugreporter::trackExpressionValue(BR.getErrorNode(), TrackingExpr, BR);
         OS << "Smart pointer";
         checkAndPrettyPrintRegion(OS, ThisRegion);
         OS << " reset using a null value";
       }));
   // TODO: Make sure to ivalidate the region in the Store if we don't have
   // time to model all methods.
void SmartPtrModeling::handleGet(const CallEvent &Call,
   if (!IC)
     return;
 
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
   if (!ThisRegion)
     return;
 
   SVal InnerPointerVal;
-  if (const auto *InnerValPtr = State->get<TrackedRegionMap>(ThisRegion)) {
-    InnerPointerVal = *InnerValPtr;
-  } else {
-    const auto *CallExpr = Call.getOriginExpr();
-    InnerPointerVal = C.getSValBuilder().conjureSymbolVal(
-        CallExpr, C.getLocationContext(), Call.getResultType(), C.blockCount());
-    State = State->set<TrackedRegionMap>(ThisRegion, InnerPointerVal);
-  }
-
+  std::tie(InnerPointerVal, State) = retrieveOrConjureInnerPtrVal(
+      State, ThisRegion, Call.getOriginExpr(), Call.getResultType(), C);
   State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
                           InnerPointerVal);
   // TODO: Add NoteTag, for how the raw pointer got using 'get' method.
   C.addTransition(State);
 }
 
 bool SmartPtrModeling::handleAssignOp(const CallEvent &Call,
                                       CheckerContext &C) const {

clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp

llvm::Optional<int> tryExpandAsInteger(StringRef Macro,
   if (Size >= 2) {
     if (FilteredTokens[Size - 2].is(tok::minus))
       IntValue = -IntValue;
   }
 
   return IntValue.getSExtValue();
 }
 
+OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
+                                                 bool IsBinary) {
+  llvm::StringMap<BinaryOperatorKind> BinOps{
+#define BINARY_OPERATION(Name, Spelling) {Spelling, BO_##Name},
+#include "clang/AST/OperationKinds.def"
+  };
+  llvm::StringMap<UnaryOperatorKind> UnOps{
+#define UNARY_OPERATION(Name, Spelling) {Spelling, UO_##Name},
+#include "clang/AST/OperationKinds.def"
+  };
+
+  switch (OOK) {
+#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, MemberOnly)  \
+  case OO_##Name:                                                              \
+    if (IsBinary) {                                                            \
+      auto BinOpIt = BinOps.find(Spelling);                                    \
+      if (BinOpIt != BinOps.end())                                             \
+        return OperatorKind(BinOpIt->second);                                  \
+      else                                                                     \
+        llvm_unreachable("operator was expected to be binary but is not");     \
+    } else {                                                                   \
+      auto UnOpIt = UnOps.find(Spelling);                                      \
+      if (UnOpIt != UnOps.end())                                               \
+        return OperatorKind(UnOpIt->second);                                   \
+      else                                                                     \
+        llvm_unreachable("operator was expected to be unary but is not");      \
+    }                                                                          \
+    break;
+#include "clang/Basic/OperatorKinds.def"
+  default:
+    llvm_unreachable("unexpected operator kind");
+  }
+}
+
 } // namespace ento
 } // namespace clang

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

public:
   unique_ptr<T> &operator=(nullptr_t) noexcept;
 };
 
 // TODO :: Once the deleter parameter is added update with additional template parameter.
 template <typename T>
 void swap(unique_ptr<T> &x, unique_ptr<T> &y) noexcept {
   x.swap(y);
 }
+
+template <typename T1, typename T2>
+bool operator==(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
+template <typename T1, typename T2>
+bool operator!=(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
+template <typename T1, typename T2>
+bool operator<(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
+template <typename T1, typename T2>
+bool operator>(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
+template <typename T1, typename T2>
+bool operator<=(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
+template <typename T1, typename T2>
+bool operator>=(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
+template <typename T>
+bool operator==(const unique_ptr<T> &x, nullptr_t y);
+
+template <typename T>
+bool operator!=(const unique_ptr<T> &x, nullptr_t y);
+
+template <typename T>
+bool operator<(const unique_ptr<T> &x, nullptr_t y);
+
+template <typename T>
+bool operator>(const unique_ptr<T> &x, nullptr_t y);
+
+template <typename T>
+bool operator<=(const unique_ptr<T> &x, nullptr_t y);
+
+template <typename T>
+bool operator>=(const unique_ptr<T> &x, nullptr_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 <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 <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);
+
 } // namespace std
 #endif
 
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
   void free(void *);
 }

clang/test/Analysis/smart-ptr.cpp

 }
 
 void derefAfterBranchingOnUnknownInnerPtr(std::unique_ptr<A> P) {
   A *RP = P.get();
   if (!RP) {
     P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
   }
 }
+
+// The following is a silly function,
+// but serves to test if we are picking out
+// standard comparision functions from custom ones.
+template <typename T>
+bool operator<(std::unique_ptr<T> &x, double d);
+
+void uniquePtrComparision(std::unique_ptr<int> unknownPtr) {
+  auto ptr = std::unique_ptr<int>(new int(13));
+  auto nullPtr = std::unique_ptr<int>();
+  auto otherPtr = std::unique_ptr<int>(new int(29));
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(ptr <= ptr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);        // expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);    // expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
+
+void uniquePtrComparisionStateSplitting(std::unique_ptr<int> unknownPtr) {
+  auto ptr = std::unique_ptr<int>(new int(13));
+
+  clang_analyzer_eval(ptr > unknownPtr); // expected-warning{{TRUE}}
+  // expected-warning@-1{{FALSE}}
+}
+
+void uniquePtrComparisionDifferingTypes(std::unique_ptr<int> unknownPtr) {
+  auto ptr = std::unique_ptr<int>(new int(13));
+  auto nullPtr = std::unique_ptr<A>();
+  auto otherPtr = std::unique_ptr<double>(new double(3.14));
+
+  clang_analyzer_eval(nullPtr <= unknownPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(unknownPtr >= nullPtr); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != otherPtr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > nullPtr);   // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(ptr != nullptr);        // expected-warning{{TRUE}}
+  clang_analyzer_eval(nullPtr != nullptr);    // expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}