Fix PR17239 by changing the semantics of the RemainingArgsClass Option kind (clang side)

[rafaelauler]

Summary

This is the Clang side of the patch in http://reviews.llvm.org/D4899

This patch aims at fixing PR17239.

Note: it depends on http://reviews.llvm.org/D4899 the llvm side of the patch, landing in the LLVM tree.

This bug happens because the /link (clang-cl.exe argument) is marked as "consume all remaining arguments". However, when inside a response file, /link should only consume all remaining arguments inside the response file where it is located, not the entire command line after expansion. The LLVM side of the patch will change the semantics of the RemainingArgsClass kind to always consume only until the end of the response file when the option originally came from a response file. There are only two options in this class: dash dash (--) and /link.

In the Clang side, I just used the ExpandedArgs class created in the LLVM side of the patch to fix this bug.

[rafaelauler]

This is the clang side of my updated patch in D4899. I updated the Driver to cope with the new LLVM API proposed in that patch:

• Call ExpandResponseFiles with MarkEOLs set to true to allow correct expansion of the /LINK argument, fixing PR17239. Only do this for the clang driver, not for clang -cc1 tools.
• Ignore nullptrs in the argv vector because they are now used to indicate end of lines.

I also updated the Driver::ParseArgStrings parameter name that had a confusing argument name that was the same of a known class used in the same scope (NFC).

[rnk]

This is tied to D4899, so I'll land it too. Thanks!

tools/driver/driver.cpp

406–409

This seems impossible. If we invoked a cc1 tool, we shouldn't have added EOL markers, right?

[rafaelauler]

Hi Reid,

Sure, thanks for reviewing it. I answered your comment below.

tools/driver/driver.cpp

406–409

I made this check just in case the user puts "-cc1" as a first argument in a response file. In this case, MarkEOLs would be true. After expanding the response files, we would end up here with MarkEOLs true (a contrived example, but possible).

[rnk]

Landed in r216281.

tools/driver/driver.cpp

406–409

OK, I added a test case for it and will commit it soon.

lib/Driver/Driver.cpp

Driver::~Driver() {
   llvm::DeleteContainerSeconds(ToolChains);
 }
 
 void Driver::ParseDriverMode(ArrayRef<const char *> Args) {
   const std::string OptName =
     getOpts().getOption(options::OPT_driver_mode).getPrefixedName();
 
   for (size_t I = 0, E = Args.size(); I != E; ++I) {
+    // Ingore nullptrs, they are response file's EOL markers
+    if (Args[I] == nullptr)
+      continue;
     const StringRef Arg = Args[I];
     if (!Arg.startswith(OptName))
       continue;
 
     const StringRef Value = Arg.drop_front(OptName.size());
     const unsigned M = llvm::StringSwitch<unsigned>(Value)
         .Case("gcc", GCCMode)
         .Case("g++", GXXMode)
         .Case("cpp", CPPMode)
         .Case("cl",  CLMode)
         .Default(~0U);
 
     if (M != ~0U)
       Mode = static_cast<DriverMode>(M);
     else
       Diag(diag::err_drv_unsupported_option_argument) << OptName << Value;
   }
 }
 
