This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Block in critical section extension
ClosedPublic

Authored by zdtorok on Feb 5 2017, 10:32 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). In this patch C11 and Pthread locking/unlocking support has been added and also two new parameters have been implemented to be able to switch these on/off.

Diff Detail

Repository
rL LLVM

Event Timeline

zdtorok created this revision.Feb 5 2017, 10:32 AM
dcoughlin edited edge metadata.Feb 10 2017, 10:12 AM

I saw on Twitter today that this checker already has at least one happy user! See https://twitter.com/steipete/status/830099493890703360

A question: under what scenarios do you envision the C11Enabled and PthreadEnabled analyzer options being used? Can we get away with not having the the options and just always check for them?

Also: Thanks for adding documentation to the .html!

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
141–145

I think it is worth fixing the diagnostic now to correctly mention the called function rather than have a stray '%s' in the message.

See, for example, CompareReturnTypes() in CheckObjCInstMethSignature.cpp for how to create a diagnostic message dynamically.

xazax.hun edited edge metadata.EditedFeb 11 2017, 1:36 AM

A question: under what scenarios do you envision the C11Enabled and PthreadEnabled analyzer options being used? Can we get away with not having the the options and just always check for them?

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

zaks.anna edited edge metadata.Feb 11 2017, 8:37 AM

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

I did not measure the performance, only observed the implementation. Do you want me to do a measurement first? I fear that it might be noisy due to the fact that the execution time might still be dominated by other parts of the analyzer.

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

I would put the cache inside the CallDescription objects, so checker authors do not need to wory about this.

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I will submit a separate patch.

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

I did not measure the performance, only observed the implementation. Do you want me to do a measurement first? I fear that it might be noisy due to the fact that the execution time might still be dominated by other parts of the analyzer.

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

I would put the cache inside the CallDescription objects, so checker authors do not need to wory about this.

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I will submit a separate patch.

https://reviews.llvm.org/D29884 is up for review. With those changes accepted +1 for removing the options.

I think we need to change CallDescription to enable those by default.

See https://clang.llvm.org/doxygen/CallEvent_8cpp_source.html#l00213

So in case an identifier does not exist in a translation unit, the identifier lookup is repeated on every CallEvent::isCalled call which might become a performance issue. We should cache the fact a lookup was already done.

Good point! Have you observed performance issues due to this or is this based on an observation that the implementation of CallEvent::isCalled is not optimal?

I did not measure the performance, only observed the implementation. Do you want me to do a measurement first? I fear that it might be noisy due to the fact that the execution time might still be dominated by other parts of the analyzer.

Do you propose to keep a cache of all identifiers that were were previously looked up by all callers of CallEvent? Or should there be a per checker cache?

I would put the cache inside the CallDescription objects, so checker authors do not need to wory about this.

The main adopters of the new CallEvent API are ValistChecker.cpp, SimpleStreamChecker.cpp, and BlockInCriticalSectionChecker.cpp. The API optimization should be a separate patch. Are there volunteers to tackle it?

I will submit a separate patch.

https://reviews.llvm.org/D29884 is up for review. With those changes accepted +1 for removing the options.

Commited in r295186.

zaks.anna added inline comments.Feb 24 2017, 10:11 PM
lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
141–145

I think Devin suggests using llvm::raw_svector_ostream here. Another example is in MallocChecker.cpp.

zdtorok updated this revision to Diff 90052.Feb 28 2017, 9:53 AM

Based on your feedback I've removed the PThreadEnabled and C11Enabled options and also in the warning %s has been replaced by the function's name itself.
Thanks for your feedback and helpful comments!

dcoughlin accepted this revision.Feb 28 2017, 8:03 PM

Thanks for updating this!

It looks good to me with the diagnostic changes (single quotes, removed period) mentioned inline!

lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
143
  • You should add single quotes around the function name to be consistent existing clang diagnostics. (For example, " function 'sleep' is")
  • Similarly, you should not end the diagnostic with a period. (We are not always consistent about this in the analyzer -- but we should do better!)

Also, here is a suggestion for a slightly re-worded diagnostic that seems more direct to me:

"Call to blocking function 'foo' inside of critical section"

What do you think?

148

You shouldn't do it in this commit, but one thing that would make this checker even more helpful would be to add a BugReporterVisitor adding a path note indicating where the critical section began.

This revision is now accepted and ready to land.Feb 28 2017, 8:03 PM
zdtorok updated this revision to Diff 90699.Mar 6 2017, 7:36 AM
zdtorok marked 3 inline comments as done.

Changed the warning message.

zdtorok added a comment.EditedMar 6 2017, 7:37 AM

Thanks for the feedback again. I've changed the diagnostic message and uploaded the new diff.
Please note, that I'm not allowed to commit yet, so if it's accepted, may I ask some of you to commit my changes please? Thanks!

Changed the warning message.

Did you rerun the tests? As far as I can see, you did not update the test files according to the rewording of the message, so I think they won't pass.

zdtorok updated this revision to Diff 91199.Mar 9 2017, 11:38 AM
zdtorok set the repository for this revision to rL LLVM.

Fixed tests after re-phrasing the diagnostic message.

Fixed the tests with the new diagnostic message. Now all tests pass.

This revision was automatically updated to reflect the committed changes.