Fix PR17239 by changing the semantics of the RemainingArgsClass Option kind

[rafaelauler]

Summary

This patch contains the LLVM side of the fix of PR17239 (I will submit an additional patch for the clang side), a bug that 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.

My 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.

To do this, I had to change how response file expansion works in the CommandLine[.cpp|h] component. Now, the expansion returns not only the expanded argv vector, but also a vector with pairs of numbers that marks the first and last arguments that come from response files. With this information, I am able to fix this bug.

The problem is how to encapsulate this into a clean interface. To solve this, I created the ExpandedArgs class, which manages an argv vector after response files expansion. I was reluctant to implement a new class because Clang Driver already has ArgLists and InputInfos, that is, two other layers for input argument processing. However, ExpandedArgs was necessary as a first argv wrapper to manage (and keep correct) the response file indexes while driver.cpp (in clang) fiddles with the argument vector by adding/changing elements.

[rafaelauler]

The Clang side of this patch is available at http://reviews.llvm.org/D4900

[• rafael]

This seems more than what is needed.

If all that we need is to find the end of the response file following an argument, can't we just change ExpandResponseFiles to include a null in NewArgv every time a response file ends?

That way the processing of /link would claim all arguments until the null. Regular processing in cl::ParseCommandLineOptions would just ignore the null entries. There are only 3 calls to ExpandResponseFiles. One llvm, one in clang and one in lld.

[rnk]

So, /link actually only consumes arguments until end-of-line. See http://llvm.org/PR17239. If we're going to add support for /link in response files, we should actually get it right. We haven't fixed this yet because most MSVC build systems don't use the compiler to run the link step.

[rafaelauler]

Hi Rafael,

Regarding the generality of this approach, I agree that it seems more that what is needed. However, I ran into a problem when trying to implement a simpler way. Consider the following situation (in my test case):

clang-cl /link @response.txt /align...(some linker flag)

If we implement the "null" approach here, we would insert a null after response.txt and would prematurely end which arguments "/link" get.

Regards,
Rafael Auler

[rafaelauler]

Hi Reid,

Thanks for pointing out the end-of-line issue. Looks like I overlooked this and made it expand to all arguments in response file. I will fix this patch.

Regards,
Rafael Auler

[rafaelauler]

One issue remains, though. Should we also make dash dash (--) and all options in the RemainingArgsClass only expand until the end of line? Or should we create a new kind, say, RemainingArgsEndOfLineClass, just for "/link"?

Regards,
Rafael Auler

[rnk]

In D4899#8, @rafaelauler wrote:

Hi Rafael,

Regarding the generality of this approach, I agree that it seems more that what is needed. However, I ran into a problem when trying to implement a simpler way. Consider the following situation (in my test case):

clang-cl /link @response.txt /align...(some linker flag)

If we implement the "null" approach here, we would insert a null after response.txt and would prematurely end which arguments "/link" get.

It appears we don't need to support "/link @t.rsp". MSVC doesn't support nested response files, and when cl invokes the linker, it uses a response file. It does not expand response files after /link, so you end up with a nested response file and link.exe errors out because it thinks it's an input file.

[rafaelauler]

In D4899#11, @rnk wrote:

In D4899#8, @rafaelauler wrote:

Hi Rafael,

Regarding the generality of this approach, I agree that it seems more that what is needed. However, I ran into a problem when trying to implement a simpler way. Consider the following situation (in my test case):

clang-cl /link @response.txt /align...(some linker flag)

If we implement the "null" approach here, we would insert a null after response.txt and would prematurely end which arguments "/link" get.

It appears we don't need to support "/link @t.rsp". MSVC doesn't support nested response files, and when cl invokes the linker, it uses a response file. It does not expand response files after /link, so you end up with a nested response file and link.exe errors out because it thinks it's an input file.

That's great, that allows for a much simpler patch. I'll simplify it.

[rafaelauler]

I implemented the suggestions of Rafael & Reid, using nullptrs.

Populating a string vector with nullptrs may be dangerous if the existing users of ExpandResponseFiles assumes that all strings are valid pointers. To avoid headaches, I added a flag "MarkEOLs" in ExpandResponseFiles that is false by default. ExpandResponseFiles will only populate the Argv vector with nullptrs if this flag is true. I also updated the Option library to cope with nullptrs while parsing the Argv vector.

I will also upload the clang side in D4900. Let me know if you have more suggestions!

Cheers,
Rafael Auler

[rnk]

Thanks, lgtm.

Do you need someone to commit this?

[rafaelauler]

