Page MenuHomePhabricator

[clang-tidy] New check to warn when storing dispatch_once_t in non-static, non-global storage
ClosedPublic

Authored by mwyman on Sep 13 2019, 12:02 PM.

Details

Summary

Creates a new darwin ClangTidy module and adds the darwin-dispatch-once-nonstatic check that warns about dispatch_once_t variables not in static or global storage. This catches a missing static for local variables in e.g. singleton initialization behavior, and also warns on storing dispatch_once_t values in Objective-C instance variables. C/C++ struct/class instances may potentially live in static/global storage, and are ignored for this check.

The osx.API static analysis checker can find the non-static storage use of dispatch_once_t; I thought it useful to also catch this issue in clang-tidy when possible.

Diff Detail

Repository
rL LLVM

Event Timeline

mwyman created this revision.Sep 13 2019, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 12:02 PM
  1. Please split each check into separate review.
  2. Is dispatch_once_t OSX-specific thing? Should those checks be in osx module?
mwyman updated this revision to Diff 220169.Sep 13 2019, 2:06 PM
mwyman retitled this revision from New ClangTidy checks to warn about misusing dispatch_once_t to New ClangTidy checks to warn when storing dispatch_once_t in non-static, non-global storage.

Moved the assignment check to a separate review.

mwyman retitled this revision from New ClangTidy checks to warn when storing dispatch_once_t in non-static, non-global storage to New ClangTidy check to warn when storing dispatch_once_t in non-static, non-global storage.Sep 13 2019, 2:08 PM
mwyman edited the summary of this revision. (Show Details)
  1. Please split each check into separate review.
  2. Is dispatch_once_t OSX-specific thing? Should those checks be in osx module?
  1. I split the review.
  2. I don't see an osx module—am I somehow missing seeing it? dispatch_once_t is something from libdispatchwhich is an Apple-created API, but is OSS and available outside Apple. It's also a C API, which is why I didn't feel objc was an appropriate module either. I'm open to a better home module for this check, it's just not clear to me where that might be.
  1. Please split each check into separate review.
  2. Is dispatch_once_t OSX-specific thing? Should those checks be in osx module?
  1. I split the review.
  1. I don't see an osx module—am I somehow missing seeing it?

One can be added.

dispatch_once_t is something from libdispatchwhich is an Apple-created API, but is OSS and available outside Apple. It's also a C API, which is why I didn't feel objc was an appropriate module either. I'm open to a better home module for this check, it's just not clear to me where that might be.

No opinion on my question, just thought it should be asked,

It definitely shouldn't be in an osx directory since it's available on iOS and other Darwin-like operating systems.

Probably darwin would be OK. Adding a new subdirectory isn't trivial, there's a lot of places to update last I looked.

Should we tackle moving things around in a separate diff?

  1. Please split each check into separate review.
  2. Is dispatch_once_t OSX-specific thing? Should those checks be in osx module?
  1. I split the review.
  1. I don't see an osx module—am I somehow missing seeing it?

One can be added.

dispatch_once_t is something from libdispatchwhich is an Apple-created API, but is OSS and available outside Apple. It's also a C API, which is why I didn't feel objc was an appropriate module either. I'm open to a better home module for this check, it's just not clear to me where that might be.

No opinion on my question, just thought it should be asked,

As Ben commented, I don't feel this should be in an osx module, as libdispatch is highly used on iOS and other Apple platforms. I know it's available elsewhere, but it may be so seldom used that I think a darwinmodule might be reasonable. I looked at the process for creating a new module, and can do that if we'd like, but should that be part of this same review?

Eugene.Zelenko retitled this revision from New ClangTidy check to warn when storing dispatch_once_t in non-static, non-global storage to [clang-tidy] New check to warn when storing dispatch_once_t in non-static, non-global storage.Sep 13 2019, 4:34 PM
Eugene.Zelenko added a project: Restricted Project.
NoQ added a subscriber: NoQ.EditedSep 13 2019, 6:53 PM

