[Driver] Make "upgrade" of -include to include-pch optional; disable in clangd

[sammccall]

Summary

If clang is passed "-include foo.h", it will rewrite to "-include-pch foo.h.pch"
before passing it to cc1, if foo.h.pch exists.

Existence is checked, but validity is not. This is probably a reasonable
assumption for the compiler itself, but not for clang-based tools where the
actual compiler may be a different version of clang, or even GCC.
In the end, we lose our -include, we gain a -include-pch that can't be used,
and the file often fails to parse.

I would like to turn this off for all non-clang invocations (i.e.
createInvocationFromCommandLine), but we have explicit tests of this behavior
for libclang and I can't work out the implications of changing it.

Instead this patch:

• makes it optional in the driver, default on (no change)
• makes it optional in createInvocationFromCommandLine, default on (no change)
• changes driver to do IO through the VFS so it can be tested
• tests the option
• turns the option off in clangd where the problem was reported

Subsequent patches should make libclang opt in explicitly and flip the default
for all other tools. It's probably also time to extract an options struct
for createInvocationFromCommandLine.

Fixes https://github.com/clangd/clangd/issues/856
Fixes https://github.com/clangd/vscode-clangd/issues/324

[sammccall]

Kadir, sending this your way as it looks like you investigated #856.

I do plan to flip the global default, but not for libclang and friends :-( I feel like a coward.

[kadircet]

thanks, LGTM!

as discussed offline, let's move forward with what you proposed (i.e. this patch, and then flip the default while making libclang opt-in)

clang/include/clang/Driver/Driver.h

275

nit: reflow the comment

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

51

this comment feels a little bit outdated. we're definitely not turning this off and also probing is done in VFS now.
So maybe drop this and rather have a comment in header, pointing out possible failure modes due to version mismatch when this is turned on?

clang-tools-extra/clangd/Compiler.cpp

for (const auto &S : Inputs.CompileCommand.CommandLine)
     ArgStrs.push_back(S.c_str());
 
   CreateInvocationOptions CIOpts;
   CIOpts.VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
   CIOpts.CC1Args = CC1Args;
   CIOpts.RecoverOnError = true;
   CIOpts.Diags =
       CompilerInstance::createDiagnostics(new DiagnosticOptions, &D, false);
+  CIOpts.ProbePrecompiled = false;
   std::unique_ptr<CompilerInvocation> CI = createInvocation(ArgStrs, CIOpts);
   if (!CI)
     return nullptr;
   // createInvocationFromCommandLine sets DisableFree.
   CI->getFrontendOpts().DisableFree = false;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   CI->getLangOpts()->RetainCommentsFromSystemHeaders = true;
 

clang/include/clang/Driver/Driver.h

private:
   std::unique_ptr<llvm::opt::InputArgList> CfgOptions;
 
   /// Arguments originated from command line.
   std::unique_ptr<llvm::opt::InputArgList> CLOptions;
 
   /// Whether to check that input files exist when constructing compilation
   /// jobs.
   unsigned CheckInputsExist : 1;
+  /// Whether to probe for PCH files on disk, in order to upgrade
+  /// -include foo.h to -include-pch foo.h.pch.
+  unsigned ProbePrecompiled : 1;
 
 public:
   /// Force clang to emit reproducer for driver invocation. This is enabled
   /// indirectly by setting FORCE_CLANG_DIAGNOSTICS_CRASH environment variable
   /// or when using the -gen-reproducer driver flag.
   unsigned GenReproducer : 1;
 
   // getFinalPhase - Determine which compilation mode we are in and record
public:
   DiagnosticsEngine &getDiags() const { return Diags; }
 
   llvm::vfs::FileSystem &getVFS() const { return *VFS; }
 
   bool getCheckInputsExist() const { return CheckInputsExist; }
 
   void setCheckInputsExist(bool Value) { CheckInputsExist = Value; }
 
+  bool getProbePrecompiled() const { return ProbePrecompiled; }
+  void setProbePrecompiled(bool Value) { ProbePrecompiled = Value; }
+
   void setTargetAndMode(const ParsedClangName &TM) { ClangNameParts = TM; }
 
   const std::string &getTitle() { return DriverTitle; }
   void setTitle(std::string Value) { DriverTitle = std::move(Value); }
 
   std::string getTargetTriple() const { return TargetTriple; }
 
   /// Get the path to the main clang executable.

clang/include/clang/Frontend/Utils.h

struct CreateInvocationOptions {
   /// Used e.g. to probe for system headers locations.
   /// If not provided, the real filesystem is used.
   /// FIXME: the driver does perform some non-virtualized IO.
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr;
   /// Whether to attempt to produce a non-null (possibly incorrect) invocation
   /// if any errors were encountered.
   /// By default, always return null on errors.
   bool RecoverOnError = false;
+  /// Allow the driver to probe the filesystem for PCH files.
+  /// This is used to replace -include with -include-pch in the cc1 args.
+  /// FIXME: ProbePrecompiled=true is a poor, historical default.
+  /// It misbehaves if the PCH file is from GCC, has the wrong version, etc.
+  bool ProbePrecompiled = true;
   /// If set, the target is populated with the cc1 args produced by the driver.
   /// This may be populated even if createInvocation returns nullptr.
   std::vector<std::string> *CC1Args = nullptr;
 };
 
 /// Interpret clang arguments in preparation to parse a file.
 ///
 /// This simulates a number of steps Clang takes when its driver is invoked:

 
 /// Deprecated version of createInvocation with individual optional args.
 std::unique_ptr<CompilerInvocation> createInvocationFromCommandLine(
     ArrayRef<const char *> Args,
     IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
         IntrusiveRefCntPtr<DiagnosticsEngine>(),
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr,
     bool ShouldRecoverOnErrors = false,
-    std::vector<std::string> *CC1Args = nullptr);
+    std::vector<std::string> *CC1Args = nullptr, bool ProbePrecompiled = true);
 
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_UTILS_H

