[PATCH] D105819: [analyzer] MallocChecker: Add a visitor to leave a note on functions that could have, but did not change ownership on leaked memory

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:801
+    // must be a superset of, but not necessarily equal to ExitOwners.
+    return !llvm::set_is_subset(ExitOwners, CurrOwners);
+  }
----------------
NoQ wrote:
> Can you also comment on what's, generally, the default scenario / motivating example where this is necessary? What makes you hunt down store bindings that didn't cause an escape to happen (given that an escape would have been a state change)? IIUC this is for the situation when the callee stores the pointer in a caller-local variable and in this case you don't want to claim that ownership didn't change?
The comment above is meant to explain it. 
    // [...] Any pointers
    // inside the call that pointed to the allocated memory are of little
    // consequence if their lifetime ends before within the function


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:832-833
+      ArrayRef<ParmVarDecl *> Parameters, unsigned ParamIdx) override {
+    // TODO: Check whether the allocated memory was actually passed into the
+    // function.
+    return emitNote(N);
----------------
NoQ wrote:
> How difficult would it be to re-use this part as well?
Ugh, I gave this a shot, and its worse then expected. I think the original patch that George made was intended to be used by `NoStoreFuncVisitor`, and nothing else. As a consequence, its practically impossible to factor out without rewriting the whole thing.

At this point, I'm more in favor of just biting the bullet, and reuse how pointees and casts are printed from D50506, and maybe some of the dereferencing technology as well. It might be sliiiiiightly slower, but the majority of the price is only paid when `FieldChainInfo` is printed, not when its constructed.

For the time being, I made a rather primitive implementation here, which should lean on the conservative side (we under approximate the set of function calls where the allocated memory was actually passed into).


================
Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:104-106
+// TODO: We don't want a note here. sink() doesn't seem like a function that
+// even attempts to take care of any memory ownership problems.
+namespace memory_passed_into_fn_that_doesnt_intend_to_free {
----------------
NoQ wrote:
> How do you think this is different from the very first test, `memory_allocated_in_fn_call()`? Is this just because of how the pointer is put into a variable first? This function is kinda still the only place that could free memory.
That was my idea, yes, but you have a point there... 


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

https://reviews.llvm.org/D105819