FTR, we already have a similar Static Analyzer check, eg.:
https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L15
https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L26

Your check is a bit more aggressive but i don't see why didn't we do it that way in the first place :) Though you won't be able to warn on the heap example.

In D67567#1670264, @NoQ wrote:

FTR, we already have a similar Static Analyzer check, eg.:
https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L15
https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L26

Your check is a bit more aggressive but i don't see why didn't we do it that way in the first place :) Though you won't be able to warn on the heap example.

The Static Analyzer check was pointed out by a colleague; unfortunately our build environment doesn't currently play nice with running the static analyzer (so many devs don't end up running it) but ClangTidy gets run as part of our code review process. Given libdispatch's documented requirements, it seemed reasonable to be aggressive with a ClangTidy check when we can reasonably identify non-static/global storage.

In D67567#1670264, @NoQ wrote:

FTR, we already have a similar Static Analyzer check, eg.:
https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L15
https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L26

Your check is a bit more aggressive but i don't see why didn't we do it that way in the first place :) Though you won't be able to warn on the heap example.

The Static Analyzer check was pointed out by a colleague; unfortunately our build environment doesn't currently play nice with running the static analyzer (so many devs don't end up running it) but ClangTidy gets run as part of our code review process. Given libdispatch's documented requirements, it seemed reasonable to be aggressive with a ClangTidy check when we can reasonably identify non-static/global storage.

You can run static analyzer checks as normal clang-tidy checks.

mwyman updated this revision to Diff 220357.Sep 16 2019, 10:24 AM
mwyman edited the summary of this revision. (Show Details)

Migrated check to new darwin module.

  1. Is dispatch_once_t OSX-specific thing? Should those checks be in osx module?
  1. I don't see an osx module—am I somehow missing seeing it?

One can be added.

dispatch_once_t is something from libdispatchwhich is an Apple-created API, but is OSS and available outside Apple. It's also a C API, which is why I didn't feel objc was an appropriate module either. I'm open to a better home module for this check, it's just not clear to me where that might be.

No opinion on my question, just thought it should be asked,

As Ben commented, I don't feel this should be in an osx module, as libdispatch is highly used on iOS and other Apple platforms. I know it's available elsewhere, but it may be so seldom used that I think a darwinmodule might be reasonable. I looked at the process for creating a new module, and can do that if we'd like, but should that be part of this same review?

As alluded to, technically Grand Central Dispatch is documented to support FreeBSD. With that said, I would be okay with putting this in a darwin module because GCD is most prevalent on Darwin platforms.