clang/lib/Driver/Driver.cpp

Driver::Driver(StringRef ClangExecutable, StringRef TargetTriple,
     : Diags(Diags), VFS(std::move(VFS)), Mode(GCCMode),
       SaveTemps(SaveTempsNone), BitcodeEmbed(EmbedNone),
       CXX20HeaderType(HeaderMode_None), ModulesModeCXX20(false),
       LTOMode(LTOK_None), ClangExecutable(ClangExecutable),
       SysRoot(DEFAULT_SYSROOT), DriverTitle(Title), CCCPrintBindings(false),
       CCPrintOptions(false), CCPrintHeaders(false), CCLogDiagnostics(false),
       CCGenDiagnostics(false), CCPrintProcessStats(false),
       TargetTriple(TargetTriple), Saver(Alloc), CheckInputsExist(true),
-      GenReproducer(false), SuppressMissingInputWarning(false) {
+      ProbePrecompiled(true), GenReproducer(false),
+      SuppressMissingInputWarning(false) {
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
     this->VFS = llvm::vfs::getRealFileSystem();
 
   Name = std::string(llvm::sys::path::filename(ClangExecutable));
   Dir = std::string(llvm::sys::path::parent_path(ClangExecutable));
   InstalledDir = Dir; // Provide a sensible default installed dir.
 

clang/lib/Driver/ToolChains/Clang.cpp

if (YcArg || YuArg) {
         CmdArgs.push_back(
             Args.MakeArgString(Twine("-pch-through-header=") + ThroughHeader));
       }
     }
   }
 
   bool RenderedImplicitInclude = false;
   for (const Arg *A : Args.filtered(options::OPT_clang_i_Group)) {
-    if (A->getOption().matches(options::OPT_include)) {
+    if (A->getOption().matches(options::OPT_include) &&
+        D.getProbePrecompiled()) {
       // Handling of gcc-style gch precompiled headers.
       bool IsFirstImplicitInclude = !RenderedImplicitInclude;
       RenderedImplicitInclude = true;
 
       bool FoundPCH = false;
       SmallString<128> P(A->getValue());
       // We want the files to have a name like foo.h.pch. Add a dummy extension
       // so that replace_extension does the right thing.
       P += ".dummy";
       llvm::sys::path::replace_extension(P, "pch");
-      if (llvm::sys::fs::exists(P))
+      if (D.getVFS().exists(P))
         FoundPCH = true;
 
       if (!FoundPCH) {
         llvm::sys::path::replace_extension(P, "gch");
-        if (llvm::sys::fs::exists(P)) {
+        if (D.getVFS().exists(P)) {
           FoundPCH = true;
         }
       }
 
       if (FoundPCH) {
         if (IsFirstImplicitInclude) {
           A->claim();
           CmdArgs.push_back("-include-pch");

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

Args.insert(
       "-fsyntax-only");
 
   // FIXME: We shouldn't have to pass in the path info.
   driver::Driver TheDriver(Args[0], llvm::sys::getDefaultTargetTriple(), *Diags,
                            "clang LLVM compiler", Opts.VFS);
 
   // Don't check that inputs exist, they may have been remapped.
   TheDriver.setCheckInputsExist(false);
+  TheDriver.setProbePrecompiled(Opts.ProbePrecompiled);
 
   std::unique_ptr<driver::Compilation> C(TheDriver.BuildCompilation(Args));
   if (!C)
     return nullptr;
 
   // Just print the cc1 options if -### was present.
   if (C->getArgs().hasArg(driver::options::OPT__HASH_HASH_HASH)) {
     C->getJobs().Print(llvm::errs(), "\n", true);
if (!CompilerInvocation::CreateFromArgs(*CI, CCArgs, *Diags, Args[0]) &&
       !Opts.RecoverOnError)
     return nullptr;
   return CI;
 }
 
 std::unique_ptr<CompilerInvocation> clang::createInvocationFromCommandLine(
     ArrayRef<const char *> Args, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS, bool ShouldRecoverOnErrors,
-    std::vector<std::string> *CC1Args) {
+    std::vector<std::string> *CC1Args, bool ProbePrecompiled) {
   return createInvocation(
-      Args,
-      CreateInvocationOptions{Diags, VFS, ShouldRecoverOnErrors, CC1Args});
+      Args, CreateInvocationOptions{Diags, VFS, ShouldRecoverOnErrors,
+                                    ProbePrecompiled, CC1Args});
 }

clang/unittests/Frontend/UtilsTest.cpp

 //===- unittests/Frontend/UtilsTest.cpp -----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Frontend/Utils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace {
+using testing::ElementsAre;
 
 TEST(BuildCompilerInvocationTest, RecoverMultipleJobs) {
   // This generates multiple jobs and we recover by using the first.
   std::vector<const char *> Args = {"clang", "--target=macho", "-arch",  "i386",
                                     "-arch", "x86_64",         "foo.cpp"};
   clang::IgnoringDiagConsumer D;
   CreateInvocationOptions Opts;
   Opts.RecoverOnError = true;
   Opts.Diags = clang::CompilerInstance::createDiagnostics(new DiagnosticOptions,
                                                           &D, false);
   Opts.VFS = new llvm::vfs::InMemoryFileSystem();
   std::unique_ptr<CompilerInvocation> CI = createInvocation(Args, Opts);
   ASSERT_TRUE(CI);
   EXPECT_THAT(CI->TargetOpts->Triple, testing::StartsWith("i386-"));
 }
 
+// buildInvocationFromCommandLine should not translate -include to -include-pch,
+// even if the PCH file exists.
+TEST(BuildCompilerInvocationTest, ProbePrecompiled) {
+  std::vector<const char *> Args = {"clang", "-include", "foo.h", "foo.cpp"};
+  auto FS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  FS->addFile("foo.h", 0, llvm::MemoryBuffer::getMemBuffer(""));
+  FS->addFile("foo.h.pch", 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  clang::IgnoringDiagConsumer D;
+  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
+      clang::CompilerInstance::createDiagnostics(new DiagnosticOptions, &D,
+                                                 false);
+  // Default: ProbePrecompiled is true.
+  std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
+      Args, CommandLineDiagsEngine, FS, false, nullptr);
+  ASSERT_TRUE(CI);
+  EXPECT_THAT(CI->getPreprocessorOpts().Includes, ElementsAre());
+  EXPECT_EQ(CI->getPreprocessorOpts().ImplicitPCHInclude, "foo.h.pch");
+
+  CI = createInvocationFromCommandLine(Args, CommandLineDiagsEngine, FS, false,
+                                       nullptr, /*ProbePrecompiled=*/false);
+  ASSERT_TRUE(CI);
+  EXPECT_THAT(CI->getPreprocessorOpts().Includes, ElementsAre("foo.h"));
+  EXPECT_EQ(CI->getPreprocessorOpts().ImplicitPCHInclude, "");
+}
+
 } // namespace
 } // namespace clang