[llvm-dev] [clang-tidy][RFC] checks concurrency-async-{blocking, fs, no-new-threads}
Vasily Kulikov via llvm-dev
With official C++20 inclusion of coroutines it gets important to
identify synchronous code usage inside of coroutines. Volunteer
preemption of a system thread in asynchronous code (e.g. in
coroutines/fibers/green threads) is a bug that prevents the current
thread from executing other coroutines/etc. and negatively affects
overall process performance.
I've tried to address it in a series of clang-tidy checks that
try to find such synchronous code based on blacklists from
POSIX/C++ std/C11/Boost/Linux syscalls. Basically, one should enable
concurrency-*
checks for the strictest mode. It finds usages of synchronous primitives
(e.g. std::mutex), fs access (e.g. open(3)), implicit system threads
creation (e.g. via std::execution::par). In some cases one may want to
relax the check, e.g.:
- if new threads creation is not an issue, I don't force coroutine-only
code, just don't want to block coroutine threads => disable
concurrency-async-no-new-threads
- if I have no means to delegate fs access to a thread pool and don't
want to use aio, so I have to make synchronous fs access from coroutines
=> disable concurrency-async-fs
Nathan James in a review (https://reviews.llvm.org/D94621#2497859)
questions whether such check based on functions/types blacklist worth
implementing. I think it does as every coroutine/fiber-based C++ program
may suffer from the same problem - you may not use a lot of std stuff
unless you're OK with ineffective code, so having such "do not use part
of std/boost" checks is handy.
The checks:
concurrency-async-blocking: https://reviews.llvm.org/D93940
concurrency-async-fs: https://reviews.llvm.org/D94621
concurrency-async-no-new-threads: https://reviews.llvm.org/D94622
The patch series are based on a much more simple check used in
Yandex.Taxi. I think it worth something to the community, so I've
separated a big list of functions/types apart to address e.g.
environments without fs threadpool, and added possibility to extend the
types/functions lists via option.
I'd be happy to hear any comments how the checks can be improved, or
maybe organized in some other way, or whether they confront any implicit
clang-tidy policy.
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
David Chisnall via llvm-dev
I think this is definitely a real problem and it would be great to have
a solution but I am somewhat sceptical that clang-tidy is the right tool
to address it. The problem is that it's not just blocking system calls
that are unsafe, it's anything that transitively calls those functions.
To have a reliable tool, you'd want something (possibly built on the
static analyser? I don't know what the status of cross-module analysis)
that would build a call graph and warn when a code path might end up in
a blocking call (ideally by analysing which code paths are guarded on
specific argument values).
You could probably approximate this with an incremental clang-tidy pass
that would report functions that called functions in a list and updated
the list with additional things that it had found.
It is possible that coroutine code in the wild isn't doing many deep
function calls as more general code, in which case it's quite plausible
that a simple clang-tidy check would be sufficient. I worry that a
deny-list approach is going to be far less maintainable than an
allow-list though: in POSIX there are a handful of system calls that can
block, but in win32 there are vastly more and higher-level APIs such as
Qt have a massive API surface.
A list of C++ standard-library functions that are safe to call in
coroutines seems plausible to maintain. I wonder if the right approach
is a [[clang::coroutine_safe]] attribute that we can put on libc++
functions, methods, and classes (if all methods are safe to call, or
with a [[clang::coroutine_unsafe]] variant for methods where the default
has been defined). We could then have a checker that warns if you call
a function that isn't marked as coroutine-safe from a coroutine or from
a function that is marked as coroutine-safe.
David
Vasily Kulikov via llvm-dev
Thank you for the comment!
David Chisnall:
> It is possible that coroutine code in the wild isn't doing many deep
> function calls as more general code, in which case it's quite plausible
> that a simple clang-tidy check would be sufficient. I worry that a
> deny-list approach is going to be far less maintainable than an
> allow-list though: in POSIX there are a handful of system calls that can
> block, but in win32 there are vastly more and higher-level APIs such as
> Qt have a massive API surface.
Agreed. I don't want the solution that's hard to maintain too.
> A list of C++ standard-library functions that are safe to call in
> coroutines seems plausible to maintain. I wonder if the right approach
> is a [[clang::coroutine_safe]] attribute that we can put on libc++
> functions, methods, and classes (if all methods are safe to call, or
> with a [[clang::coroutine_unsafe]] variant for methods where the default
> has been defined). We could then have a checker that warns if you call
> a function that isn't marked as coroutine-safe from a coroutine or from
> a function that is marked as coroutine-safe.
Hm, the attribute sounds interesting. And if it becomes widespread, it could be
proposed for the next standard...
However, it would suffer from the same "WL somewhere, BL somewhere" issue - it's
relatively simple to mark with [[clang::*]] for LLVM's code, it would be harder
for glibc/boost, and it will be impossible for third-party libraries we may not
commit to. The problem is almost the same as with noexcept or const - if the
keyword is not set, you may not be certain whether it is intentional or the
method really throws/changes *this.
Btw, for a full replacement of the PRs there should be at least the 2nd
attribute for "no fs access". I expect there are many projects that tolerate fs
access from coroutines as there is no fs thread pool. It's not obvious to me
what the solution should be here (if any).
Vasily Kulikov via llvm-dev
On 27.01.2021 21:45, Vasily Kulikov wrote:
> Hm, the attribute sounds interesting. And if it becomes widespread, it could be
> proposed for the next standard...
While I agree that the current approach with naive blacklists suffer from "you
have to explicitly mark all bad functions/types" issue in general, I think it
still has some non-zero limited value. If you don't develop with WinAPI32,
OpenGL, or Qt, but with Boost, POSIX and std (e.g. pretty many backend
projects), the extra steps you have to do by yourself are rather simple (if
any). Blacklists of widespread libraries can be compiled and added into upstream
clang-tidy by other volunteers.
The patches fit our environment in Yandex.Taxi, might fit others too. I'm not
sure whether I'm the right person to implement a check with ideologically better
architecture. If merge BL code, it would not complicate things for the better
architecture or significantly reduce motivation for invention, it can be
implemented on top of it. If/when a more precise check is implemented (with
attributes, or some heuristics), BL tidy check can be deprecated with a
suggestion to use the new better tool.
So, I suggest to split the problem into two:
1) merge current PRs
2) someday come to a better solution
What do you say?