[Preamble] Check system dependencies in preamble too

[ilya-biryukov]

Summary

PrecompiledPreamble hasn't checked the system dependencies changed
before. This result in invalid preamble not being rebuilt if headers
that changed were found in -isystem include paths.

This pattern is sometimes used to avoid showing warnings in third
party code, so we want to correctly handle those cases.

Tested in clangd, see the follow-up patch.

[sammccall]

This seems like it might be a nontrivial performance hit (it's going to result in stating all these files, right?).
Agreed it's important for correctness, it's possible someone wants the performance/correctness tradeoff though.

lib/Frontend/PrecompiledPreamble.cpp

71 ↗

(On Diff #154134)

avoid errors --> suppress warnings?

[ilya-biryukov]

• Fix a comment

[ilya-biryukov]

In D48946#1152764, @sammccall wrote:

This seems like it might be a nontrivial performance hit (it's going to result in stating all these files, right?).
Agreed it's important for correctness, it's possible someone wants the performance/correctness tradeoff though.

I do agree that it's (probably) useless to stat the STL in /usr/lib for most of the users, maybe in some other cases too. Happy to add an option if that turns out to be a problem.

cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp

IntrusiveRefCntPtr<vfs::InMemoryFileSystem> PCHFS(
       new vfs::InMemoryFileSystem());
   PCHFS->addFile(PCHFilename, 0, std::move(PCHBuffer));
   IntrusiveRefCntPtr<vfs::OverlayFileSystem> Overlay(
       new vfs::OverlayFileSystem(VFS));
   Overlay->pushOverlay(PCHFS);
   return Overlay;
 }
 
+class PreambleDependencyCollector : public DependencyCollector {
+public:
+  // We want to collect all dependencies for correctness. Avoiding the real
+  // system dependencies (e.g. stl from /usr/lib) would probably be a good idea,
+  // but there is no way to distinguish between those and the ones that can be
+  // spuriously added by '-isystem' (e.g. to suppress warnings from those
+  // headers).
+  bool needSystemDependencies() override { return true; }
+};
+
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
   // A static instance to be used by all clients.
   static TemporaryFiles &getInstance();
 
 private:
   // Disallow constructing the class directly.
llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
 
   // Create a file manager object to provide access to and cache the filesystem.
   Clang->setFileManager(new FileManager(Clang->getFileSystemOpts(), VFS));
 
   // Create the source manager.
   Clang->setSourceManager(
       new SourceManager(Diagnostics, Clang->getFileManager()));
 
-  auto PreambleDepCollector = std::make_shared<DependencyCollector>();
+  auto PreambleDepCollector = std::make_shared<PreambleDependencyCollector>();
   Clang->addDependencyCollector(PreambleDepCollector);
 
   // Remap the main source file to the preamble buffer.
   StringRef MainFilePath = FrontendOpts.Inputs[0].getFile();
   auto PreambleInputBuffer = llvm::MemoryBuffer::getMemBufferCopy(
       MainFileBuffer->getBuffer().slice(0, Bounds.Size), MainFilePath);
   if (PreprocessorOpts.RetainRemappedFileBuffers) {
     // MainFileBuffer will be deleted by unique_ptr after leaving the method.