Basic: Support named pipes natively in SourceManager

[dexonsmith]

Summary

Handle named pipes natively in SourceManager, removing a call to
SourceManager::overrideFileContents in
CompilerInstance::InitializeSourceManager. This is a blocker for
sinking the content cache down to FileManager (which will incidently
sink this new named pipe logic with it).

SourceManager usually checks if the file entry's size matches the
eventually loaded buffer, but that's now skipped for named pipes since
the stat won't reflect the full size. Since we can't trust
ContentsEntry->getSize(), we also need shift the check for files that
are too large until after the buffer is loaded... and load the buffer
immediately in createFileID so that no client gets a bad value from
ContentCache::getSize. FileManager::getBufferForFile also needs to
treat these files as volatile when loading the buffer.

Native support in SourceManager means that named pipes can also be
#included, and clang/test/Misc/dev-fd-fs.c was expanded to check for
that.

This is a new version of 3b18a594c7717a328c33b9c1eba675e9f4bd367c, which
was reverted in b34632201987eed369bb7ef4646f341b901c95b8 since it was
missing the SourceManager changes.

[dexonsmith]

Note that the original commit was reviewed here: https://reviews.llvm.org/D90733.

[jansvoboda11]

Left one question.

clang/lib/Basic/SourceManager.cpp

553

What's the reason behind duplicating the logic here instead of calling createFileID with const FileEntry *SourceFile?

[dexonsmith]

Updates:

• Rebased on top of https://reviews.llvm.org/D92984, which drops the const FileEntry* version of createFileID.
• Add a test for #include-ing from a named pipe, which now works because the named pipe support is native to SourceManager instead of special logic in CompilerInstance.
• Updated the commit message / summary / title to better reflect the commit content. The new changes are bigger than the original, and I realized that describing it as a revert-of-a-revert+changes was inaccurate.

[dexonsmith]

clang/lib/Basic/SourceManager.cpp

553

Yeah, I noticed the duplication too; and then I removed the other one with https://reviews.llvm.org/D92984, but forgot to rebase this on top of it. Done now!

[arphaman]

Left one minor suggestion , but LGTM otherwise

clang/lib/Basic/SourceManager.cpp

165

Would it make sense to do the diagnoseFileTooLarge check for both files and named pipes here unconditionally instead of having two separate checks? It appears that it shouldn't really matter if we check the ContentsEntry or the Buffer size for regular files, as their sizes are expected to match anyway.

[dexonsmith]

Thanks for the review!

clang/lib/Basic/SourceManager.cpp

165

