This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Syntactic matcher for leaks associated with run loop and autoreleasepool
ClosedPublic

Authored by george.karpenkov on Jul 18 2018, 5:55 PM.

Details

Summary

A checker for detecting leaks resulting from allocating temporary
autoreleasing objects before starting the main run loop.

Checks for two antipatterns:
1. ObjCMessageExpr followed by [[NARunLoop mainRunLoop] run] in the same
autorelease pool.
2. ObjCMessageExpr followed by [[NARunLoop mainRunLoop] run] in no
autorelease pool.

Happens-before relationship is modeled purely syntactically.

rdar://39299145

Diff Detail

Event Timeline

NoQ added inline comments.Jul 18 2018, 6:35 PM
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
61

*makes a joke about QuadBoolTy and runs away*

89

I wonder if we could make a reusable matcher out of this: hasDescendantsInOrder(A, ..., B).

90

This memoization map is short-lived. I don't quite understand how would we ever encounter the same statement twice during our traversal, could you give an example? Like, you're traversing the AST in a manner in which RecursiveASTVisitors usually do that, and i'm pretty sure that they encounter every statement just once, and you throw away the map immediately after that. What am i missing?

92

So is the optional here only to add an assertion? It's essentially a non-optional in release mode(?) Maybe just assert in the callee that we never return None?

189–190

That'd match non-functions and C++ methods and stuff like that.

In the non-matcher world i'd have checked FunctionDecl::isMain() here, but i don't think we have a matcher for that, but i guess we may add it.

clang/test/Analysis/html_diagnostics/relevant_lines/macros_same_file.c
16 ↗(On Diff #156198)

I guess this is accidental.

NoQ added inline comments.Jul 18 2018, 6:48 PM
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
89

Aha, i guess you're already discussing it on the mailing list, i didn't intend to block the patch on that.

george.karpenkov marked 3 inline comments as done.Jul 19 2018, 3:36 PM
george.karpenkov added inline comments.
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
90

i think we've discussed that some statements can still be DAGs (e.g. elvis operator)

NoQ accepted this revision.Jul 23 2018, 6:17 PM

Looks good!

clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
90

Yeah, apparently it has as many as 4 (!!) sub-expressions, 3 of which partially overlap. Still not sure it's worth the memoization, but let's add a comment explaining why we did that.

This revision is now accepted and ready to land.Jul 23 2018, 6:17 PM

It is great to see a check for this!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
572

Mentioning "loops" three times here doesn't seem right.

"Check for leaked memory in autorelease pools that will never be popped"

clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
13

"autoreleasing" --> "autoreleased"

20

I think it would be good to explain why this pattern may lead to leaks/abandoned memory.

Something like: "Any temporary objects autoreleased in code called in the expression will not be deallocated until the program exits and so are effectively leaks."

134

Should "mainRunLoop" really be camelCase? Does "mainRunLoop" make sense when the call is to xpc_main?

Why not just "the main run loop"?

135

The comma here should be a semi-colon to match our other diagnostics: "may never get released; consider moving them to a".

dcoughlin added inline comments.Jul 23 2018, 9:53 PM
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
16

Also note the typo: "NARunLoop" --> "NSRunLoop"

george.karpenkov marked 5 inline comments as done.
This revision was automatically updated to reflect the committed changes.