clangd vs LLVM_ENABLE_THREADS=OFF

[Quuxplusone]

Bugzilla Link	PR41569
Status	RESOLVED FIXED
Importance	P enhancement
Reported by	Nico Weber (nicolasweber@gmx.de)
Reported on	2019-04-23 08:02:16 -0700
Last modified on	2019-05-07 00:09:31 -0700
Version	unspecified
Hardware	PC All
CC	geek4civic@gmail.com, gribozavr@gmail.com, hans@chromium.org, kadircetinkaya.06.tr@gmail.com, llvm-bugs@lists.llvm.org, sammccall@google.com
Fixed by commit(s)	rL360115
Attachments
Blocks
Blocked by
See also

clangd currently doesn't build with LLVM_ENABLE_THREADS=OFF (after r358664).

This makes it build:

$ git diff
diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang-tools-
extra/clangd/index/Background.cpp
index 0746a358371..2314a36908e 100644
--- a/clang-tools-extra/clangd/index/Background.cpp
+++ b/clang-tools-extra/clangd/index/Background.cpp
@@ -191,11 +191,15 @@ void BackgroundIndex::run() {
       Queue.pop_front();
     }

+#if LLVM_ENABLE_THREADS
     if (Priority != llvm::ThreadPriority::Default && !PreventStarvation.load())
       llvm::set_thread_priority(Priority);
+#endif
     (*Task)();
+#if LLVM_ENABLE_THREADS
     if (Priority != llvm::ThreadPriority::Default)
       llvm::set_thread_priority(llvm::ThreadPriority::Default);
