Page MenuHomePhabricator

[clang-tidy] Warn pointer captured in async block
ClosedPublic

Authored by ellis on Jun 30 2020, 12:08 PM.

Details

Summary

The block arguments in dispatch_async() and dispatch_after() are
guaranteed to escape. If those blocks capture any pointers with the
noescape attribute then it is an error.

Test Plan:
ninja check-clang-tools

Diff Detail

Event Timeline

ellis created this revision.Jun 30 2020, 12:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2020, 12:08 PM
ellis added a reviewer: Restricted Project.Jun 30 2020, 12:12 PM

Please override isLanguageVersionSupported to restrict this check to just running on ObjectiveC translation units.

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
34

Don't use auto here as type isn't spelled out explicitly in the initializer.

35

Ditto, also could this be a reference to avoid an unnecessary copy?

39

Convention is warnings start with a lower case letter, same goes for the note below.

41

You can get rid of ->getName() and just pass Var, The diagnostics builder has a special case for const NamedDecl*

clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
26

Be explicit about the column number and warning message. You don't need to include the diagnostic name though.
Also the macro should be [[@LINE-2]].

Eugene.Zelenko edited reviewers, added: alexfh, hokein, aaron.ballman, njames93; removed: Restricted Project.Jun 30 2020, 2:31 PM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
100

Please use double back-ticks for language constructs.

ellis updated this revision to Diff 274618.Jun 30 2020, 2:38 PM

Applied changes suggested in comments

ellis updated this revision to Diff 274622.Jun 30 2020, 2:48 PM

Add double backtick to doc

ellis marked 6 inline comments as done.Jun 30 2020, 4:11 PM
ellis marked an inline comment as done.Jun 30 2020, 4:37 PM
ellis added inline comments.
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
125

Not sure if we want to lint this line because it was generated by clang-tidy/add_new_check.py and every other registerCheck line is formatted in the same way.

njames93 added inline comments.Jul 1 2020, 8:26 AM
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
125

They should be formatted, but we don't pipe the changes through clang-format in the add_new_check.py script. only 8/21 clang-tidy module files are incorrectly formatted.

ellis updated this revision to Diff 274855.Jul 1 2020, 9:47 AM

Add isLanguageVersionSupported for Objective-C

ellis marked an inline comment as done.Jul 1 2020, 9:48 AM
aaron.ballman added inline comments.Jul 6 2020, 10:24 AM
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
83

It looks like unrelated formatting changes may have snuck in?

124

Given that this is limited to Objective-C, why register this under bugprone as opposed to the objc module? Alternatively, why limit to Objective-C when blocks can be used in other modes like C or C++ with -fblocks?

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
35

This makes me think we should extend the hasAnyCaptures() AST matcher so it works with blocks as well as lambdas. It would be nice to do all of this from the matcher interface.

38

Given that the capture is the issue (not the block), why not point to the use of the captured variable?

njames93 added inline comments.Jul 6 2020, 11:48 AM
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
124

Thats a good point, maybe restrict this to LangOptions::ObjC || LangOptions::Blocks Then it can be left in bugprone.

ellis marked an inline comment as done.Jul 6 2020, 12:03 PM
ellis added inline comments.
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
83

The script add_new_check.py doesn't format the code that it generated so a few lines aren't linted. Would you like me to undo these extra formatted lines?

124

Ok, I'll include LangOptions::Blocks.

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
38

I actually agree that pointing to the use of the captured variable would be easier to read, but honestly I couldn't figure out how to grab the location of that use. All I could get was the definition of the variable.

aaron.ballman added inline comments.Jul 6 2020, 12:08 PM
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
83

Yes, please. The way I usually do this, btw, is to run the patch through clang-format-diff so that only the lines I've touched get formatted: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

124

I think the only option you need is LangOptions::Blocks (it should be enabled automatically in ObjC language mode).

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
38

Ah, yeah, I just looked at the class and it looks like we don't track that information yet and it would be somewhat involved to get it. Can you add a FIXME comment above the call to diag() mentioning that we'd prefer to diagnose the use of the capture rather than the block?

ellis updated this revision to Diff 275826.Jul 6 2020, 2:02 PM
ellis marked 9 inline comments as done.

Use LangOpts.Blocks instead of LangOpts.ObjC and add FIXME about grabbing the use location of a CapturedVar.

ellis added inline comments.Jul 6 2020, 2:06 PM
clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
35

Should I add a TODO for this?

aaron.ballman accepted this revision.Jul 7 2020, 7:33 AM

LGTM with a testing request.

clang-tools-extra/clang-tidy/bugprone/NoEscapeCheck.cpp
35

Naw, I don't think it's that critical. I'm mostly just surprised we haven't done that before now given how similar blocks and lambdas are.

clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
2

Can you add an additional RUN line so we get coverage of the blocks-enabled behavior? Something like:

// RUN: %check_clang_tidy %s bugprone-no-escape %t -- -- -fblocks -x c

I'm not 100% certain I have that syntax right, but the idea is to run the test as though it were a C compilation unit with blocks explicitly enabled.

This revision is now accepted and ready to land.Jul 7 2020, 7:33 AM
njames93 added inline comments.Jul 7 2020, 8:00 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-no-escape.m
2

check_clang_tidy will add -fobjc-abi-version=2, -fobjc-arc and -fblocks if the file extension is .m or .mm.
You can trick it with assume-filename to stop that happening

// RUN: %check_clang_tidy %s -assume-filename=bugprone-no-escape.c bugprone-no-escape %t -- -- -fblocks

Not 100% certain that is the right syntax but that feels like the designed way to run the test as a C compilation unit

ellis updated this revision to Diff 276095.Jul 7 2020, 9:09 AM

Add RUN line to test in C

ellis marked 2 inline comments as done.Jul 7 2020, 9:11 AM
ellis updated this revision to Diff 276118.Jul 7 2020, 10:11 AM

Update commit message

plotfi edited the summary of this revision. (Show Details)Jul 7 2020, 10:16 AM
plotfi edited the summary of this revision. (Show Details)
ellis added a comment.Jul 7 2020, 10:23 AM

Hey @aaron.ballman, thanks for accepting my diff! Would you mind landing my diff since I don't have commit access yet?

aaron.ballman closed this revision.Jul 7 2020, 10:32 AM

Hey @aaron.ballman, thanks for accepting my diff! Would you mind landing my diff since I don't have commit access yet?

Gladly -- I've committed on your behalf in dfa0db79d0e37d5cf24a63d1e2b7ba5f40617574, thank you for the patch!

ellis marked an inline comment as done.Jul 7 2020, 10:52 AM
ellis added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
13

It looks like I triggered a build failure for the clang-tools docs http://lab.llvm.org:8011/builders/clang-tools-sphinx-docs/builds/62418

I think the fix is to add a new line here, but I haven't been able to test it locally.

aaron.ballman added inline comments.Jul 7 2020, 10:55 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-no-escape.rst
13

I pushed that fix up before getting your email. I'll keep an eye on the bot to ensure the fix worked. Thanks!