segoon updated this revision to Diff 322695.
segoon added a comment.
Herald added a subscriber: nullptr.cpp.
- explicitly state in docs that the check must be used only for async code
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94621/new/
https://reviews.llvm.org/D94621
Files:
clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.cpp
clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.h
clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/concurrency-async-fs.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/concurrency-async-fs.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/concurrency-async-fs.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/concurrency-async-fs.cpp
@@ -0,0 +1,87 @@
+// RUN: %check_clang_tidy %s concurrency-async-fs %t -- \
+// RUN: -config='{CheckOptions: [{key: "concurrency-async-fs.FunctionsExtra", value: "::my::block"},{key: "concurrency-async-fs.TypesExtra", value: "::my::file"}]}'
+
+void chdir(const char *);
+
+namespace std {
+namespace filesystem {
+bool exists(const char *);
+
+void copy(const char *, const char *);
+
+class directory_iterator {
+public:
+ directory_iterator(const char *);
+
+ bool operator!=(const directory_iterator &) const;
+
+ directory_iterator &operator++();
+
+ int operator*() const;
+};
+
+directory_iterator begin(directory_iterator iter) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: type 'directory_iterator' may access filesystem in a blocking way [concurrency-async-fs]
+directory_iterator end(const directory_iterator &) noexcept;
+
+} // namespace filesystem
+
+template <typename T>
+class basic_fstream {};
+
+template <typename T>
+class basic_ofstream {};
+
+template <typename T>
+class basic_ifstream {};
+
+typedef basic_fstream<char> fstream;
+typedef basic_ofstream<char> ofstream;
+typedef basic_ifstream<char> ifstream;
+
+} // namespace std
+
+void copy();
+
+void test_core() {
+ chdir("/");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'chdir' may block on filesystem access [concurrency-async-fs]
+
+ std::filesystem::exists("/");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'exists' may block on filesystem access [concurrency-async-fs]
+
+ std::filesystem::copy("/a", "/b");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'copy' may block on filesystem access [concurrency-async-fs]
+
+ copy();
+
+ for (const auto &f : std::filesystem::directory_iterator("/")) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: type 'directory_iterator' may access filesystem in a blocking way [concurrency-async-fs]
+ }
+}
+
+void test_fstream() {
+ std::fstream fs;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: type 'basic_fstream<char>' may access filesystem in a blocking way [concurrency-async-fs]
+ std::ofstream of;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: type 'basic_ofstream<char>' may access filesystem in a blocking way [concurrency-async-fs]
+ std::ifstream ifs;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: type 'basic_ifstream<char>' may access filesystem in a blocking way [concurrency-async-fs]
+
+ std::basic_fstream<char> bfs;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: type 'basic_fstream<char>' may access filesystem in a blocking way [concurrency-async-fs]
+}
+
+namespace my {
+class file {};
+
+void block();
+} // namespace my
+
+void test_user() {
+ my::file f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: type 'file' may access filesystem in a blocking way [concurrency-async-fs]
+
+ my::block();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'block' may block on filesystem access [concurrency-async-fs]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -139,6 +139,7 @@
`clang-analyzer-valist.CopyToSelf <clang-analyzer-valist.CopyToSelf.html>`_,
`clang-analyzer-valist.Uninitialized <clang-analyzer-valist.Uninitialized.html>`_,
`clang-analyzer-valist.Unterminated <clang-analyzer-valist.Unterminated.html>`_,
+ `concurrency-async-fs <concurrency-async-fs.html>`_,
`concurrency-mt-unsafe <concurrency-mt-unsafe.html>`_,
`cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_,
`cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/concurrency-async-fs.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/concurrency-async-fs.rst
@@ -0,0 +1,46 @@
+.. title:: clang-tidy - concurrency-async-fs
+
+concurrency-async-fs
+====================
+
+Finds filesystem accesses that might block current system thread.
+Asynchronous code may deal with it in numerous ways, the most widespread
+ways are the following:
+
+* use asynchronous API like `aio` or `io_uring`
+* delegate all filesystem access to a thread pool
+
+Some projects may consider filesystem access from asynchronous code as
+a non-issue, e.g. it is known that all filesystem code doesn't block
+system threads as it resides in memory (e.g. `tmpfs`), or blocking functions
+are used in the startup code only, or asynchronous API/thread pool is not
+acceptable for some reason.
+
+.. note::
+
+ The check doesn't identify synchronous and asynchronous code. Insead, it
+ assumes that all analyzed code is asynchronous and all blocking calls have to
+ be found. You should split the sync and async code at the filesystem level
+ and enable `concurrency-async-*` checks for files with asynchronous code
+ only.
+
+The check by default tries to find all fs-related types/functions from the
+following list:
+
+* C++ std
+* POSIX
+* Linux syscalls
+* Boost.Filesystem, Boost.Nowide
+
+Options
+-------
+
+.. option:: FunctionsExtra
+
+ Specifies additional functions to search for, separated with semicolon.
+ Defaults to empty string (no functions).
+
+.. option:: TypesExtra
+
+ Specifies additional types to search for, separated with semicolon.
+ Defaults to empty string (no types).
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -121,6 +121,11 @@
Finds structs that are inefficiently packed or aligned, and recommends
packing and/or aligning of said structs as needed.
+- New :doc:`concurrency-async-fs
+ <clang-tidy/checks/concurrency-async-fs>` check.
+
+ Finds filesystem accesses that might block current system thread.
+
- New :doc:`cppcoreguidelines-prefer-member-initializer
<clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
Index: clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
+++ clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp
@@ -9,6 +9,7 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "AsyncFsCheck.h"
#include "MtUnsafeCheck.h"
namespace clang {
@@ -18,6 +19,8 @@
class ConcurrencyModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<AsyncFsCheck>(
+ "concurrency-async-fs");
CheckFactories.registerCheck<concurrency::MtUnsafeCheck>(
"concurrency-mt-unsafe");
}
Index: clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt
@@ -4,6 +4,7 @@
)
add_clang_library(clangTidyConcurrencyModule
+ AsyncFsCheck.cpp
ConcurrencyTidyModule.cpp
MtUnsafeCheck.cpp
Index: clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.h
@@ -0,0 +1,38 @@
+//===--- AsyncFsCheck.h - clang-tidy ----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CONCURRENCY_ASYNCFSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CONCURRENCY_ASYNCFSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace concurrency {
+
+/// Finds filesystem accesses that might block current system thread.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/concurrency-async-fs.html
+class AsyncFsCheck : public ClangTidyCheck {
+public:
+ AsyncFsCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts);
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ const std::string FunctionsExtra;
+ const std::string TypesExtra;
+};
+
+} // namespace concurrency
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CONCURRENCY_ASYNCFSCHECK_H
Index: clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.cpp
@@ -0,0 +1,302 @@
+//===--- AsyncFsCheck.cpp - clang-tidy ------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AsyncFsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+static const char Function[] = "func";
+static const char TypeName[] = "type";
+static const char Name[] = "name";
+
+static const clang::StringRef FsFunctions[] = {
+ /* C++ std */
+ "::std::filesystem::absolute", //
+ "::std::filesystem::canonical", //
+ "::std::filesystem::weakly_canonical", //
+ "::std::filesystem::relative", //
+ "::std::filesystem::proximate", //
+ "::std::filesystem::copy", //
+ "::std::filesystem::copy_file", //
+ "::std::filesystem::copy_symlink", //
+ "::std::filesystem::create_directory", //
+ "::std::filesystem::create_directories", //
+ "::std::filesystem::create_hard_link", //
+ "::std::filesystem::create_symlink", //
+ "::std::filesystem::create_directory_symlink", //
+ "::std::filesystem::exists", //
+ "::std::filesystem::equivalent", //
+ "::std::filesystem::file_size", //
+ "::std::filesystem::hard_link_count", //
+ "::std::filesystem::last_write_time", //
+ "::std::filesystem::permissions", //
+ "::std::filesystem::read_symlink", //
+ "::std::filesystem::remove", //
+ "::std::filesystem::remove_all", //
+ "::std::filesystem::rename", //
+ "::std::filesystem::resize_file", //
+ "::std::filesystem::space", //
+ "::std::filesystem::status", //
+ "::std::filesystem::symlink_status", //
+
+ "::std::filesystem::is_block_file", //
+ "::std::filesystem::is_character_file", //
+ "::std::filesystem::is_directory", //
+ "::std::filesystem::is_empty", //
+ "::std::filesystem::is_fifo", //
+ "::std::filesystem::is_other", //
+ "::std::filesystem::is_regular_file", //
+ "::std::filesystem::is_socket", //
+ "::std::filesystem::is_symlink", //
+ "::std::filesystem::status_known", //
+
+ /* Boost.Filesystem */
+ "::boost::filesystem::absolute", //
+ "::boost::filesystem::canonical", //
+ "::boost::filesystem::copy", //
+ "::boost::filesystem::copy_directory", //
+ "::boost::filesystem::copy_file", //
+ "::boost::filesystem::copy_symlink", //
+ "::boost::filesystem::create_directories", //
+ "::boost::filesystem::create_directory", //
+ "::boost::filesystem::create_hard_link", //
+ "::boost::filesystem::create_symlink", //
+ "::boost::filesystem::equivalent", //
+ "::boost::filesystem::exists", //
+ "::boost::filesystem::file_size", //
+ "::boost::filesystem::hard_link_count", //
+ "::boost::filesystem::last_write_time", //
+ "::boost::filesystem::permissions", //
+ "::boost::filesystem::proximate", //
+ "::boost::filesystem::read_symlink", //
+ "::boost::filesystem::relative", //
+ "::boost::filesystem::remove", //
+ "::boost::filesystem::remove_all", //
+ "::boost::filesystem::rename", //
+ "::boost::filesystem::resize_file", //
+ "::boost::filesystem::space", //
+ "::boost::filesystem::status", //
+ "::boost::filesystem::status_known", //
+ "::boost::filesystem::symlink_status", //
+ "::boost::filesystem::system_complete", //
+ "::boost::filesystem::temp_directory_path", //
+ "::boost::filesystem::unique_path", //
+ "::boost::filesystem::weakly_canonical", //
+
+ "::boost::filesystem::is_block_file", //
+ "::boost::filesystem::is_character_file", //
+ "::boost::filesystem::is_directory", //
+ "::boost::filesystem::is_empty", //
+ "::boost::filesystem::is_fifo", //
+ "::boost::filesystem::is_other", //
+ "::boost::filesystem::is_regular_file", //
+ "::boost::filesystem::is_socket", //
+ "::boost::filesystem::is_symlink", //
+ "::boost::filesystem::status_known", //
+
+ /* Boost.Nowide */
+ "::boost::nowide::fopen", //
+ "::boost::nowide::freopen", //
+ "::boost::nowide::rename", //
+ "::boost::nowide::remove", //
+ "::boost::nowide::stat", //
+
+ /* POSIX, unistd.h */
+ "::access", //
+ "::chdir", //
+ "::chown", //
+ "::faccessat", //
+ "::fchdir", //
+ "::fchown", //
+ "::fchownat", //
+ "::fdatasync", //
+ "::fsync", //
+ "::getgroups", //
+ "::gethostname", //
+ "::lchown", //
+ "::link", //
+ "::linkat", //
+ "::readlink", //
+ "::readlinkat", //
+ "::rmdir", //
+ "::symlink", //
+ "::symlinkat", //
+ "::sync", //
+ "::truncate", //
+ "::ttyname", //
+ "::ttyname_r", //
+ "::unlink", //
+ "::unlinkat", //
+
+ /* POSIX, dirent.h */
+ "::opendir", //
+ "::readdir_r", //
+ "::rewinddir", //
+ "::scandir", //
+ "::seekdir", //
+ "::telldir", //
+
+ /* POSIX, fcntl.h */
+ "::open", //
+ "::openat", //
+ "::creat", //
+ "::fcntl", //
+ "::posix_fallocate", //
+ "::posix_fadvise", //
+
+ /* POSIX, grp.h */
+ "::getgrent", //
+ "::getgrgid", //
+ "::getgrgid_r", //
+ "::getgrnam", //
+ "::getgrnam_r", //
+ "::setgrent", //
+
+ /* POSIX, stdio.h */
+ "::fdopen", //
+ "::fopen", //
+ "::freopen", //
+ "::popen", //
+ "::tempnam", //
+ "::tmpfile", //
+ "::tmpnam", //
+ "::rename", //
+ "::renameat", //
+
+ /* POSIX, stdlib.h */
+ "::mkdtemp", //
+ "::mkstemp", //
+ "::posix_openpt", //
+ "::realpath", //
+
+ /* POSIX, sys/mman.h */
+ "::msync", //
+ "::mlock", //
+ "::shm_open", //
+ "::shm_unlink", //
+
+ /* POSIX, sys/stat.h */
+ "::chmod", //
+ "::fchmod", //
+ "::fchmodat", //
+ "::fstat", //
+ "::fstatat", //
+ "::lstat", //
+ "::futimens", //
+ "::mkdir", //
+ "::mkdirat", //
+ "::mkfifo", //
+ "::mkfifoat", //
+ "::mknod", //
+ "::mknodat", //
+ "::stat", //
+ "::utimensat", //
+
+ /* POSIX, sys/statvfs.h */
+ "::fstatvsf", "::statvsf",
+
+ /* POSIX, utime.h */
+ "::utime",
+
+ /* POSIX, misc */
+ "::dlopen", //
+ "::glob", //
+ "::setlocale", //
+
+ /* Linux */
+ "::statfs", //
+ "::fstatfs", //
+ "::syncfs", //
+ "::name_to_handle_at", //
+ "::open_by_handle_at", //
+ "::mount", //
+ "::umount", //
+ "::ustat", //
+ "::chroot", //
+ "::open_tree", //
+ "::move_mount", //
+ "::fsopen", //
+ "::fsconfig", //
+ "::fsmount", //
+ "::fspick", //
+};
+
+static const clang::StringRef FsTypes[] = {
+ /* C++ std */
+ "::std::basic_ifstream", //
+ "::std::basic_ofstream", //
+ "::std::basic_fstream", //
+
+ "::std::filesystem::directory_iterator", //
+ "::std::filesystem::recursive_directory_iterator", //
+
+ /* Boost.Filesystem */
+ "::boost::filesystem::directory_iterator", //
+ "::boost::filesystem::recursive_directory_iterator", //
+};
+
+static std::vector<llvm::StringRef>
+toVector(llvm::ArrayRef<llvm::StringRef> Base, llvm::StringRef Extra) {
+ llvm::SmallVector<llvm::StringRef, 4> Tmp{Base.begin(), Base.end()};
+ if (!Extra.empty()) {
+ Extra.split(Tmp, ";");
+ }
+
+ return {Tmp.begin(), Tmp.end()};
+}
+
+namespace clang {
+namespace tidy {
+namespace concurrency {
+
+AsyncFsCheck::AsyncFsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ FunctionsExtra(Options.get("FunctionsExtra", "")),
+ TypesExtra(Options.get("TypesExtra", "")) {}
+
+void AsyncFsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "FunctionsExtra", FunctionsExtra);
+ Options.store(Opts, "TypesExtra", TypesExtra);
+}
+
+void AsyncFsCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ callExpr(
+ callee(functionDecl(hasAnyName(toVector(FsFunctions, FunctionsExtra)))
+ .bind(Name)))
+ .bind(Function),
+ this);
+
+ Finder->addMatcher(
+ valueDecl(hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+ namedDecl(hasAnyName(toVector(FsTypes, TypesExtra)))
+ .bind(Name))))))
+ .bind(TypeName),
+ this);
+}
+
+void AsyncFsCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *N = Result.Nodes.getNodeAs<NamedDecl>(Name);
+ assert(N);
+
+ if (const auto *CE = Result.Nodes.getNodeAs<CallExpr>(Function)) {
+ diag(CE->getBeginLoc(), "function %0 may block on filesystem access")
+ << N << CE->getSourceRange();
+ } else if (const auto *Decl = Result.Nodes.getNodeAs<ValueDecl>(TypeName)) {
+ diag(Decl->getBeginLoc(), "type %0 may access filesystem in a blocking way")
+ << N << Decl->getSourceRange();
+ } else {
+ assert(false);
+ }
+}
+
+} // namespace concurrency
+} // namespace tidy
+} // namespace clang
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
