[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D97183#2640559 <https://reviews.llvm.org/D97183#2640559>, @RedDocMD wrote:

> @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?

When the if's condition is evaluated, it probably triggered a state split. On one path the `aptr` (aka. the inner pointer) will be constrained to `null`.
The only way to be sure is by checking the exploded graph and see where it goes.

---

In D97183#2643671 <https://reviews.llvm.org/D97183#2643671>, @RedDocMD wrote:

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

All in all, I see where it's going. I don't know, it might be the right thing to do. I haven't spent much time on this topic though.
See my inline comments.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:87
+  const ProgramStateRef BugState;
+  llvm::SmallPtrSet<const Expr *, 16> DoneExprs;
+
----------------
I'm not sure if we should expect 16 unique places where `uptr::get()` called on a path. I would guess 4 or 2 is more than enough.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:106-109
+    if (const auto *DS = llvm::dyn_cast<DeclStmt>(S)) {
+      for (const Decl *D : DS->getDeclGroup()) {
+        if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) {
+          const Expr *Init = VD->getInit();
----------------
So you are trying to find the assignment, where the inner pointer is assigned to a variable.
This visitor logic seems to be somewhat convoluted.

What you want to achieve is slightly similar to `FindLastStoreBRVisitor`. You should have a look at that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:460
   SVal InnerPointerVal;
+  const auto *CallExpr = Call.getOriginExpr();
   if (const auto *InnerValPtr = State->get<TrackedRegionMap>(ThisRegion)) {
----------------
Nit: Declare the variable as close to the usage as you can. In the narrowest scope as well.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:475-476
+  if (const auto *StmtSet = State->get<ExprsFromGet>(ThisRegion)) {
+    auto NewStmtSet = StmtSetFactory.add(*StmtSet, CallExpr);
+    State = State->set<ExprsFromGet>(ThisRegion, NewStmtSet);
+  } else {
----------------
Why don't you 'save' the MemRegion only if the inner pointer is not proven to be null. This would relieve you from checking it later.

Nit: I don't like such if branches. The last statement is identical, which is a code smell.
It's better to think of this as a function taking stuff and producing a State.
An immediately called lambda expression would enclose any local variables and it would suggest that the algorithm it implements is self-contained.
I know that I'm the only immediately called lambda expression fan though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97183/new/

https://reviews.llvm.org/D97183