[PATCH] D135414: [clang][deps] Don't share in-memory VFS across scans

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Hi @jansvoboda11 , we (Linaro) happen to be one of the few running a bot with threading disabled (for reasons that aren't important here). It has been red a lot lately so you won't have seen the report from it.

https://lab.llvm.org/buildbot/#/builders/178/builds/3045

This does not fail on our other bots and I think `LLVM_ENABLE_THREADS=OFF` is what breaks it.

>From a working build:

  $ /home/david.spickett/build-llvm-arm/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 4 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
  Building DependencyScanningWorker
  Building DependencyScanningWorker
  Building DependencyScanningWorker
  Building DependencyScanningWorker
  BaseFS was not null
  BaseFS was not null
  BaseFS moved
  BaseFS moved

>From a build with no threading:

  $ /home/david.spickett/build-llvm-arm-broken/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 4 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
  Building DependencyScanningWorker
  BaseFS was not null
  BaseFS moved
  clang-scan-deps: /home/david.spickett/llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:393: bool clang::tooling::dependencies::DependencyScanningWorker::computeDependencies(llvm::StringRef, const std::vector<std::string> &, clang::tooling::dependencies::DependencyConsumer &, clang::DiagnosticConsumer &, llvm::Optional<StringRef>): Assertion `BaseFS' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
  Stack dump:
  0.      Program arguments: /home/david.spickett/build-llvm-arm-broken/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 4 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
  Aborted (core dumped)

I think I narrowed it down to the move you added (I put a comment on it). When threading is enabled 4 workers are made and so no one tries to use `BaseFs` after it has been moved from. When only one worker is made, it attempts to use after move `BaseFs`.

In fact, if you give `-j 1` to the threaded version, it also crashes:

  $ /home/david.spickett/build-llvm-arm/bin/clang-scan-deps -compilation-database /home/david.spickett/build-llvm-arm-broken/tools/clang/test/Cla
  ngScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 1 -format experimental-full -mode preprocess-dependency-directives -module-name=header1
  Building DependencyScanningWorker
  BaseFS was not null
  BaseFS moved
  clang-scan-deps: /home/david.spickett/llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:393: bool clang::tooling::dependencies::DependencyScanningWorker::computeDependencies(llvm::StringRef, const std::vector<std::string> &, clang::tooling::dependencies::DependencyConsumer &, clang::DiagnosticConsumer &, llvm::Optional<StringRef>): Assertion `BaseFS' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
  Aborted (core dumped)

I think the same sort of thing would happen if you said `-j N+1` when you had N cores? One worker would be run twice? I'm not up on all the details.

So yeah, to reproduce build without threading or add `-j 1`.

Do you have an idea how this might be handled? If you are moving you clearly want to transfer ownership, but I'm not sure if the `BaseFs` can just be rebuilt each time. I suppose if N workers can each have a `BaseFs`, a single worker could recreate it as needed.

Let me know what you think there.



================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:401
+    auto OverlayFS = llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(
+        std::move(BaseFS));
+    auto InMemoryFS =
----------------
This move seems like a problem when only one worker is made.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135414/new/

https://reviews.llvm.org/D135414