clang-tools-extra/test/clang-tidy/darwin-dispatch-once-nonstatic.mm
45 ↗(On Diff #220357)

Maybe add a CHECK-FIXES to make sure that no fix was recommended?

alexfh edited reviewers, added: gribozavr; removed: alexfh.Sep 18 2019, 6:28 AM
gribozavr accepted this revision.Sep 18 2019, 10:35 AM

FWIW, I like this approach better than the one in Static Analyzer because it warns on the variable declaration -- that's where the root of the issue is, not at the call to dispatch_once(). Like, what can you in principle do with a local dispatch_once_t? not much -- therefore produce a warning right there.

clang-tools-extra/docs/clang-tidy/checks/darwin-dispatch-once-nonstatic.rst
10 ↗(On Diff #220357)

s/paradigm/pattern/

This revision is now accepted and ready to land.Sep 18 2019, 10:35 AM
mwyman updated this revision to Diff 220751.Sep 18 2019, 2:23 PM

Addressed review comments.

mwyman marked 2 inline comments as done.Sep 18 2019, 2:24 PM

Thanks! Do you need me to commit the patch for you?

Thanks! Do you need me to commit the patch for you?

Yes, thank you. I don't have commit access—and also wasn't sure if anyone else had further comment.

Sorry, could you rebase the patch to apply cleanly to master? Seems like someone else edited ReleaseNotes.rst in the meanwhile.

$ arc patch D67567
...
Checking patch clang-tools-extra/docs/ReleaseNotes.rst...
error: while searching for:
  Finds instances where variables with static storage are initialized
  dynamically in header files.
mwyman updated this revision to Diff 222016.Sep 26 2019, 1:09 PM

Rebased patch to apply to current master.

Sorry, could you rebase the patch to apply cleanly to master? Seems like someone else edited ReleaseNotes.rst in the meanwhile.

$ arc patch D67567
...
Checking patch clang-tools-extra/docs/ReleaseNotes.rst...
error: while searching for:
  Finds instances where variables with static storage are initialized
  dynamically in header files.

Just rebased, resolved the conflict and uploaded, should be good.

Looks good! A couple nits/questions.

Does the check allow variables in anonymous namespaces?

namespace {
dispatch_once_t onceToken;
}

I think such variables should satisfy initialization requirements.

If not, can we update the check to not trigger on variables? Either way, can we add a test to verify behavior for variables in anonymous namespaces?

clang-tools-extra/clang-tidy/darwin/CMakeLists.txt
13 ↗(On Diff #222016)

Is this library necessary?

16 ↗(On Diff #222016)

Is this library necessary?

clang-tools-extra/clang-tidy/darwin/DispatchOnceNonstaticCheck.cpp
44 ↗(On Diff #222016)

"dispatch_once_t variables" to be consistent.

clang-tools-extra/docs/clang-tidy/checks/darwin-dispatch-once-nonstatic.rst
15 ↗(On Diff #222016)

Maybe rephrase to this:

``dispatch_once_t`` variables
16 ↗(On Diff #222016)

"structs or classes"

Does the check allow variables in anonymous namespaces?

namespace {
dispatch_once_t onceToken;
}

I think such variables should satisfy initialization requirements.

If not, can we update the check to not trigger on variables? Either way, can we add a test to verify behavior for variables in anonymous namespaces?

Nevermind, I overlooked that there is already a variable in an anonymous namespace in the test 😅 Thanks for that!

stephanemoore accepted this revision.Sep 26 2019, 2:58 PM
mwyman updated this revision to Diff 222036.Sep 26 2019, 3:34 PM
mwyman marked 4 inline comments as done.

Addressed Stephane's review feedback.

mwyman marked an inline comment as done.Sep 26 2019, 3:34 PM

It looks like everything is in order. I will proceed with landing the patch on your behalf 👍

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

Sorry, I reverted it in r373032 because the test fails on Linux: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/18323 . Could you please take a look? Thanks!

I had a fix, but I was too slow. Here's what you need for the reland: http://codepad.org/3klmw1JV

The commit also broke sphinx with this:

`
6.290 [0/1/1] Generating html Sphinx documentation for clang-tools into "/home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/build/tools/clang/tools/extra/docs/html"
FAILED: tools/clang/tools/extra/docs/CMakeFiles/docs-clang-tools-html 
cd /home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/build/tools/clang/tools/extra/docs && /usr/bin/sphinx-build -b html -d /home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/build/tools/clang/tools/extra/docs/_doctrees-clang-tools-html -q -W /home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/src/tools/clang/tools/extra/docs /home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/build/tools/clang/tools/extra/docs/html

Warning, treated as error:
/home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/src/tools/clang/tools/extra/docs/ReleaseNotes.rst:82: WARNING: Inline interpreted text or phrase reference start-string without end-string.

Please try to fix that for the reland as well :)

Sorry, I reverted it in r373032 because the test fails on Linux: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/18323 . Could you please take a look? Thanks!

What's the process for re-landing a change? New Differential, or update this one?

Sorry, I reverted it in r373032 because the test fails on Linux: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/18323 . Could you please take a look? Thanks!

What's the process for re-landing a change? New Differential, or update this one?

Either works; I think most people update the existing one.

Please take a look on PR43827.