[clang][analyzer] Move unix.BlockInCriticalSection out of alpha #93815
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-github-workflow @llvm/pr-subscribers-clang Author: Endre Fülöp (gamesh411) ChangesAfter recent improvements, and testing on open source projects, the I would like to land #93799 and #93799 first, then this modification. I have ran this checker on multiple OS projects, and found no false positives, and only 10 true ones. The results for this checker: Also note that even if I just merged a commit admitting to one of this checker's limitations (that can lead to false positives), these OS project runs did not show any, so it currently does not seem to be a very prominent limitation. Full diff: https://github.com/llvm/llvm-project/pull/93815.diff 8 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 3a31708a1e9de..86412bd3b9294 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1235,6 +1235,25 @@ Check calls to various UNIX/Posix functions: ``open, pthread_once, calloc, mallo
.. literalinclude:: checkers/unix_api_example.c
:language: c
+.. _unix-BlockInCriticalSection:
+
+unix.BlockInCriticalSection (C)
+"""""""""""""""""""""""""""""""""""""
+
+Check for calls to blocking functions inside a critical section.
+Applies to: ``lock, unlock, sleep, getc, fgets, read, recv, pthread_mutex_lock,``
+`` pthread_mutex_unlock, mtx_lock, mtx_timedlock, mtx_trylock, mtx_unlock, lock_guard, unique_lock``
+
+.. code-block:: c
+
+ void test() {
+ std::mutex m;
+ m.lock();
+ sleep(3); // warn: a blocking function sleep is called inside a critical
+ // section
+ m.unlock();
+ }
+
.. _unix-Errno:
unix.Errno (C)
@@ -3130,24 +3149,6 @@ For a more detailed description of configuration options, please see the
alpha.unix
^^^^^^^^^^
-.. _alpha-unix-BlockInCriticalSection:
-
-alpha.unix.BlockInCriticalSection (C)
-"""""""""""""""""""""""""""""""""""""
-Check for calls to blocking functions inside a critical section.
-Applies to: ``lock, unlock, sleep, getc, fgets, read, recv, pthread_mutex_lock,``
-`` pthread_mutex_unlock, mtx_lock, mtx_timedlock, mtx_trylock, mtx_unlock, lock_guard, unique_lock``
-
-.. code-block:: c
-
- void test() {
- std::mutex m;
- m.lock();
- sleep(3); // warn: a blocking function sleep is called inside a critical
- // section
- m.unlock();
- }
-
.. _alpha-unix-Chroot:
alpha.unix.Chroot (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 40f443047bd4b..668e9f6cf0716 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -509,6 +509,10 @@ def UnixAPIMisuseChecker : Checker<"API">,
HelpText<"Check calls to various UNIX/Posix functions">,
Documentation<HasDocumentation>;
+def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
+ HelpText<"Check for calls to blocking functions inside a critical section">,
+ Documentation<HasDocumentation>;
+
def DynamicMemoryModeling: Checker<"DynamicMemoryModeling">,
HelpText<"The base of several malloc() related checkers. On it's own it "
"emits no reports, but adds valuable information to the analysis "
@@ -619,10 +623,6 @@ def SimpleStreamChecker : Checker<"SimpleStream">,
HelpText<"Check for misuses of stream APIs">,
Documentation<HasDocumentation>;
-def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
- HelpText<"Check for calls to blocking functions inside a critical section">,
- Documentation<HasDocumentation>;
-
} // end "alpha.unix"
//===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index 9543ba8ec02fc..e605c62a66ad0 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -42,6 +42,7 @@
// CHECK-NEXT: security.insecureAPI.mktemp
// CHECK-NEXT: security.insecureAPI.vfork
// CHECK-NEXT: unix.API
+// CHECK-NEXT: unix.BlockInCriticalSection
// CHECK-NEXT: unix.cstring.CStringModeling
// CHECK-NEXT: unix.DynamicMemoryModeling
// CHECK-NEXT: unix.Errno
diff --git a/clang/test/Analysis/block-in-critical-section.c b/clang/test/Analysis/block-in-critical-section.c
index 1e174af541b18..36ecf9ac55f7d 100644
--- a/clang/test/Analysis/block-in-critical-section.c
+++ b/clang/test/Analysis/block-in-critical-section.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.BlockInCriticalSection -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.BlockInCriticalSection -verify %s
// expected-no-diagnostics
// This should not crash
diff --git a/clang/test/Analysis/block-in-critical-section.cpp b/clang/test/Analysis/block-in-critical-section.cpp
index 87c26b9f1b520..238d9a84f5f77 100644
--- a/clang/test/Analysis/block-in-critical-section.cpp
+++ b/clang/test/Analysis/block-in-critical-section.cpp
@@ -1,5 +1,5 @@
// RUN: %clang_analyze_cc1 \
-// RUN: -analyzer-checker=alpha.unix.BlockInCriticalSection \
+// RUN: -analyzer-checker=unix.BlockInCriticalSection \
// RUN: -std=c++11 \
// RUN: -analyzer-output text \
// RUN: -verify %s
diff --git a/clang/test/Analysis/block-in-critical-section.m b/clang/test/Analysis/block-in-critical-section.m
index 73d58479f4bf4..2b5ec31568ba1 100644
--- a/clang/test/Analysis/block-in-critical-section.m
+++ b/clang/test/Analysis/block-in-critical-section.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.BlockInCriticalSection -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.BlockInCriticalSection -verify -Wno-objc-root-class %s
// expected-no-diagnostics
@interface SomeClass
diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
index 14aca5a948bf4..345a4e8f44efd 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -50,6 +50,7 @@
// CHECK-NEXT: security.insecureAPI.mktemp
// CHECK-NEXT: security.insecureAPI.vfork
// CHECK-NEXT: unix.API
+// CHECK-NEXT: unix.BlockInCriticalSection
// CHECK-NEXT: unix.cstring.CStringModeling
// CHECK-NEXT: unix.DynamicMemoryModeling
// CHECK-NEXT: unix.Errno
diff --git a/clang/www/analyzer/alpha_checks.html b/clang/www/analyzer/alpha_checks.html
index 2c8eece41fb2f..411baae695b93 100644
--- a/clang/www/analyzer/alpha_checks.html
+++ b/clang/www/analyzer/alpha_checks.html
@@ -780,39 +780,6 @@ <h3 id="unix_alpha_checkers">Unix Alpha Checkers</h3>
<tbody>
-<tr><td><a id="alpha.unix.BlockInCriticalSection"><div class="namedescr expandable"><span class="name">
-alpha.unix.BlockInCriticalSection</span><span class="lang">
-(C)</span><div class="descr">
-Check for calls to blocking functions inside a critical section. Applies to:
-<div class=functions>
-lock<br>
-unlock<br>
-sleep<br>
-getc<br>
-fgets<br>
-read<br>
-revc<br>
-pthread_mutex_lock<br>
-pthread_mutex_unlock<br>
-mtx_lock<br>
-mtx_timedlock<br>
-mtx_trylock<br>
-mtx_unlock<br>
-lock_guard<br>
-unique_lock</div>
-</div></div></a></td>
-<td><div class="exampleContainer expandable">
-<div class="example"><pre>
-void test() {
- std::mutex m;
- m.lock();
- sleep(3); // warn: a blocking function sleep is called inside a critical
- // section
- m.unlock();
-}
-</pre></div></div></td></tr>
-
-
<tr><td><a id="alpha.unix.Chroot"><div class="namedescr expandable"><span class="name">
alpha.unix.Chroot</span><span class="lang">
(C)</span><div class="descr">
|
gamesh411
force-pushed
the
block-in-critical-dealpha
branch
from
6fd3830
to
898940a
Compare
After recent improvements, and testing on open source projects, the checker is ready to move out of the alpha package.
gamesh411
force-pushed
the
block-in-critical-dealpha
branch
from
898940a
to
1da3d4d
Compare
|
Rebased and squashed noisy small commits. |
steakhal
approved these changes
Jun 1, 2024
steakhal
approved these changes
Jun 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After recent improvements (#80029), and testing on open-source projects, the
checker is ready to move out of the alpha package.
I have run this checker on multiple OS projects and found no false positives and only 10 true ones.
The complete set of tested projects:
codechecker, memcached, tmux, curl, twin, vim, openssl, sqlite, ffmpeg, postgres, tinyxml2, libwebm, xerces, bitcoin, protobuf, qtbase, openrct2, llvm-project.
The results for this checker:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_alpha.unix.BlockInCriticalSection-evaluation&is-unique=on&diff-type=New&checker-name=alpha.unix.BlockInCriticalSection
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_alpha.unix.BlockInCriticalSection-evaluation&is-unique=on&diff-type=New&checker-name=alpha.unix.BlockInCriticalSection
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openrct2_v0.4.8_alpha.unix.BlockInCriticalSection-evaluation&is-unique=on&diff-type=New&checker-name=alpha.unix.BlockInCriticalSection
Also note that even if there is a recent commit (#93799) admitting to one of this checker's limitations (that can lead to false positives), these OS project runs did not show any, so it currently does not seem to be a very prominent limitation.
Please see this comment #93799 (comment) for my plans to deal with this limitation and reduce code duplication.