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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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

> Important question from @vsavchenko:
>
>> I have two major questions about this implementation:
>>
>> - Why don't we need an actual check for `IfStmt`? Won't it trigger on `bool unused = !pointer;`? And if so it won't mean **constrained**.
>> - Why do we only care about implicit pointer-to-bool conversion? What about situations like `pointer == nullptr`, `NULL != pointer`, `__builtin_expect(pointer, 0)`, etc?

I think there's no way around re-using/generalizing the logic from `ConditionBRVisitor::VisitNode` in some form. I guess you could try to separate the part where it looks at the current program point and finds out what's constrained. Then apply it to the moment of time where the interesting constraint appears (whereas `ConditionBRVisitor` continously scans all program points with the same hopefully-reusable logic).



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:258
+  const Decl *D = DS->getSingleDecl();
+  assert(D && "DeclStmt should have at least one Decl");
+  const auto *VD = llvm::dyn_cast<VarDecl>(D);
----------------
RedDocMD wrote:
> RedDocMD wrote:
> > NoQ wrote:
> > > That's not what the assert is saying; the assert is saying that the `DeclStmt` has //exactly// one `Decl`. It basically forbids code like
> > > ```
> > > int x = 1, y = 2;
> > > ```
> > > . You may wonder why don't you crash all over the place. That's because Clang CFG [[ https://github.com/llvm/llvm-project/blob/llvmorg-12.0.0/clang/lib/Analysis/CFG.cpp#L2826 | creates its own ]] `DeclStmt`s that aren't normally present in the AST, that always have exactly one declaration. This is necessary because there may be non-trivial control flow between these declarations (due to, say, presence of operator `?:` in the initializer) so they have to be represented as different elements (possibly even in different blocks) in the CFG.
> > So I guess the tests at lines `317` and `378` of `smart-ptr-text-output.cpp` work because of the CFG messing with the AST? So should I remove the assert?
> @NoQ?
> So I guess the tests at lines 317 and 378 of smart-ptr-text-output.cpp work because of the CFG messing with the AST?

Yes. The rest of the static analyzer works for the same reason; a lot of code relies on it.

> So should I remove the assert?

The assert is correct but the message is wrong / misleading.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183