To explain, the reason I split it was to be able to return faster (don't bother loading the buffer) if it's going to be the wrong size... but an mmap is cheap and the error path isn't worth micro-optimizing.

I agree it's cleaner to just move it. I'll update to do that.

clang/lib/Basic/FileManager.cpp

 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
 FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
                               bool RequiresNullTerminator) {
   uint64_t FileSize = Entry->getSize();
   // If there's a high enough chance that the file have changed since we
   // got its size, force a stat before opening it.
-  if (isVolatile)
+  if (isVolatile || Entry->isNamedPipe())
     FileSize = -1;
 
   StringRef Filename = Entry->getName();
   // If the file is already open, use the open file descriptor.
   if (Entry->File) {
     auto Result = Entry->File->getBuffer(Filename, FileSize,
                                          RequiresNullTerminator, isVolatile);
     Entry->closeFile();

clang/lib/Basic/SourceManager.cpp

if (Buffer)
     return Buffer->getMemBufferRef();
   if (!ContentsEntry)
     return None;
 
   // Start with the assumption that the buffer is invalid to simplify early
   // return paths.
   IsBufferInvalid = true;
 
-  // Check that the file's size fits in an 'unsigned' (with room for a
-  // past-the-end value). This is deeply regrettable, but various parts of
-  // Clang (including elsewhere in this file!) use 'unsigned' to represent file
-  // offsets, line numbers, string literal lengths, and so on, and fail
-  // miserably on large source files.
-  if ((uint64_t)ContentsEntry->getSize() >=
-      std::numeric_limits<unsigned>::max()) {
-    if (Diag.isDiagnosticInFlight())
-      Diag.SetDelayedDiagnostic(diag::err_file_too_large,
-                                ContentsEntry->getName());
-    else
-      Diag.Report(Loc, diag::err_file_too_large)
-        << ContentsEntry->getName();
-
-    return None;
-  }
-
   auto BufferOrError = FM.getBufferForFile(ContentsEntry, IsFileVolatile);
 
   // If we were unable to open the file, then we are in an inconsistent
   // situation where the content cache referenced a file which no longer
   // exists. Most likely, we were using a stat cache with an invalid entry but
   // the file could also have been removed during processing. Since we can't
   // really deal with this situation, just create an empty buffer.
   if (!BufferOrError) {
     if (Diag.isDiagnosticInFlight())
       Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,
                                 ContentsEntry->getName(),
                                 BufferOrError.getError().message());
     else
       Diag.Report(Loc, diag::err_cannot_open_file)
           << ContentsEntry->getName() << BufferOrError.getError().message();
 
     return None;
   }
 
   Buffer = std::move(*BufferOrError);
 
-  // Check that the file's size is the same as in the file entry (which may
+  // Check that the file's size fits in an 'unsigned' (with room for a
+  // past-the-end value). This is deeply regrettable, but various parts of
+  // Clang (including elsewhere in this file!) use 'unsigned' to represent file
+  // offsets, line numbers, string literal lengths, and so on, and fail
+  // miserably on large source files.
+  //
+  // Note: ContentsEntry could be a named pipe, in which case
+  // ContentsEntry::getSize() could have the wrong size. Use
+  // MemoryBuffer::getBufferSize() instead.
+  if (Buffer->getBufferSize() >= std::numeric_limits<unsigned>::max()) {
+    if (Diag.isDiagnosticInFlight())
+      Diag.SetDelayedDiagnostic(diag::err_file_too_large,
+                                ContentsEntry->getName());
+    else
+      Diag.Report(Loc, diag::err_file_too_large)
+        << ContentsEntry->getName();
+
+    return None;
+  }
+
+  // Unless this is a named pipe (in which case we can handle a mismatch),
+  // check that the file's size is the same as in the file entry (which may
   // have come from a stat cache).
-  if (Buffer->getBufferSize() != (size_t)ContentsEntry->getSize()) {
+  if (!ContentsEntry->isNamedPipe() &&
+      Buffer->getBufferSize() != (size_t)ContentsEntry->getSize()) {
     if (Diag.isDiagnosticInFlight())
       Diag.SetDelayedDiagnostic(diag::err_file_modified,
                                 ContentsEntry->getName());
     else
       Diag.Report(Loc, diag::err_file_modified)
         << ContentsEntry->getName();
 
     return None;
   }
 

 }
 
 FileID SourceManager::createFileID(FileEntryRef SourceFile,
                                    SourceLocation IncludePos,
                                    SrcMgr::CharacteristicKind FileCharacter,
                                    int LoadedID, unsigned LoadedOffset) {
   SrcMgr::ContentCache &IR = getOrCreateContentCache(&SourceFile.getFileEntry(),
                                                      isSystem(FileCharacter));
+
+  // If this is a named pipe, immediately load the buffer to ensure subsequent
+  // calls to ContentCache::getSize() are accurate.
+  if (IR.ContentsEntry->isNamedPipe())
+    (void)IR.getBufferOrNone(Diag, getFileManager(), SourceLocation());
+
   return createFileIDImpl(IR, SourceFile.getName(), IncludePos, FileCharacter,
                           LoadedID, LoadedOffset);
 }
 
 /// Create a new FileID that represents the specified memory buffer.
 ///
 /// This does no caching of the buffer and takes ownership of the
 /// MemoryBuffer, so only pass a MemoryBuffer to this once.

clang/lib/Frontend/CompilerInstance.cpp

bool CompilerInstance::InitializeSourceManager(const FrontendInputFile &Input,
   if (InputFile != "-") {
     auto FileOrErr = FileMgr.getFileRef(InputFile, /*OpenFile=*/true);
     if (!FileOrErr) {
       // FIXME: include the error in the diagnostic.
       consumeError(FileOrErr.takeError());
       Diags.Report(diag::err_fe_error_reading) << InputFile;
       return false;
     }
-    FileEntryRef File = *FileOrErr;
 
-    // The natural SourceManager infrastructure can't currently handle named
-    // pipes, but we would at least like to accept them for the main
-    // file. Detect them here, read them with the volatile flag so FileMgr will
-    // pick up the correct size, and simply override their contents as we do for
-    // STDIN.
-    if (File.getFileEntry().isNamedPipe()) {
-      auto MB =
-          FileMgr.getBufferForFile(&File.getFileEntry(), /*isVolatile=*/true);
-      if (MB) {
-        // Create a new virtual file that will have the correct size.
-        FileEntryRef FE =
-            FileMgr.getVirtualFileRef(InputFile, (*MB)->getBufferSize(), 0);
-        SourceMgr.overrideFileContents(FE, std::move(*MB));
-        SourceMgr.setMainFileID(
-            SourceMgr.createFileID(FE, SourceLocation(), Kind));
-      } else {
-        Diags.Report(diag::err_cannot_open_file) << InputFile
-                                                 << MB.getError().message();
-        return false;
-      }
-    } else {
-      SourceMgr.setMainFileID(
-          SourceMgr.createFileID(File, SourceLocation(), Kind));
-    }
+        SourceMgr.createFileID(*FileOrErr, SourceLocation(), Kind));
   } else {
     llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> SBOrErr =
         llvm::MemoryBuffer::getSTDIN();
     if (std::error_code EC = SBOrErr.getError()) {
       Diags.Report(diag::err_fe_error_reading_stdin) << EC.message();
       return false;
     }
     std::unique_ptr<llvm::MemoryBuffer> SB = std::move(SBOrErr.get());

clang/test/Misc/dev-fd-fs.c

 // Check that we can operate on files from /dev/fd.
 // REQUIRES: dev-fd-fs
 // REQUIRES: shell
 
 // Check reading from named pipes. We cat the input here instead of redirecting
 // it to ensure that /dev/fd/0 is a named pipe, not just a redirected file.
 //
 // RUN: cat %s | %clang -x c /dev/fd/0 -E > %t
 // RUN: FileCheck --check-prefix DEV-FD-INPUT < %t %s
 //
+// RUN: cat %s | %clang -x c %s -E -DINCLUDE_FROM_STDIN > %t
+// RUN: FileCheck --check-prefix DEV-FD-INPUT \
+// RUN:           --check-prefix DEV-FD-INPUT-INCLUDE < %t %s
+//
+// DEV-FD-INPUT-INCLUDE: int w;
 // DEV-FD-INPUT: int x;
+// DEV-FD-INPUT-INCLUDE: int y;
 
 
 // Check writing to /dev/fd named pipes. We use cat here as before to ensure we
 // get a named pipe.
 //
 // RUN: %clang -x c %s -E -o /dev/fd/1 | cat > %t
 // RUN: FileCheck --check-prefix DEV-FD-FIFO-OUTPUT < %t %s
 //
 // DEV-FD-FIFO-OUTPUT: int x;
 
 
 // Check writing to /dev/fd regular files.
 //
 // RUN: %clang -x c %s -E -o /dev/fd/1 > %t
 // RUN: FileCheck --check-prefix DEV-FD-REG-OUTPUT < %t %s
 //
 // DEV-FD-REG-OUTPUT: int x;
 
+#ifdef INCLUDE_FROM_STDIN
+#undef INCLUDE_FROM_STDIN
+int w;
+#include "/dev/fd/0"
+int y;
+#else
 int x;
+#endif