This is an archive of the discontinued LLVM Phabricator instance.

add new check to find OSSpinlock usage
ClosedPublic

Authored by Wizard on Nov 21 2017, 3:09 PM.

Details

Summary

This check finds the use of methods related to OSSpinlock in Objective-C code, which should be deprecated due to livelock issues.
The following method call will be detected:

  • OSSpinlockLock()
  • OSSpinlockTry()
  • OSSpinlockUnlcok()

Event Timeline

Wizard created this revision.Nov 21 2017, 3:09 PM
Wizard edited the summary of this revision. (Show Details)
Wizard added a subscriber: cfe-commits.
Wizard updated this revision to Diff 123860.Nov 21 2017, 3:14 PM

add doc to header file

hokein edited edge metadata.Nov 22 2017, 11:29 AM

The implementation looks good, I will let Ben to do the Object-C specific review since he probably knows more about it.

clang-tidy/objc/AvoidSpinlockCheck.cpp
36

nit: clang-tidy message is not a complete message, please follow the rule.

The OSSpinlock is also deprecated on macOS, I think? from https://developer.apple.com/documentation/os/1646466-os_unfair_lock_lock.

hokein added inline comments.Nov 22 2017, 11:36 AM
clang-tidy/objc/AvoidSpinlockCheck.cpp
36

oops, sorry for my typo: s/is not a complete message/is not a complete sentence.

Wizard updated this revision to Diff 124011.Nov 22 2017, 1:40 PM
Wizard marked an inline comment as done.

fix nit

Wizard marked an inline comment as done.Nov 22 2017, 1:41 PM
benhamilton added inline comments.Nov 27 2017, 9:53 AM
clang-tidy/objc/AvoidSpinlockCheck.cpp
23–25

Why? OSSpinLock() calls should also be avoided in C++.

I think you should remove this.

28

matchesName() uses a regular expression, and is many orders of magnitude slower than hasAnyName(A, B, C).

Can you change to hasAnyName(), please?

clang-tidy/objc/CMakeLists.txt
4

IMHO this is really a check which should apply to products built on Apple SDKs, not for Objective-C.

I don't know if that means we should move this to an apple submodule or if there is a better solution.

You don't have to move it in this review, but let's open up a discussion to figure out where non-ObjC checks should go.

docs/clang-tidy/checks/objc-avoid-spinlock.rst
14

Typo: Unlcok -> Unlock

Wizard updated this revision to Diff 124427.Nov 27 2017, 11:26 AM

address comments

Wizard marked 4 inline comments as done.Nov 27 2017, 11:29 AM
Wizard added inline comments.
clang-tidy/objc/CMakeLists.txt
4

Sure. It would be a good time to move them when we have more apple checks like this.

benhamilton accepted this revision.Nov 27 2017, 11:32 AM
benhamilton added inline comments.
clang-tidy/objc/AvoidSpinlockCheck.cpp
32–33

Please fix hokein@'s comment — this should be a full sentence. Try this:

Use os_unfair_lock_lock() or dispatch queue APIs instead of the deprecated OSSpinLock().

clang-tidy/objc/AvoidSpinlockCheck.h
20

Remove "in Objective-C files".

docs/ReleaseNotes.rst
63

Remove "in Objective-C files".

docs/clang-tidy/checks/objc-avoid-spinlock.rst
7

Remove "in Objective-C files".

This revision is now accepted and ready to land.Nov 27 2017, 11:32 AM
Wizard updated this revision to Diff 124434.Nov 27 2017, 11:53 AM
Wizard marked 5 inline comments as done.

fix doc

Thanks, looks good. Just one question about the test.

test/clang-tidy/objc-avoid-spinlock.m
5

Not sure why you declared (and defined?) this one but not the others.

Either declare them all (no definition needed) or don't..

Wizard marked an inline comment as done.Nov 27 2017, 12:49 PM
Wizard added inline comments.
test/clang-tidy/objc-avoid-spinlock.m
5

Oh I added this when I tested the first method since I thought it is needed. But looks like I don't have to. Removed it.

Wizard marked 2 inline comments as done.Nov 27 2017, 12:49 PM
benhamilton added inline comments.Nov 27 2017, 12:53 PM
test/clang-tidy/objc-avoid-spinlock.m
5

It probably raises a compiler warning since the function isn't declared, but that does not cause the test to fail.

Wizard updated this revision to Diff 124456.Nov 27 2017, 1:28 PM

fix conflict

This revision was automatically updated to reflect the committed changes.