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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

A brief summary of an offline discussion we recently had.

(1) Basically we figured out that it's still necessary to do something like I originally suggested:

In D97183#2598806 <https://reviews.llvm.org/D97183#2598806>, @NoQ wrote:

> We could, for instance, teach it to mark //exploded nodes// as interesting when it changes tracking mode. That'd be a completely new form of interestingness for us to track. Or maybe pairs (exploded node, expression) so that to be more specific. Then we could query it from our note tag.

This is necessary because the symbol produced by `.get()` is not immediately collapsed to a constant and it remains interesting as a symbol for the entire duration of the new visitor's lifetime, but there may be unrelated `.get()`s on the same smart pointer during said lifetime that don't deserve a note despite producing the same symbol.

(2) We also came up with a different approach to communicating with `trackExpressionValue()`. First of all, we probably don't need to mark all nodes/expressions on which `trackExpressionValue()` switches modes as interesting; we're only interested in the spot where tracking //ends//. This happens because the checker fully models `.get()` and therefore it's impossible for a generic solution like `trackExpressionValue()` to proceed with tracking as that would have required checker-specific machinery. We could reduce the scope of proposal (1) by only marking the last node as interesting but I have a better idea: let's add a callback to `trackExpressionValue()` that's invoked once tracking ends. In our case such callback would attach a checker-specific visitor to the smart pointer which solves our problem perfectly.

Such callback could be useful in a lot more cases though, because it provides us with an extremely generic benefit of //knowing the origin of the value//. We already demand such knowledge in a number of other machines that are currently hard-coupled to `trackExpressionValue()`: namely, i'm talking about inlined defensive check suppressions. Both of these suppressions basically say "if a null/zero value //originates// from a nested function call that was exited before the bug node, suppress the warning". These suppressions don't care where the value was passing through, they only care where it originated from. As such, by providing a callback for the origin of the value, we could decouple these suppressions and possibly even move them into the respective checkers (eg., the null dereference checker). I think this could be an excellent refactoring pass.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:61-64
+  PathDiagnosticPieceRef visitAssgnStmt(const ExplodedNode *Node,
+                                        const ProgramStateRef State,
+                                        BugReporterContext &BRC, const Stmt *S,
+                                        const SVal InnerPtrVal);
----------------
Typo!


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183