Thanks for putting in time to review this. Yes, I don't have commit access, could you please commit it?

[rnk]

Landed in r216280.

include/llvm/Support/CommandLine.h

 //
 /// The quoting rules match those used by GCC and other tools that use
 /// libiberty's buildargv() or expandargv() utilities, and do not match bash.
 /// They differ from buildargv() on treatment of backslashes that do not escape
 /// a special character to make it possible to accept most Windows file paths.
 ///
 /// \param [in] Source The string to be split on whitespace with quotes.
 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
+/// \param [in] MarkEOLs true if tokenizing a response file and you want end of
+/// lines and end of the response file to be marked with a nullptr string.
 /// \param [out] NewArgv All parsed strings are appended to NewArgv.
 void TokenizeGNUCommandLine(StringRef Source, StringSaver &Saver,
-                            SmallVectorImpl<const char *> &NewArgv);
+                            SmallVectorImpl<const char *> &NewArgv,
+                            bool MarkEOLs = false);
 
 /// \brief Tokenizes a Windows command line which may contain quotes and escaped
 /// quotes.
 ///
 /// See MSDN docs for CommandLineToArgvW for information on the quoting rules.
 /// http://msdn.microsoft.com/en-us/library/windows/desktop/17w5ykft(v=vs.85).aspx
 ///
 /// \param [in] Source The string to be split on whitespace with quotes.
 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
+/// \param [in] MarkEOLs true if tokenizing a response file and you want end of
+/// lines and end of the response file to be marked with a nullptr string.
 /// \param [out] NewArgv All parsed strings are appended to NewArgv.
 void TokenizeWindowsCommandLine(StringRef Source, StringSaver &Saver,
-                                SmallVectorImpl<const char *> &NewArgv);
+                                SmallVectorImpl<const char *> &NewArgv,
+                                bool MarkEOLs = false);
 
 /// \brief String tokenization function type.  Should be compatible with either
 /// Windows or Unix command line tokenizers.
 typedef void (*TokenizerCallback)(StringRef Source, StringSaver &Saver,
-                                  SmallVectorImpl<const char *> &NewArgv);
+                                  SmallVectorImpl<const char *> &NewArgv,
+                                  bool MarkEOLs);
 
 /// \brief Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.  Argv should contain the command line
-/// before expansion and will be modified in place.
+/// before expansion and will be modified in place. If requested, Argv will
+/// also be populated with nullptrs indicating where each response file line
+/// ends, which is useful for the "/link" argument that needs to consume all
+/// remaining arguments only until the next end of line, when in a response
+/// file.
 ///
 /// \param [in] Saver Delegates back to the caller for saving parsed strings.
 /// \param [in] Tokenizer Tokenization strategy. Typically Unix or Windows.
 /// \param [in,out] Argv Command line into which to expand response files.
+/// \param [in] MarkEOLs Mark end of lines and the end of the response file
+/// with nullptrs in the Argv vector.
 /// \return true if all @files were expanded successfully or there were none.
 bool ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
-                         SmallVectorImpl<const char *> &Argv);
+                         SmallVectorImpl<const char *> &Argv,
+                         bool MarkEOLs = false);
 
 } // End namespace cl
 
 } // End namespace llvm
 
 #endif

lib/Option/OptTable.cpp

