Page MenuHomePhabricator

[analyzer][Review request] Partial fix for PR19102.
Needs ReviewPublic

Authored by ayartsev on Jun 4 2014, 5:40 PM.

Details

Reviewers
jordan_rose
Summary

Attached patch is a partial fix for PR19102. The idea is to assume the unused operator new as not a leak if the following requirement is met:

  • The invoked constructor has a parameter - pointer or reference to a record that has a public method (any access level , if a constructed record is a friend of a parameter record), that accepts a pointer to the type of the constructed record or one of its bases.

The analysis can be much more precise if the analyzer can analyze the body of the constructor, but for now the criteria above can be used regardless of whether the body of the constructor is accessible to the analyzer or not.
Please review.

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 10118.Jun 4 2014, 5:40 PM
ayartsev retitled this revision from to [analyzer][Review request] Partial fix for PR19102..
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: jordan_rose.
ayartsev added a subscriber: Unknown Object (MLST).
jordan_rose edited edge metadata.Jun 16 2014, 9:59 AM

This is searching all the methods on the class being passed, but what about its superclasses?

You should definitely be caching this information. I'm also just still worried that this is a very expensive search for every record parameter of every constructor, even if you collected all the types referenced by each class instead of just looking for a particular one.

ayartsev updated this revision to Diff 11541.Jul 16 2014, 3:55 PM
ayartsev edited edge metadata.

Now searching on the class and all superclasses that has methods accessible from the constructed record. Referenced types are now cached.

The search is expensive, but the case is very rare I hope. Only unassigned new are analyzed this way.

Now that I'm paging this back in...I'm not convinced. People who use unassigned new usually know what they're doing. The false positives come when someone uses new while storing the result, but they also escape the variable into storage somewhere.

For the unused result case, I wonder if a -Wunused-like warning would cover this: warn on un-consumed new, silence it by casting to void, and assume any un-consumed new has already escaped (unless we inlined the constructor or can see it is trivial).

I'd be tempted to ignore the used case except that that's blocking adoption in LLVM. We'd have to get sign-on from llvmdev if we wanted to change the new Value(module, ...) idiom they have going on there (e.g. to something like Value::create(module, ...).)

ayartsev updated this revision to Diff 11808.Jul 23 2014, 6:58 AM

Simplified the check, an un-consumed new is now treated a leak unless the constructor has a parameter of a pointer-to-record type. The check is not expensive and covers LLVM cases.

Did not added -Wunused-like new warning for now. Do you think this warning is needed apart from the checker? Doubt it ever be used. Anyway I think it is better to implement the warning as a separate patch as the change is not analyzer specific.

Is there a way to get the constructor inlined when it is called from a new expression? ExprEngine::VisitCXXConstructExpr() always left 'wasInlined' equal 'false'.

IIRC, constructors are never inlined for 'new' right now because we never got the right region, and I was always hoping we'd implement proper allocator support first (http://llvm.org/bugs/show_bug.cgi?id=12014). Karthik Bhat was working on this but I don't remember how far he got. We also didn't have destructor support for 'delete' until last fall (also thanks to Karthik).

Other than that this is looking pretty good. More tests would be nice, and possibly a test on a real code base that would otherwise have false positives. Unfortunately the only one I can think of is LLVM itself, but even that could tell us how many false positives we've gone down by.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
768

Style nitpick: * next to the variable name.

772–773

getAsCXXRecordDecl will call getCanonicalType for you, so you can skip that step.

776

Please make this const (and move the *).

785–786

I don't think you need to worry about either canonicalizing the type or stripping qualifiers.

ayartsev updated this revision to Diff 12171.Aug 4 2014, 12:19 PM
ayartsev edited the test plan for this revision. (Show Details)
ayartsev edited the test plan for this revision. (Show Details)

Done! Ok to commit?
Just ran the updated NewDeleteLeaks checker over LLVM codebase with -analyzer-opt-analyze-headers option turned on. Failed to attach reports in Phabricator, I'll send them with the next mail.

Attached are leak reports generated by NewDeleteLeaks checker ran over
the LLVM codebase with -analyzer-opt-analyze-headers option turned on.
Haven't addressed them yet. Looking at suspicious leak reports generated
from headers (e.g. reports from TinyPtrVector.h).

You're still missing tests that show that the leaks are reported no matter what the arguments are if the newly-created instance is consumed. But other than that this looks good; please commit once you've added those!

(Yes, I know they're present elsewhere in the test suite, but it's good to have some explicit ones here.)

Committed as r214909.