[PATCH] D60112: [analyzer] Treat write into a top-level parameter variable with destructor as escape.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added a comment.

In D60112#1451198 <https://reviews.llvm.org/D60112#1451198>, @Szelethus wrote:

> Woah, the code looks amazing, cheers on the refactoring! I'll be honest, I'm struggling a bit with the sentence "we're now in the top frame". In order, I don't understand what does
>
> - we
> - now
> - in the top frame mean. "Top-level argument" is another one -- Do we have **precise** definitions for there terms?


Cf. `LocationContext::inTopFrame()`.

The top frame is the `StackFrameContext` whose parent context is `nullptr`. There is only one such context and it is the root of the location context tree. It corresponds to the stack frame from which the current Analysis begins. That is, each such context corresponds to a path-sensitive analysis line in the `-analyzer-display-progress` dump. Other stack frame contexts (with non-null parents) correspond to function calls *during* analysis. They usually correspond to stack frames of inlined calls (since D49443 <https://reviews.llvm.org/D49443> we sometimes create stack frame contexts for calls that will never be inlined, but the rough idea is still the same).

So this patch talks about the situation when we start analysis from function, say, `test(A a) { ... }`, and its argument `a` exists throughout the whole analysis: its constructor was called before the analysis has started, and its destructor will be called after the analysis has ended. Having no parent context means that we don't know anything about the caller or the call site of `test(a)` - the information we usually do have for inlined calls.

The phrase "We are now in the top frame" therefore roughly translates to "The `CoreEngine` worklist item that the `ExprEngine` is currently processing corresponds to an `ExplodedNode` that has a `ProgramPoint` with a `LocationContext` whose nearest `StackFrameContext` is the top frame". When i'm talking about top-level variables, i mean "A `VarRegion` whose parent region is a `StackArgumentsSpaceRegion` whose identity is a top-frame `StackFrameContext`".

Do you think i should document it somehow?



================
Comment at: clang/test/Analysis/malloc.cpp:151
+  char *getName() {
+    if (!name) {
+      name = static_cast<char *>(malloc(10));
----------------
Szelethus wrote:
> Is this relevant? `name` will never be null.
Not really, just makes the code look a bit more sensible and idiomatic and less warning-worthy-anyway, to make it as clear as possible that the positive here is indeed false. We don't really have a constructor in this class, but we can imagine that it zero-initializes name. Without this check calling `getName()` multiple times would immediately result in a leak.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60112