InputArgList *OptTable::ParseArgs(const char *const *ArgBegin,
                                   unsigned FlagsToExclude) const {
   InputArgList *Args = new InputArgList(ArgBegin, ArgEnd);
 
   // FIXME: Handle '@' args (or at least error on them).
 
   MissingArgIndex = MissingArgCount = 0;
   unsigned Index = 0, End = ArgEnd - ArgBegin;
   while (Index < End) {
+    // Ingore nullptrs, they are response file's EOL markers
+    if (Args->getArgString(Index) == nullptr) {
+      ++Index;
+      continue;
+    }
     // Ignore empty arguments (other things may still take them as arguments).
     StringRef Str = Args->getArgString(Index);
     if (Str == "") {
       ++Index;
       continue;
     }
 
     unsigned Prev = Index;

lib/Option/Option.cpp

Arg *Option::accept(const ArgList &Args,
   }
   case SeparateClass:
     // Matches iff this is an exact match.
     // FIXME: Avoid strlen.
     if (ArgSize != strlen(Args.getArgString(Index)))
       return nullptr;
 
     Index += 2;
-    if (Index > Args.getNumInputArgStrings())
+    if (Index > Args.getNumInputArgStrings() ||
+        Args.getArgString(Index - 1) == nullptr)
       return nullptr;
 
     return new Arg(UnaliasedOption, Spelling,
                    Index - 2, Args.getArgString(Index - 1));
   case MultiArgClass: {
     // Matches iff this is an exact match.
     // FIXME: Avoid strlen.
     if (ArgSize != strlen(Args.getArgString(Index)))
case JoinedOrSeparateClass: {
     // FIXME: Avoid strlen.
     if (ArgSize != strlen(Args.getArgString(Index))) {
       const char *Value = Args.getArgString(Index) + ArgSize;
       return new Arg(*this, Spelling, Index++, Value);
     }
 
     // Otherwise it must be separate.
     Index += 2;
-    if (Index > Args.getNumInputArgStrings())
+    if (Index > Args.getNumInputArgStrings() ||
+        Args.getArgString(Index - 1) == nullptr)
       return nullptr;
 
     return new Arg(UnaliasedOption, Spelling,
                    Index - 2, Args.getArgString(Index - 1));
   }
   case JoinedAndSeparateClass:
     // Always matches.
     Index += 2;
-    if (Index > Args.getNumInputArgStrings())
+    if (Index > Args.getNumInputArgStrings() ||
+        Args.getArgString(Index - 1) == nullptr)
       return nullptr;
 
     return new Arg(UnaliasedOption, Spelling, Index - 2,
                    Args.getArgString(Index - 2) + ArgSize,
                    Args.getArgString(Index - 1));
   case RemainingArgsClass: {
     // Matches iff this is an exact match.
     // FIXME: Avoid strlen.
     if (ArgSize != strlen(Args.getArgString(Index)))
       return nullptr;
     Arg *A = new Arg(UnaliasedOption, Spelling, Index++);
-    while (Index < Args.getNumInputArgStrings())
+    while (Index < Args.getNumInputArgStrings() &&
+           Args.getArgString(Index) != nullptr)
       A->getValues().push_back(Args.getArgString(Index++));
     return A;
   }
   default:
     llvm_unreachable("Invalid option kind!");
   }
 }

lib/Support/CommandLine.cpp

static bool isQuote(char C) {
   return C == '\"' || C == '\'';
 }
 
 static bool isGNUSpecial(char C) {
   return strchr("\\\"\' ", C);
 }
 
 void cl::TokenizeGNUCommandLine(StringRef Src, StringSaver &Saver,
-                                SmallVectorImpl<const char *> &NewArgv) {
+                                SmallVectorImpl<const char *> &NewArgv,
+                                bool MarkEOLs) {
   SmallString<128> Token;
   for (size_t I = 0, E = Src.size(); I != E; ++I) {
     // Consume runs of whitespace.
     if (Token.empty()) {
-      while (I != E && isWhitespace(Src[I]))
+      while (I != E && isWhitespace(Src[I])) {
+        // Mark the end of lines in response files
+        if (MarkEOLs && Src[I] == '\n')
+          NewArgv.push_back(nullptr);
         ++I;
+      }
       if (I == E) break;
     }
 
     // Backslashes can escape backslashes, spaces, and other quotes.  Otherwise
     // they are literal.  This makes it much easier to read Windows file paths.
     if (I + 1 < E && Src[I] == '\\' && isGNUSpecial(Src[I + 1])) {
       ++I;  // Skip the escape.
       Token.push_back(Src[I]);
for (size_t I = 0, E = Src.size(); I != E; ++I) {
 
     // This is a normal character.  Append it.
     Token.push_back(Src[I]);
   }
 
   // Append the last token after hitting EOF with no whitespace.
   if (!Token.empty())
     NewArgv.push_back(Saver.SaveString(Token.c_str()));
+  // Mark the end of response files
+  if (MarkEOLs)
+    NewArgv.push_back(nullptr);
 }
 
 /// Backslashes are interpreted in a rather complicated way in the Windows-style
 /// command line, because backslashes are used both to separate path and to
 /// escape double quote. This method consumes runs of backslashes as well as the
 /// following double quote if it's escaped.
 ///
 ///  * If an even number of backslashes is followed by a double quote, one
if (FollowedByDoubleQuote) {
     Token.push_back('"');
     return I;
   }
   Token.append(BackslashCount, '\\');
   return I - 1;
 }
 
 void cl::TokenizeWindowsCommandLine(StringRef Src, StringSaver &Saver,
-                                    SmallVectorImpl<const char *> &NewArgv) {
+                                    SmallVectorImpl<const char *> &NewArgv,
+                                    bool MarkEOLs) {
   SmallString<128> Token;
 
   // This is a small state machine to consume characters until it reaches the
   // end of the source string.
   enum { INIT, UNQUOTED, QUOTED } State = INIT;
   for (size_t I = 0, E = Src.size(); I != E; ++I) {
     // INIT state indicates that the current input index is at the start of
     // the string or between tokens.
     if (State == INIT) {
-      if (isWhitespace(Src[I]))
+      if (isWhitespace(Src[I])) {
+        // Mark the end of lines in response files
+        if (MarkEOLs && Src[I] == '\n')
+          NewArgv.push_back(nullptr);
         continue;
+      }
       if (Src[I] == '"') {
         State = QUOTED;
         continue;
       }
       if (Src[I] == '\\') {
         I = parseBackslash(Src, I, Token);
         State = UNQUOTED;
         continue;
       }
       Token.push_back(Src[I]);
       State = UNQUOTED;
       continue;
     }
 
     // UNQUOTED state means that it's reading a token not quoted by double
     // quotes.
     if (State == UNQUOTED) {
       // Whitespace means the end of the token.
       if (isWhitespace(Src[I])) {
         NewArgv.push_back(Saver.SaveString(Token.c_str()));
         Token.clear();
         State = INIT;
+        // Mark the end of lines in response files
+        if (MarkEOLs && Src[I] == '\n')
+          NewArgv.push_back(nullptr);
         continue;
       }
       if (Src[I] == '"') {
         State = QUOTED;
         continue;
       }
       if (Src[I] == '\\') {
         I = parseBackslash(Src, I, Token);
if (State == QUOTED) {
         continue;
       }
       Token.push_back(Src[I]);
     }
   }
   // Append the last token after hitting EOF with no whitespace.
   if (!Token.empty())
     NewArgv.push_back(Saver.SaveString(Token.c_str()));
+  // Mark the end of response files
+  if (MarkEOLs)
+    NewArgv.push_back(nullptr);
 }
 
 static bool ExpandResponseFile(const char *FName, StringSaver &Saver,
                                TokenizerCallback Tokenizer,
-                               SmallVectorImpl<const char *> &NewArgv) {
+                               SmallVectorImpl<const char *> &NewArgv,
+                               bool MarkEOLs = false) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
       MemoryBuffer::getFile(FName);
   if (!MemBufOrErr)
     return false;
   MemoryBuffer &MemBuf = *MemBufOrErr.get();
   StringRef Str(MemBuf.getBufferStart(), MemBuf.getBufferSize());
 
   // If we have a UTF-16 byte order mark, convert to UTF-8 for parsing.
   ArrayRef<char> BufRef(MemBuf.getBufferStart(), MemBuf.getBufferEnd());
   std::string UTF8Buf;
   if (hasUTF16ByteOrderMark(BufRef)) {
     if (!convertUTF16ToUTF8String(BufRef, UTF8Buf))
       return false;
     Str = StringRef(UTF8Buf);
   }
 
   // Tokenize the contents into NewArgv.
-  Tokenizer(Str, Saver, NewArgv);
+  Tokenizer(Str, Saver, NewArgv, MarkEOLs);
 
   return true;
 }
 
 /// \brief Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
-                             SmallVectorImpl<const char *> &Argv) {
+                             SmallVectorImpl<const char *> &Argv,
+                             bool MarkEOLs) {
   unsigned RspFiles = 0;
   bool AllExpanded = true;
 
   // Don't cache Argv.size() because it can change.
   for (unsigned I = 0; I != Argv.size(); ) {
     const char *Arg = Argv[I];
+    // Check if it is an EOL marker
+    if (Arg == nullptr) {
+      ++I;
+      continue;
+    }
     if (Arg[0] != '@') {
       ++I;
       continue;
     }
 
     // If we have too many response files, leave some unexpanded.  This avoids
     // crashing on self-referential response files.
     if (RspFiles++ > 20)
       return false;
 
     // Replace this response file argument with the tokenization of its
     // contents.  Nested response files are expanded in subsequent iterations.
     // FIXME: If a nested response file uses a relative path, is it relative to
     // the cwd of the process or the response file?
     SmallVector<const char *, 0> ExpandedArgv;
-    if (!ExpandResponseFile(Arg + 1, Saver, Tokenizer, ExpandedArgv)) {
+    if (!ExpandResponseFile(Arg + 1, Saver, Tokenizer, ExpandedArgv,
+                            MarkEOLs)) {
       // We couldn't read this file, so we leave it in the argument stream and
       // move on.
       AllExpanded = false;
       ++I;
       continue;
     }
     Argv.erase(Argv.begin() + I);
     Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());