[clang-tidy] add concurrency-async-fs

[segoon]

Summary

Search for filesystem accesses that might block current system thread.

The check tries to find all fs-related types/functions from the
following list:

• C++ std
• POSIX
• Linux syscalls
• Boost.Filesystem, Boost.Nowide

Custom functions and types can be added via options.

[segoon]

fix

[njames93]

Is this just flagging all these functions, if so I don't think there's much value in here.

[segoon]

njames93, the purpose is to flag it indeed. This approach was found valueable in Yandex.Taxi, as it is very easy to forget that you're in a coroutine and may not use blocking API. The bug does affect performance (e.g. all coroutine threads wait for fs), it cannot be found by functional tests (as it is not a functional invariant violation) and may be rather tricky to debug (as the performance harm depends on many things like IO limits, page cache size, current load, etc.). It can be caught during code review, but it suffers from human errors. Rather than playing catch-me-if-you-can games, the check can be automated. As C/C++ standard libraries contain quite many blocking functions and C++20 gains official coroutine support, I find it valuable for the C++ community to have an already compiled list of such blocking functions and the check that uses it.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/concurrency-async-fs.rst

7

Please make first statement same as in Release Notes.

11

Please highlight aio and io_uring with single back-ticks.

16

Please highlight tmpfs with single back-ticks.

33

Please add information about default value. Same below.

[segoon]

• use back-ticks
• fix the first document line
• add default values

[segoon]

Eugene.Zelenko, thanks for the review! fixed

[segoon]

Eugene.Zelenko, njames93, any comments on the patch?

[Eugene.Zelenko]

In D94621#2509324, @segoon wrote:

Eugene.Zelenko, njames93, any comments on the patch?

Sorry, you need to wait for approval from one of maintainers mentioned as reviewers. I check only documentation and for minor code issues.

[segoon]

• explicitly state in docs that the check must be used only for async code

[segoon]

alexfh, aaron.ballman, hi! Any comments on the patch?

[aaron.ballman]

In D94621#2554228, @segoon wrote:

alexfh, aaron.ballman, hi! Any comments on the patch?

Have you run this check over large real-world code bases beyond Yandex.Taxi?

I think this is likely to be a maintenance burden because the list of blocking functions will likely never be complete (e.g., it's missing all the Win32 functions and wchar_t variants from C and POSIX) and I'm concerned that the output will be too chatty to be useful except in projects that are laid out in a very particular way. I would anticipate that these functions get used within a concurrent thread with some frequency specifically *because* they're blocking operations (so they are performed in a thread to keep the main thread unblocked).

[segoon]

see https://reviews.llvm.org/D94622#2617417

clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/concurrency/AsyncFsCheck.cpp

+//===--- 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

clang-tools-extra/clang-tidy/concurrency/CMakeLists.txt

   )
 
 add_clang_library(clangTidyConcurrencyModule
+  AsyncFsCheck.cpp
   ConcurrencyTidyModule.cpp
   MtUnsafeCheck.cpp
 

clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AsyncFsCheck.h"
 #include "MtUnsafeCheck.h"
 
 namespace clang {

 class ConcurrencyModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AsyncFsCheck>(
+        "concurrency-async-fs");
     CheckFactories.registerCheck<concurrency::MtUnsafeCheck>(
         "concurrency-mt-unsafe");
   }

clang-tools-extra/docs/ReleaseNotes.rst

   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.
 

clang-tools-extra/docs/clang-tidy/checks/concurrency-async-fs.rst

+.. 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.
+
+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).

clang-tools-extra/docs/clang-tidy/checks/list.rst

    `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>`_,

clang-tools-extra/test/clang-tidy/checkers/concurrency-async-fs.cpp

+// 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]
+}