[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 3 inline comments as done.
NoQ added a comment.

Thx!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:109
+    llvm::raw_svector_ostream OS(Str);
+    OS << "Deallocating object passed through parameter '" << PVD->getName()
+       << '\'';
----------------
dcoughlin wrote:
> Could we just have the note say "'name' is deallocated"?
> 
> Or "Value passed through parameter 'name' is deallocated"
> 
> The ".. is ... " construction matches our other checkers. (Like "Memory is released" from the Malloc Checker.)
Great point! I'd pick the latter because it's important to point out that the value is loaded from the parameter.

Hmm, btw, we should probably add more "visitors" in order to explain that the value is indeed copied from the parameter.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:113
+  });
+  C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
 }
----------------
Charusso wrote:
> This is a cool approach, but it is difficult to use this API in other checkers. If you do not out-chain D58367 I would like to see something like that here:
> 
> ```
>   SmallString<64> Str;
>   llvm::raw_svector_ostream OS(Str);
>   OS << "Deallocating object passed through parameter '" << PVD->getName()
>      << '\'';
> 
>   C.addNote(C.getState()->set<ReleasedParameter>(true), OS.str());
> ```
I'll reply in D58367 because it seems to be universally relevant :)


================
Comment at: clang/test/Analysis/mig.mm:52
+kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1, vm_address_t addr2, vm_size_t size) {
+  kern_return_t ret = KERN_ERROR; // expected-note{{'ret' initialized to 1}}
+  vm_deallocate(port, addr1, size); // expected-note{{Deallocating object passed through parameter 'addr1'}}
----------------
dcoughlin wrote:
> A nice QoI improvement here (for a later patch, perhaps) would be to have this note use the macro name: "'ret initialized to KERN_ERROR'".
> 
> Users probably won't know that KERN_ERROR is 1.
Yup, but that's a separate story, because this message is produced by a generic, checker-inspecific visitor. We'll have to teach ~~`trackNullOrUndefValue()`~~ `trackExpressionValue()` to be aware of macros, and it might turn out that we'd also want to mention macro names in messages that correspond to the subsequent copies of the same value (which, in turn, is tricky as it causes time paradoxes due to visiting order).


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

https://reviews.llvm.org/D58368