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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D52193: RFC: [clan... Steven Noonan via Phabricator via cfe-commits

Reply via email to