This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MIGChecker: Implement bug reporter visitors.
ClosedPublic

Authored by NoQ on Feb 18 2019, 9:09 PM.

Details

Summary

This adds two visitors to the checker:

  • trackExpressionValue() in order to highlight where does the return value come from when it's not a literal.
  • A tag-based visitor (as in D58367) that explains where parameters are deallocated.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 18 2019, 9:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2019, 9:09 PM
NoQ updated this revision to Diff 187502.Feb 19 2019, 7:43 PM

Rebase.

dcoughlin accepted this revision.Feb 19 2019, 9:02 PM

Looks good to me. I have some a minor diagnostic wording suggestion in line.

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
109 ↗(On Diff #187502)

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

clang/test/Analysis/mig.mm
52 ↗(On Diff #187502)

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.

This revision is now accepted and ready to land.Feb 19 2019, 9:02 PM
Charusso requested changes to this revision.Feb 20 2019, 12:57 PM
Charusso added a reviewer: Charusso.
Charusso added a subscriber: Charusso.
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
113 ↗(On Diff #187502)

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());
This revision now requires changes to proceed.Feb 20 2019, 12:57 PM
Charusso added inline comments.Feb 20 2019, 1:10 PM
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
47 ↗(On Diff #187502)

; is not necessary.

NoQ marked 3 inline comments as done.Feb 20 2019, 5:09 PM

Thx!

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
109 ↗(On Diff #187502)

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.

113 ↗(On Diff #187502)

I'll reply in D58367 because it seems to be universally relevant :)

clang/test/Analysis/mig.mm
52 ↗(On Diff #187502)

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

NoQ marked 3 inline comments as done.
NoQ added inline comments.
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
47 ↗(On Diff #187502)

Addressed in the earlier patch, D57558.

NoQ updated this revision to Diff 187873.Feb 21 2019, 3:13 PM
NoQ marked an inline comment as done.

Address comments.

@Charusso: I agreed not to rush for D58367 and implemented an old-style visitor here instead :)

This revision was not accepted when it landed; it landed in state Needs Review.Feb 21 2019, 4:06 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 4:06 PM
In D58368#1406490, @NoQ wrote:

Address comments.

@Charusso: I agreed not to rush for D58367 and implemented an old-style visitor here instead :)

Thanks you! I wanted to accept it when you remove it from the review-chain and after rewriting the summary. The latter is not that big deal, just wanted to let you know it remained the same.