[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Steven Noonan via Phabricator via llvm-commits]

tycho added a comment.

I did some local work to make this build and pass almost all tests on Linux as well, not to make use of the multi-process features but just to avoid having a separate "for Windows only" branch, and to ensure tests pass across platforms. Unfortunately my change is not at all acceptable for inclusion because it's extremely Linux-specific, and Unix/Program.inc very explicitly says "only use generic UNIX code here". I use `pidfd`s and a `timerfd` combined with `epoll_wait` to implement `sys::WaitMany`. It works well, but I'd be concerned about how to implement this as smoothly for any other *nix platform. pidfd/timerfd/epoll made it downright easy, but I started off trying to look at options that would work more universally on e.g. Linux *and* the BSDs, but I couldn't find anything that wouldn't require building a Rube-Goldberg machine juggling timers, alarms, signals, etc, etc.

One thing I noticed in this was one test in particular: `Clang :: Driver/cl-fallback.c`. It seems to expose a small design oversight. Since we don't ever use `Command::Execute` or the overrides for it, we don't ever fall back to the other command in `FallbackCommand::Execute`.

Another thing I see as a big problem for this patch is the change in return value for `sys::ExecuteAndWait`. I think the rationale of using `bool` would be a good one in a new project: separating execution failure from child process runtime failure makes sense. But changing it now does cause problems if you merge patches that expect the old return values, and the build silently continues (i.e. `if (llvm::sys::ExecuteAndWait(...))` is a common construct). I noticed this in particular when I rebased on top of the LLVM master branch, as there were several new `sys::ExecuteAndWait` callers (e.g. `llvm/tools/llvm-reduce`). The difference in return type would probably be less dramatic if it didn't invert the logic (i.e. previously 0 == success, but now `true` means success and `0 != true`, so the whole `if(ExecuteAndWait` thing breaks terribly). It'd actually be better if the function was renamed or took a different set of arguments, just to cause the build to explicitly break so the callers could be addressed.

Anyway, I'd love to see this patch or a successor to it move forward, as we'd like to start building things with LLVM on *all* platforms where I work, and this would greatly help with the issues for the Visual Studio users (which are the majority of the prospective users at my office).


Repository:
  rC Clang

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

https://reviews.llvm.org/D52193