-InputArgList *Driver::ParseArgStrings(ArrayRef<const char *> ArgList) {
+InputArgList *Driver::ParseArgStrings(ArrayRef<const char *> ArgStrings) {
   llvm::PrettyStackTraceString CrashInfo("Command line argument parsing");
 
   unsigned IncludedFlagsBitmask;
   unsigned ExcludedFlagsBitmask;
   std::tie(IncludedFlagsBitmask, ExcludedFlagsBitmask) =
     getIncludeExcludeOptionFlagMasks();
 
   unsigned MissingArgIndex, MissingArgCount;
-  InputArgList *Args = getOpts().ParseArgs(ArgList.begin(), ArgList.end(),
+  InputArgList *Args = getOpts().ParseArgs(ArgStrings.begin(), ArgStrings.end(),
                                            MissingArgIndex, MissingArgCount,
                                            IncludedFlagsBitmask,
                                            ExcludedFlagsBitmask);
 
   // Check for missing argument error.
   if (MissingArgCount)
     Diag(clang::diag::err_drv_missing_argument)
       << Args->getArgString(MissingArgIndex) << MissingArgCount;

test/Driver/cl-link-at-file.c

+// PR17239 - The /link option, when inside a response file, should only extend
+// until the end of the response file (and not the entire command line)
+
+// Don't attempt slash switches on msys bash.
+// REQUIRES: shell-preserves-root
+
+// Note: %s must be preceded by -- or bound to another option, otherwise it may
+// be interpreted as a command-line option, e.g. on Mac where %s is commonly
+// under /Users.
+
+// RUN: echo /link bar.lib baz.lib > %t.args
+// RUN: touch %t.obj
+// RUN: %clang_cl -### @%t.args -- %t.obj 2>&1 | FileCheck %s -check-prefix=ARGS
+// If the "/link" option captures all remaining args beyond its response file,
+// it will also capture "--" and our input argument. In this case, Clang will
+// be clueless and will emit "argument unused" warnings. If PR17239 is properly
+// fixed, this should not happen because the "/link" option is restricted to
+// consume only remaining args in its response file.
+// ARGS-NOT: warning
+// ARGS-NOT: argument unused during compilation
+// Identify the linker command
+// ARGS: link.exe

tools/driver/driver.cpp

if (Edit[0] == '^') {
     Args.push_back(Str);
   } else if (Edit[0] == 's' && Edit[1] == '/' && Edit.endswith("/") &&
              Edit.slice(2, Edit.size()-1).find('/') != StringRef::npos) {
     StringRef MatchPattern = Edit.substr(2).split('/').first;
     StringRef ReplPattern = Edit.substr(2).split('/').second;
     ReplPattern = ReplPattern.slice(0, ReplPattern.size()-1);
 
     for (unsigned i = 1, e = Args.size(); i != e; ++i) {
+      // Ignore end-of-line response file markers
+      if (Args[i] == nullptr)
+        continue;
       std::string Repl = llvm::Regex(MatchPattern).sub(ReplPattern, Args[i]);
 
       if (Repl != Args[i]) {
         OS << "### Replacing '" << Args[i] << "' with '" << Repl << "'\n";
         Args[i] = GetStableCStr(SavedStrings, Repl);
       }
     }
   } else if (Edit[0] == 'x' || Edit[0] == 'X') {
for (unsigned i = 1; i < Args.size();) {
             OS << "### Invalid X edit, end of command line!\n";
         }
       } else
         ++i;
     }
   } else if (Edit[0] == 'O') {
     for (unsigned i = 1; i < Args.size();) {
       const char *A = Args[i];
+      // Ignore end-of-line response file markers
+      if (A == nullptr)
+        continue;
       if (A[0] == '-' && A[1] == 'O' &&
           (A[2] == '\0' ||
            (A[3] == '\0' && (A[2] == 's' || A[2] == 'z' ||
                              ('0' <= A[2] && A[2] <= '9'))))) {
         OS << "### Deleting argument " << Args[i] << '\n';
         Args.erase(Args.begin() + i);
       } else
         ++i;
std::error_code EC = llvm::sys::Process::GetArgumentVector(
       argv, ArrayRef<const char *>(argv_, argc_), ArgAllocator);
   if (EC) {
     llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
     return 1;
   }
 
   std::set<std::string> SavedStrings;
   StringSetSaver Saver(SavedStrings);
-  llvm::cl::ExpandResponseFiles(Saver, llvm::cl::TokenizeGNUCommandLine, argv);
 
-  // Handle -cc1 integrated tools.
+  // Determines whether we want nullptr markers in argv to indicate response
+  // files end-of-lines. We only use this for the /LINK driver argument.
+  bool MarkEOLs = true;
   if (argv.size() > 1 && StringRef(argv[1]).startswith("-cc1"))
+    MarkEOLs = false;
+  llvm::cl::ExpandResponseFiles(Saver, llvm::cl::TokenizeGNUCommandLine, argv,
+                                MarkEOLs);
+
+  // Handle -cc1 integrated tools.
+  if (argv.size() > 1 && argv[1] != nullptr &&
+    StringRef(argv[1]).startswith("-cc1")) {
+    // Remove markers, if we added them
+    if (MarkEOLs) {
+      auto newEnd = std::remove(argv.begin(), argv.end(), nullptr);
+      argv.resize(newEnd - argv.begin());
+    }
     return ExecuteCC1Tool(argv, argv[1] + 4);
+  }
 
   bool CanonicalPrefixes = true;
   for (int i = 1, size = argv.size(); i < size; ++i) {
+    // Skip end-of-line response file markers
+    if (argv[i] == nullptr)
+      continue;
     if (StringRef(argv[i]) == "-no-canonical-prefixes") {
       CanonicalPrefixes = false;
       break;
     }
   }
 
   // Handle CCC_OVERRIDE_OPTIONS, used for editing a command line behind the
   // scenes.
if (DiagnoseCrash) {
       TheDriver.generateCompilationDiagnostics(*C, FailingCommand);
       break;
     }
   }
 
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
-  
   llvm::llvm_shutdown();
 
 #ifdef LLVM_ON_WIN32
   // Exit status should not be negative on Win32, unless abnormal termination.
   // Once abnormal termiation was caught, negative status should not be
   // propagated.
   if (Res < 0)
     Res = 1;
 #endif
 
   // If we have multiple failing commands, we return the result of the first
   // failing command.
   return Res;
 }