+#endif

     {
       std::unique_lock<std::mutex> Lock(QueueMu);

But then a bunch of tests fail:

********************
Testing Time: 21.81s
********************
Failing Tests (17):
    Clang Tools :: clangd/background-index.test
    Extra Tools Unit Tests :: clangd/./ClangdTests/ClangdThreadingTest.StressTest
    Extra Tools Unit Tests :: clangd/./ClangdTests/ClangdVFSTest.MemoryUsage
    Extra Tools Unit Tests :: clangd/./ClangdTests/ClangdVFSTest.ReparseOpenedFiles
    Extra Tools Unit Tests :: clangd/./ClangdTests/DocumentSymbolsTest.ExternSymbol
    Extra Tools Unit Tests :: clangd/./ClangdTests/DocumentSymbolsTest.InHeaderFile
    Extra Tools Unit Tests :: clangd/./ClangdTests/GoToInclude.All
    Extra Tools Unit Tests :: clangd/./ClangdTests/TUSchedulerTests.ManyUpdates
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.AnonymousNamespace
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.Enums
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.GlobalNamespaceQueries
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.Globals
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.MultiFile
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.Namespaces
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.Ranking
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.Unnamed
    Extra Tools Unit Tests :: clangd/./ClangdTests/WorkspaceSymbolsTest.WithLimit

Here's an example stack:

Assertion failed: (PrettyStackTraceHead == this && "Pretty stack trace entry
destruction is out of order"), function ~PrettyStackTraceEntry, file
../../llvm/lib/Support/PrettyStackTrace.cpp, line 143.
0  ClangdTests              0x000000010b1b6075
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1  ClangdTests              0x000000010b1b4e76 llvm::sys::RunSignalHandlers() +
198
2  ClangdTests              0x000000010b1b66e8 SignalHandler(int) + 264
3  libsystem_platform.dylib 0x00007fffb036bb3a _sigtramp + 26
4  libsystem_platform.dylib 0x00007fd27403eca8 _sigtramp + 3285004680
5  libsystem_c.dylib        0x00007fffb01f0420 abort + 129
6  libsystem_c.dylib        0x00007fffb01b7893 basename_r + 0
7  ClangdTests              0x000000010b15ac12
llvm::PrettyStackTraceEntry::~PrettyStackTraceEntry() + 66
8  ClangdTests              0x000000010b4f58e2 clang::ParseAST(clang::Sema&,
bool, bool) + 850
9  ClangdTests              0x000000010a73166a clang::FrontendAction::Execute()
+ 138
10 ClangdTests              0x000000010a76d375
clang::PrecompiledPreamble::Build(clang::CompilerInvocation const&,
llvm::MemoryBuffer const*, clang::PreambleBounds, clang::DiagnosticsEngine&,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>,
std::__1::shared_ptr<clang::PCHContainerOperations>, bool,
clang::PreambleCallbacks&) + 3573
11 ClangdTests              0x000000010a1d96a5
clang::clangd::buildPreamble(llvm::StringRef, clang::CompilerInvocation&,
std::__1::shared_ptr<clang::clangd::PreambleData const>,
clang::tooling::CompileCommand const&, clang::clangd::ParseInputs const&, bool,
std::__1::function<void (clang::ASTContext&,
std::__1::shared_ptr<clang::Preprocessor>, clang::clangd::CanonicalIncludes
const&)>) + 2453
12 ClangdTests              0x000000010a252e5e void llvm::unique_function<void
()>::CallImpl<clang::clangd::(anonymous
namespace)::ASTWorker::update(clang::clangd::ParseInputs,
clang::clangd::WantDiagnostics)::$_0>(void*) + 2334
13 ClangdTests              0x000000010a24eb6e void llvm::unique_function<void
()>::CallImpl<clang::clangd::(anonymous
namespace)::ASTWorker::create(llvm::StringRef,
clang::clangd::GlobalCompilationDatabase const&,
clang::clangd::TUScheduler::ASTCache&, clang::clangd::AsyncTaskRunner*,
clang::clangd::Semaphore&, std::__1::chrono::duration<long long,
std::__1::ratio<1l, 1000000000l> >, bool,
clang::clangd::ParsingCallbacks&)::$_5>(void*) + 3726
14 ClangdTests              0x000000010a259584 void*
std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
std::__1::default_delete<std::__1::__thread_struct> >,
clang::clangd::AsyncTaskRunner::runAsync(llvm::Twine const&,
llvm::unique_function<void ()>)::$_4, std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >,
llvm::unique_function<void ()>,
llvm::detail::scope_exit<clang::clangd::AsyncTaskRunner::runAsync(llvm::Twine
const&, llvm::unique_function<void ()>)::$_0> > >(void*) + 244
15 libsystem_pthread.dylib  0x00007fffb037593b _pthread_body + 180
16 libsystem_pthread.dylib  0x00007fffb0375887 _pthread_body + 0
17 libsystem_pthread.dylib  0x00007fffb037508d thread_start + 13
index AST for /clangd-test/foo.cpp (main=false):
  symbol slab: 2 symbols, 4688 bytes
  ref slab: 0 symbols, 0 refs, 136 bytes
Built preamble of size 200960 for file /clangd-test/foo.cpp
index AST for /clangd-test/foo.cpp (main=true):
  symbol slab: 0 symbols, 128 bytes
  ref slab: 0 symbols, 0 refs, 136 bytes

../../llvm/lib/Support/PrettyStackTrace.cpp assumes that if
LLVM_ENABLE_THREADS=OFF, then there are no threads, but clangd starts threads
anyways.

We don't need a great fix here (clangd with threads off won't work great), just
one that works. Options:

- If it's not too hard to do, make clangd not spawn threads if
LLVM_ENABLE_THREADS=OFF

- Add some lit setting that detects if threads are on and add a REQUIRES line
for that to those tests

- Check for LLVM_ENABLE_THREADS=OFF in the cmake build and omti clangd (and all
its tests) from the build completely in that case

- Your idea here

[Quuxplusone]

TL;DR: I think we should skip building clangd if threads are off, I'll send a
patch.

Clangd won't work properly without threads as currently designed.
And (as a product) there isn't really any value in doing so, developers don't
use it on platforms that don't have threads available.
Am I missing something about why this configuration is important?

We do have a "run-synchronously" flag that stops threads spawning in most
situations, but it's limited: basically designed to make output deterministic
enough for lit tests. Not all features work properly in this mode, and some
threads are still spawned where effects aren't observable.
And it's not actually usable: operations that should be fast (code completion)
will get scheduled behind those that are very slow (rebuilding precompiled
preambles).

We have lots of unit tests that test threading behavior (e.g. that operations
actually are asynchronous). We could #ifdef them all. It doesn't seem worth
maintaining for a configuration we don't support.

> Check for LLVM_ENABLE_THREADS=OFF in the cmake build and omti clangd (and all
its tests) from the build completely in that case

I'll try to remember how CMake works, and send a patch :-)

[Quuxplusone]

It was suggested to me it would be better to disable building all of clang-
tools-extra in this configuration.

Advantages:
 - simpler to describe and implement: a single CMake root governs clang-tools-extra
 - less configuration space for these tools to support

Do you/we know of anyone who actually needs to build in such a configuration?

[Quuxplusone]

Chromium builds some things (e.g. clang) with threads off. It's a supported build configuration of LLVM. Some (most) of the tools in clang-tools-extra work fine with threads off.

[Quuxplusone]

Right.

There are apparently no buildbots that build CTE without threads, so I'm not
sure how supported it is for that subtree.

CTE is unfortunately set up in a pretty monolithic way where it's awkward to
enable/disable particular tools.

But this was a good excuse to split out the clangd build rules (r359424).

So:
 - I'll add a flag to disable building clangd
 - the rest of clang-tools-extra can stay in its ambiguously-supported state

[Quuxplusone]

Sounds great. Thanks much!