[Clang] Fix expansion of response files in -Wp after integrated-cc1 change

[aganea]

Summary

After rGb4a99a061f517e60985667e39519f60186cbb469, passing a response file such as -Wp,@a.rsp wasn't working anymore because .rsp expansion happens inside clang's main() function.

I'm noticing along the way that explicit arguments in -Wp are parsed in the driver, whereas arguments inside a response file are parsed in the -cc1 tool. Is that the desired behavior? This was the previous behavior but I'm not sure it's the right behavior.

[wenlei]

Is that the desired behavior? This was the previous behavior but I'm not sure it's the right behavior.

Good point. It might be safer to keep that previous behavior though.

Thanks for the quick fix, LGTM!

[hans]

clang/tools/driver/driver.cpp

338

This looks pretty hacky.

Can you explain more what the current problem is and how you're proposing to fix it? I'm not familiar with the -Wp flag and how it relates to response files.

[aganea]

clang/tools/driver/driver.cpp

338

The problem is that for -Wp,@file.rsp, file.rsp is not expanded anymore, because previously it was done in the cc1 process, at L379 below.

This patch calls back into ExecuteClangTool instead of ExecuteCC1Tool. This was my initial proposal for rGb4a99a061f517e60985667e39519f60186cbb469, purposely to fix this issue (which I forgot about)

An alternate way is to duplicate L379 into ExecuteCC1Tool, but I felt that wasn't pretty and opens the door to more code duplication down the line. See D73120 for an alternate implementation.

[hans]

clang/tools/driver/driver.cpp

338

I think that if cc1 is supposed to expand response files, then let's make that explicit.

I don't think the "reenter" flag in D73120 is necessary: if cc1 is supposed to expand response files, let's make it do that always.

[aganea]

Abandoning this in favor of D73120. Please take a look at the other patch.

clang/include/clang/Driver/Driver.h

public:
 
   /// Whether the driver is generating diagnostics for debugging purposes.
   unsigned CCGenDiagnostics : 1;
 
   /// Pointer to the ExecuteCC1Tool function, if available.
   /// When the clangDriver lib is used through clang.exe, this provides a
   /// shortcut for executing the -cc1 command-line directly, in the same
   /// process.
-  typedef int (*CC1ToolFunc)(ArrayRef<const char *> argv);
+  typedef int (*CC1ToolFunc)(SmallVectorImpl<const char *> &);
   CC1ToolFunc CC1Main = nullptr;
 
 private:
   /// Raw target triple.
   std::string TargetTriple;
 
   /// Name to use when invoking gcc/g++.
   std::string CCCGenericGCCName;

