[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+    ProgramStateRef State;
----------------
xazax.hun wrote:
> NoQ wrote:
> > xazax.hun wrote:
> > > NoQ wrote:
> > > > WDYT about re-using `ExprEngine::escapeValue()` by changing it to accept `ArrayRef<SVal>` instead of a single `SVal`?
> > > It is not entirely the same. Here we only collect symbols from non-stack regions. There (as far as I understand) we collect all symbols. I could add a flag but at that point I wonder if it is worth the change.
> > Umm, wait, right, so what are you doing in this callback to begin with? The code says "gather all symbols that are encountered as immediate values stored in traversed regions". Why not simply "gather all symbols that are traversed from parameter regions"?
> My understanding is the following but correct me if I am wrong:
> 
> ```
> int *getConjuredSymbol();
> 
> call(getConjuredSymbol());
> ```
> 
> So we have can talk about two symbols here. One symbol is what was returned by `getConjuredSymbol` and the other is the pointee, the symbol that we get when we dereference the result of `getConjuredSymbol`. And my understanding is that we want to invoke escape for the latter and not the former. `ExprEngine::escapeValue` invalidates the former but not the latter. The visitor here invalidates the latter but not the former.  This behavior solves most of my test cases in FuchsiaHandleChecker. 
How about `escapeValue(getSVal(getArgSVal()))`? (the type for `getSVal()` could be obtained from the AST).


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

https://reviews.llvm.org/D71224