Page MenuHomePhabricator

[analyzer] lock_guard and unique_lock extension for BlockInCriticalSection Static Analyzer checker
ClosedPublic

Authored by zdtorok on May 31 2017, 10:25 AM.

Details

Summary

This is an extension to an already existing alpha-checker which finds the calls to blocking functions inside a critical section. (See that original diff: https://reviews.llvm.org/D21506, and an earlier extension: https://reviews.llvm.org/D29567). In this patch lock_guard and unique_lock support has been added.

Diff Detail

Repository
rL LLVM

Event Timeline

zdtorok created this revision.May 31 2017, 10:25 AM
zaks.anna edited edge metadata.Jun 14 2017, 5:03 PM

Looks good overall, but we should try and extend isCalled if possible.

Also, what is the plan for bringing this checker out of alpha? Turning checkers on by default should be top priority since most users are not exposed to them otherwise!

Thank you!
Anna

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
111 ↗(On Diff #100885)

Why isCalled is not used here? Would it be possible coextend it to support CXXConstructorCall?

NoQ added a subscriber: NoQ.Jun 21 2017, 3:11 AM
NoQ added inline comments.
lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
111 ↗(On Diff #100885)

CallDescription objects don't support C++ constructors or Objective-C methods. By the way, we've just noticed that this checker crashes on pretty much any Objective-C code:

$ cat test.m

#include <Foundation/Foundation.h>

void foo() {
  NSObject *obj = [[NSObject alloc] init];
  [obj release];
}

$ clang --analyze -Xanalyzer -analyzer-checker -Xanalyzer alpha.unix.BlockInCriticalSection test.m

Assertion failed: (getKind() != CE_ObjCMessage && "Obj-C methods are not supported"), function isCalled, file <censored>/llvm/tools/clang/lib/StaticAnalyzer/Core/CallEvent.cpp, line 214.

So when you use isCalled(), currently you must ensure that a plain C function is being called.

I totally agree that isCalled() should be improved to support various calls - its whole point is removing code duplication, i.e. stuff like initIdentifierInfo().

zaks.anna added inline comments.Jun 26 2017, 11:13 AM
lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
111 ↗(On Diff #100885)

I suggest extending isCalled with C++ constructors.
We should also make sure that it isCalled conservatively handles ObjC method calls when assertions are off. Not sure what we want to do for assert builds... there are benefits of having the assertion there and there are downsides.

Another option would be to remove isCalled and all dependencies on it if we do not think it should be used by everyone.

@xazax.hun, what do you think?

xazax.hun added inline comments.Jul 4 2017, 2:25 AM
lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
111 ↗(On Diff #100885)

I do agree, handling ObjC methods conservatively in a release build is a good idea. I am in favor of keeping the asserts though.

One of the reasons I do like the isCalled abstraction, we do not need to use the mutable keyword in the checkers. It would definitely be good to support C++ methods and constructors. It is not trivial to come up with a good design due to overloading.

The CallDescription could accept a list of strings that describes the name of the types of the arguments, but what about type aliases?
It should also have a way to describe constraints on namespaces.
It could have an enum to restrict results to ctors/dtors/overloaded operators/methods/objc-methods/c functions in the future.
Also in case of methods, a method might be const, volatile, it could be overloaded on lvalues/rvalues.

We might add features as we go, but it might become harder to extend it in the future if we do not have a good design upfront. I think such a "call description" will be useful in the future since similar mechanism would be needed to have API Notes support for C++.

Ping... While extending isCalled is not a must, this should not crash on the sample ObjC code.

xazax.hun edited edge metadata.Sep 5 2017, 5:52 AM

Ping... While extending isCalled is not a must, this should not crash on the sample ObjC code.

Created a new patch for that: https://reviews.llvm.org/D37470

zaks.anna accepted this revision.Oct 27 2017, 10:53 AM

Thanks!

This revision is now accepted and ready to land.Oct 27 2017, 10:53 AM
xazax.hun accepted this revision.Oct 27 2017, 11:17 AM

LGTM as well.

This revision was automatically updated to reflect the committed changes.