Page MenuHomePhabricator

[Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ClosedPublic

Authored by ahatanak on Apr 15 2019, 3:36 PM.

Details

Summary

If the block implicitly referencing self doesn't escape, there is no risk of creating retain cycles, so clang shouldn't diagnose it and force users to add self-> to silence the diagnostic.

Also, fix a bug where clang was failing to diagnose self referenced inside a block that was nested inside a c++ lambda.

rdar://problem/25059955

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Apr 15 2019, 3:36 PM

Sorry, clang was failing to diagnose self referenced inside a c++ lambda that was nested inside a block.

Akira and I were just talking about an alternative approach to this: Keep a vector of pairs of BlockDecls and SourceLocations in the enclosing method's FunctionScopeInfo, and emit any unsuppressed diagnostics when popping the method. This would avoid having to traverse all the blocks in methods in ARC mode, at the cost of a small amount of memory.

lib/Sema/Sema.cpp
1682–1687 ↗(On Diff #195262)

Maybe call this "diagnoseBlockDecl" or something so it doesn't sounds like a CRTP-overridden method.

ahatanak updated this revision to Diff 195498.Apr 16 2019, 5:58 PM

Keep a list of pairs of BlockDecl and SourceLocation and, after the body of an ObjC method is parsed, emit a diagnostic if a SourceLocation in the list is inside an escaping block.

rjmccall added inline comments.Apr 16 2019, 8:41 PM
lib/Sema/SemaExpr.cpp
2580 ↗(On Diff #195498)

IIRC this check can be expensive enough that it's probably not worth doing if you expect these entries to typically not result in diagnostics.

ahatanak updated this revision to Diff 195589.Apr 17 2019, 9:50 AM
ahatanak marked 2 inline comments as done.

Remove a potentially expensive check.

lib/Sema/SemaExpr.cpp
2580 ↗(On Diff #195498)

DiagStateMap::lookup is doing a binary search. Is that what makes this check expensive?

rjmccall added inline comments.Apr 17 2019, 10:06 AM
include/clang/AST/DeclBase.h
1798 ↗(On Diff #195589)

"innermost" is one word, so this should be getInnermostBlockDecl.

lib/Sema/SemaExpr.cpp
2580 ↗(On Diff #195498)

Yeah. It's not immensely expensive, but adding some entries to a vector in the fast path and then processing them later probably makes more sense.

ahatanak updated this revision to Diff 195601.Apr 17 2019, 10:59 AM
ahatanak marked an inline comment as done.

Rename function.

lib/Parse/ParseObjc.cpp
3696–3699 ↗(On Diff #195498)

Any reason not to do this check in ActOnFinishFunctionBody (which is called by ParseFunctionStatementBody)? Seems like there are some similar diagnostics implemented there.

rjmccall accepted this revision.Apr 17 2019, 12:27 PM

Alright, LGTM.

This revision is now accepted and ready to land.Apr 17 2019, 12:27 PM
ahatanak updated this revision to Diff 195639.Apr 17 2019, 3:22 PM

Diagnose implicitly retained self in ActOnFinishFunctionBody.

erik.pilkington accepted this revision.Apr 17 2019, 3:27 PM

LGTM too, thanks!

lib/Sema/SemaDecl.cpp
13169 ↗(On Diff #195639)

This can just be R = !CurBD->doesNotEscape();

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 4:13 PM