clang/lib/Driver/Job.cpp

 
 int CC1Command::Execute(ArrayRef<llvm::Optional<StringRef>> /*Redirects*/,
                         std::string *ErrMsg, bool *ExecutionFailed) const {
   PrintFileNames();
 
   SmallVector<const char *, 128> Argv;
   Argv.push_back(getExecutable());
   Argv.append(getArguments().begin(), getArguments().end());
-  Argv.push_back(nullptr);
 
   // This flag simply indicates that the program couldn't start, which isn't
   // applicable here.
   if (ExecutionFailed)
     *ExecutionFailed = false;
 
   llvm::CrashRecoveryContext CRC;
   CRC.DumpStackAndCleanupOnFailure = true;

clang/test/Driver/Wp-args.c

 
 // RUN: %clang -target i386-pc-linux-gnu -### \
 // RUN:   -Wp,-MMD -fsyntax-only %s 2> %t
 // RUN: FileCheck -check-prefix MMD < %t %s
 
 // MMD: "-cc1"
 // MMD-NOT: -MMD
 // MMD: "-dependency-file" "Wp-args.d"
+
+// Ensure response files are properly expanded with -Wp
+// RUN: echo -rewrite-macros > %t.rsp
+// RUN: %clang -Wp,@%t.rsp -E %s | FileCheck -check-prefix RSP %s
+
+// RSP: inception

clang/tools/driver/driver.cpp

static void getCLEnvVarOptions(std::string &EnvValue, llvm::StringSaver &Saver,
                                SmallVectorImpl<const char *> &Opts) {
   llvm::cl::TokenizeWindowsCommandLine(EnvValue, Saver, Opts);
   // The first instance of '#' should be replaced with '=' in each option.
   for (const char *Opt : Opts)
     if (char *NumberSignPtr = const_cast<char *>(::strchr(Opt, '#')))
       *NumberSignPtr = '=';
 }
 
-static int ExecuteCC1Tool(ArrayRef<const char *> argv);
-
 static void SetBackdoorDriverOutputsFromEnvVars(Driver &TheDriver) {
   // Handle CC_PRINT_OPTIONS and CC_PRINT_OPTIONS_FILE.
   TheDriver.CCPrintOptions = !!::getenv("CC_PRINT_OPTIONS");
   if (TheDriver.CCPrintOptions)
     TheDriver.CCPrintOptionsFilename = ::getenv("CC_PRINT_OPTIONS_FILE");
 
   // Handle CC_PRINT_HEADERS and CC_PRINT_HEADERS_FILE.
   TheDriver.CCPrintHeaders = !!::getenv("CC_PRINT_HEADERS");
if (llvm::sys::fs::exists(InstalledPathParent))
     TheDriver.setInstalledDir(InstalledPathParent);
 }
 
 static int ExecuteCC1Tool(ArrayRef<const char *> argv) {
   // If we call the cc1 tool from the clangDriver library (through
   // Driver::CC1Main), we need to clean up the options usage count. The options
   // are currently global, and they might have been used previously by the
   // driver.
-  llvm::cl::ResetAllOptionOccurrences();
   StringRef Tool = argv[1];
   void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
   if (Tool == "-cc1")
     return cc1_main(argv.slice(2), argv[0], GetExecutablePathVP);
   if (Tool == "-cc1as")
     return cc1as_main(argv.slice(2), argv[0], GetExecutablePathVP);
   if (Tool == "-cc1gen-reproducer")
     return cc1gen_reproducer_main(argv.slice(2), argv[0], GetExecutablePathVP);
 
   // Reject unknown tools.
   llvm::errs() << "error: unknown integrated tool '" << Tool << "'. "
                << "Valid tools include '-cc1' and '-cc1as'.\n";
   return 1;
 }
 
-int main(int argc_, const char **argv_) {
-  noteBottomOfStack();
-  llvm::InitLLVM X(argc_, argv_);
-  SmallVector<const char *, 256> argv(argv_, argv_ + argc_);
-
-  if (llvm::sys::Process::FixupStandardFileDescriptors())
-    return 1;
-
-  llvm::InitializeAllTargets();
+int ExecuteClangTool(SmallVectorImpl<const char *> &argv) {
   auto TargetAndMode = ToolChain::getTargetAndModeFromProgramName(argv[0]);
 
+  static unsigned ReenteranceCount;
+  if (ReenteranceCount++)
+    llvm::cl::ResetAllOptionOccurrences();
+
   llvm::BumpPtrAllocator A;
   llvm::StringSaver Saver(A);
 
   // Parse response files using the GNU syntax, unless we're in CL mode. There
   // are two ways to put clang in CL compatibility mode: argv[0] is either
   // clang-cl or cl, or --driver-mode=cl is on the command line. The normal
   // command line parsing can't happen until after response file parsing, so we
   // have to manually search for a --driver-mode=cl argument the hard way.
int ExecuteClangTool(SmallVectorImpl<const char *> &argv) {
   SetInstallDir(argv, TheDriver, CanonicalPrefixes);
   TheDriver.setTargetAndMode(TargetAndMode);
 
   insertTargetAndModeArgs(TargetAndMode, argv, SavedStrings);
 
   SetBackdoorDriverOutputsFromEnvVars(TheDriver);
 
   if (!UseNewCC1Process) {
-    TheDriver.CC1Main = &ExecuteCC1Tool;
+    TheDriver.CC1Main = &ExecuteClangTool;
     // Ensure the CC1Command actually catches cc1 crashes
     llvm::CrashRecoveryContext::Enable();
   }
 
   std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(argv));
   int Res = 1;
   if (C && !C->containsError()) {
     SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
#ifdef _WIN32
   if (Res < 0)
     Res = 1;
 #endif
 
   // If we have multiple failing commands, we return the result of the first
   // failing command.
   return Res;
 }
+
+int main(int argc_, const char **argv_) {
+  noteBottomOfStack();
+  llvm::InitLLVM X(argc_, argv_);
+  SmallVector<const char *, 256> argv(argv_, argv_ + argc_);
+
+  if (llvm::sys::Process::FixupStandardFileDescriptors())
+    return 1;
+
+  llvm::InitializeAllTargets();
+  return ExecuteClangTool(argv);
+}