@llvm/pr-subscribers-llvm-support

Author: Michał Górny (mgorny)

diff --git a/llvm/lib/Support/Unix/Jobserver.inc b/llvm/lib/Support/Unix/Jobserver.inc
index 53bf7f288ca1f..300fb5e55045e 100644
--- a/llvm/lib/Support/Unix/Jobserver.inc
+++ b/llvm/lib/Support/Unix/Jobserver.inc
@@ -19,14 +19,6 @@
 #include <unistd.h>
 
 namespace {
-/// Returns true if the given file descriptor is a FIFO (named pipe).
-bool isFifo(int FD) {
-  struct stat StatBuf;
-  if (::fstat(FD, &StatBuf) != 0)
-    return false;
-  return S_ISFIFO(StatBuf.st_mode);
-}
-
 /// Returns true if the given file descriptors are valid.
 bool areFdsValid(int ReadFD, int WriteFD) {
   if (ReadFD == -1 || WriteFD == -1)
@@ -75,7 +67,7 @@ JobserverClientImpl::JobserverClientImpl(const JobserverConfig &Config) {
   case JobserverConfig::PosixFifo:
     // Open the FIFO for reading. It must be non-blocking and close-on-exec.
     ReadFD = ::open(Config.Path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC);
-    if (ReadFD < 0 || !isFifo(ReadFD)) {
+    if (ReadFD < 0) {
       if (ReadFD >= 0)
         ::close(ReadFD);
       ReadFD = -1;

This change looks good functionally, but I think it leaves some confusing naming and comments behind.

Previously, JobserverConfig::PosixFifo was tightly coupled to a real POSIX FIFO (we enforced S_ISFIFO), so the fifo naming and comments in this code matched the actual kernel object. Now that we intentionally accept any endpoint that implements the jobserver token protocol (including FUSE files or CUSE character devices), the FIFO terminology becomes misleading: it's really 'FIFO-compatible endpoint as described by fifo: in MAKEFLAGS', not necessarily a kernel FIFO.

It would be clearer to future readers if we adjusted the naming/comments in a follow-up:

• In Unix/Jobserver.inc:

  • The constructor comment around L40-L44 ("For FIFO-based jobservers, it opens the named pipe") and the comment on release() ("For FIFO-based jobservers, the write FD might not be open yet") should be reworded to something like 'FIFO-style' or 'path-based jobservers'.
  • The member FifoPath and related debug messages ("opening FIFO", "Opened FIFO as new WriteFD") could be renamed to something like EndpointPath or JobserverPath.

• In Jobserver.cpp:

  • The FifoPath local and JobserverConfig::FifoPath member share the same assumption and may deserve a more generic name or an updated comment explaining that fifo: is a protocol tag, not a strict type guarantee.

It might be worth adding a small regression test to lock in the new behavior around fifo: endpoints.

Right now the Unix jobserver tests (JobserverClientTest.UnixClientFifo and the JobserverStrategyTest cases) only exercise the "happy path" with a real FIFO created by ScopedFifo. Before this patch, JobserverClientImpl would reject any --jobserver-auth=fifo:PATH that did not point to a S_ISFIFO object; after this patch we intentionally accept non-FIFO endpoints as long as open() succeeds.

To make that intent explicit and prevent future regressions, I would suggest adding a test alongside UnixClientFifo that:

• Creates a regular temporary file (or uses createTemporaryFile but does not replace it with a FIFO).
• Sets MAKEFLAGS to --jobserver-auth=fifo: using ScopedEnvironment.
• Calls JobserverClient::getInstance() and asserts we get a non-null client and that it is considered valid (no "invalid FIFO" rejection).

This would clearly document that fifo: is now a protocol tag rather than a strict POSIX FIFO requirement, and that we deliberately allow FUSE/CUSE-backed implementations presenting themselves under a fifo: URL.

Rebased and added a test so far. I've just copied the whole logic from UnixClientFifo test, since it also works fine for an empty file.

@yxsamliu ping