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

567

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

567

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

167

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

167

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

ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM,
   // 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()) {
+  auto diagnoseFileTooLarge = [&](uint64_t Size) {
+    if (Size < std::numeric_limits<unsigned>::max())
+      return false;
+
     if (Diag.isDiagnosticInFlight())
       Diag.SetDelayedDiagnostic(diag::err_file_too_large,
                                 ContentsEntry->getName());
     else
       Diag.Report(Loc, diag::err_file_too_large)
         << ContentsEntry->getName();
 
+    return true;
+  };
+
+  // Check ContentsEntry's size, unless it's a named pipe (in which case we
+  // need to load the buffer first).
+  if (!ContentsEntry->isNamedPipe() &&
+      diagnoseFileTooLarge(ContentsEntry->getSize()))
     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
-  // have come from a stat cache).
-  if (Buffer->getBufferSize() != (size_t)ContentsEntry->getSize()) {
+  // If this is a named pipe, check the buffer to see if it's too big.
+  // Otherwise, check that the file's size is the same as in the file entry
+  // (which may have come from a stat cache).
+  if (ContentsEntry->isNamedPipe()) {
+    // Check the buffer size directly if this is a named pipe.
+    if (diagnoseFileTooLarge(Buffer->getBufferSize()))
+      return None;
+  } else if (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;

 /// This translates NULL into standard input.
 FileID SourceManager::createFileID(const FileEntry *SourceFile,
                                    SourceLocation IncludePos,
                                    SrcMgr::CharacteristicKind FileCharacter,
                                    int LoadedID, unsigned LoadedOffset) {
   assert(SourceFile && "Null source file!");
   SrcMgr::ContentCache &IR =
       getOrCreateContentCache(SourceFile, 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);
 }
 
 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

if (InputFile != "-") {
     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.
-        const FileEntry *FE =
-            FileMgr.getVirtualFile(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));
-    }
   } 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());