[llvm-ar] Fix llvm-ar response file reading on Windows

[gbreynoo]

Summary

Response files where not being correctly read on Windows, this change fixes the issue and adds some tests.

[MaskRay]

llvm/test/tools/llvm-ar/response-utf8.test

9

The reliance on en_US.UTF-8 is probably ok but not ideal. Do we need XFAIL: system-darwin (http://45.33.8.238/mac/1350/step_10.txt) and what's the resolution to the reverted D68472 (@thopre)?

[thakis]

llvm/tools/llvm-ar/llvm-ar.cpp

1157

If we do this, we almost certainly want to add an --rsp-quoting flag like clang-cl and lld-link have, for cross builds targeting windows on linux.

[thopre]

llvm/test/tools/llvm-ar/response-utf8.test

9

I haven't had time to test manually on Mac OS X yet as I was on holidays. I'll try to do it in the coming days and if all looks good I'll revert the revert and remove the XFAIL for system-darwin.

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

1157

I agree that flag would be useful but that it should be in another diff. I think that this change is useful by itself and could be committed first.

[gbreynoo]

Added rsp-quoting switch as thopre requested.

[ruiu]

llvm/tools/llvm-ar/llvm-ar.cpp

268

MatchFlagWithArg function defined in ar_main does something very similar to this function, so can you factor it out and then use that function instead of writing a new one?

That doesn't only reduce the code size but also allows users use --rsp-quoting <style> in addition to --rsp-quoting=<style>.

[thopre]

llvm/test/tools/llvm-ar/response-utf8.test

9

Could you try using the same approach as in https://github.com/llvm/llvm-project/blob/c3eded068c64bfe614d25359927a2917ff8e4a35/llvm/test/tools/llvm-ar/mri-nonascii.test#L19 ?

[gbreynoo]

• Pulled out the MatchFlagWithArg lambda expression into static function, and done the same with some code that handles hyphen removal.
• Added test cases to response.test.
• Removed the python call in response-utf8.test. I would prefer the file comparison but it should be sufficient to show the archive is functioning properly.

[MaskRay]

llvm/test/tools/llvm-ar/response-utf8.test

9

@thopre Hi, have you tried fixing the macOS (or Windows) problem?

llvm/tools/llvm-ar/llvm-ar.cpp

1102

Prefer using in new code. It can be used in an alias template and thus improve consistency with other constructs of the language.

1105

I feel that we can get rid of some if (Arg.empty()) checks if matchFlagWithArg itself recognizes one or two dash, then removeHyphens may probably be unnecessary.

1129

Argv is passed as a non-const reference but it is not actually mutated in the function as I expected. Should getRspQuoting remove --rsp-quoting so that on L1204 we don't need if (matchFlagWithArg("rsp-quoting", Arg, ArgIt, Argv.end())... ?

[gbreynoo]

• Use using instead of typedef
• removeHyphens is now called from matchFlagWithArg
• Removed Argv reference argument from getRspQuoting
• Fix response.test that failed on linux

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

1129

Good catch, it was left over from a previous implementation. I think with the current implementation it may be best not to modify Argv. The call to matchFlagWithArg is cleaner than removing rsp-quoting arguments that can be in one vector element or in two elements. If there is an implementation you had in mind I'd be happy to make changes.

[MaskRay]

Context not available.

https://llvm.org/docs/GettingStarted.html#sending-patches

apt install arcanist (clone libphutil + clone arcanist)
cd llvm; arc install-certificate
arc diff 'HEAD^'  # Make sure the commit message includes `Differential Revision: `. Update the revision with a full context

llvm/test/tools/llvm-ar/response.test

25

If \ has to be used, it is recommended to indent the continuation lines to make them stand out.

llvm/tools/llvm-ar/llvm-ar.cpp

182

Why KeyLetters? I think the ar terminology uses operations and modifiers. Collectively they can be called options.

1123

Redundant pair of braces

1181

space after ,

[thopre]

llvm/test/tools/llvm-ar/response-utf8.test

9

My bad, forgot about this message. I'm not sure what problem you are referring to. The file I linked to in my previous message passes on all three of Linux, Mac OS X and Microsoft Windows.

[MaskRay]

llvm/test/tools/llvm-ar/response-utf8.test

9

Sorry, I didn't notice that llvm/test/tools/llvm-ar/mri-nonascii.test has been re-committed.

[gbreynoo]

Fixed:

• Redundant pair of braces
• space after ,
• indent the continuation lines

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

182

I wanted to make the use of the variable clearer as the term Options differs in meaning:

• In the help text the term Options is used for flags (non-single letter) that are not Operations or Modifiers.
• In the documentation Options is for Operations , Modifiers and Other (non-single letter)
• In the code the Options string only holds Operations and Modifiers, i.e. single letter arguments. Longer flags (not-single letter) are handled by other methods.

I hoped that changing the variable name would lessen confusion but can change it back if required.

[gbreynoo]

ping

[ruiu]

llvm/tools/llvm-ar/llvm-ar.cpp

87

clang nor lld support --rsp-quoting=default. If you pass --rsp-quoting=default to clang, it looks like it is accepted, but the truth is that clang ignores an --rsp-quoting if it is passed with an unknown argument (you can try --rsp-quoting=foo). So, for consistency, maybe you shouldn't add that one to llvm-ar?

401

Could you revert this naming change? It makes code diff a bit hard to follow.

1102

I don't think this using improves code readability.

1140

It seems that this code can be improved in terms of readability, like this:

for (StringRef Arg : Argv) {
  if (Arg == "-rsp-quoting=posix" || Arg == "--rsp-quoting=posix")
    Ret = cl::TokenizeGNUCommandLine;
  else if (Arg == "-rsp-quoting=windows" || Arg == "--rsp-quoting=windows")
    Ret = cl::TokenizeWindowsCommandLine;
}

This is I believe basically what we do in the clang driver.

[gbreynoo]

Fixed:

• Removed --rsp-quoting=default
• Reverted rename of Options
• Removed use of using

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

1140

llvm-ar handles all the other long arguments with MatchFlagWithArg and the majority of this patch is so rsp-quoting can share this behaviour. Would you prefer to use the code above? It would reduce the size of the patch but would mean behaviour inconsistencies in how long arguments are handled.

[gbreynoo]

ping

[MaskRay]

UTF-8 tests on Windows used to cause llvm-ar trouble. I don't know enough of Windows (nor do I have a Windows machine...) Hope other people can review and confirm the response-utf8.test test will not cause trouble.

[MaskRay]

llvm/test/tools/llvm-ar/response.test

28

Prefer single quotes. It does less translation. Shell quoting is difficult. With single quotes, a reader can think less what happens here.

[MaskRay]

llvm/test/tools/llvm-ar/response.test

28

If you do want a form feed, you need echo -e.

You are currently using a bash/zsh extension.

[gbreynoo]

• Use single quotes in tests
• Clarify use of echo in response.test

[gbreynoo]

llvm/test/tools/llvm-ar/response.test

28

I wasn't looking for a form feed but have used echo -e to clarify what is output

[gbreynoo]

The use of python in the test response.test was failing on my Windows machine with Python 3. This patch fixes the issue.

[gbreynoo]

ping

[gbreynoo]

ping

[jhenderson]

In D69665#1814619, @MaskRay wrote:

UTF-8 tests on Windows used to cause llvm-ar trouble. I don't know enough of Windows (nor do I have a Windows machine...) Hope other people can review and confirm the response-utf8.test test will not cause trouble.

At @gbreynoo's request, I ran this test locally, and it passed fine. It's worth noting that I sit opposite him, so it's possible that my Windows machine has some unusual but identical-to-his setup that causes it to work. I also inspected the two response files used by the utf8 test, and they have been encoded in UTF-8 (the bytes the '£' character are 0xc2a3 as expected). Meanwhile, the file "response-utf8.test.tmp-£.a" appears to be named as such when viewed in the UI or via PowerShell's or ls's directory listing. The only issue I found was when lit printed the commands, the output got corrupted slightly by the '£' character, but that's not really an issue with the test, I think. I think the response-utf8.test looks good to me.

I haven't looked at the rest of the code. @MaskRay, if there is anything else you'd like me to take a look at, please let me know.

[MaskRay]

The tests are good, and I don't have more questions about Windows. But please mark all addresses issues "done". Click the "Done" button before uploading a Differential. When the Differential is uploaded, all marked comments will be marked "Done".

llvm/tools/llvm-ar/llvm-ar.cpp

1116

removeHyphens is only used once. Consider inlining it.

I feel a bit strange about removeHyphens. Does it accept an option without a dash?

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

1116

I have inlined the function..

[gbreynoo]

Inlined removeHyphens function

[gbreynoo]

Thanks Maskray for your comment regarding marking comments as "done", I'll look out for that in future.

llvm/tools/llvm-ar/llvm-ar.cpp

1116

Regarding the your question, the removeHyphens function would accept options without a dash and return the same value.

[gbreynoo]

ping

[MaskRay]

llvm/test/tools/llvm-ar/response.test

24

There is no positive --rsp-quoting= test, so it is hard to tell whether the feature works.

llvm/tools/llvm-ar/llvm-ar.cpp

1102

Unused

1121

Three-dash ---rsp-quoting will be accidentally accepted.

1137

SmallVector -> ArrayRef

1165

I think we may need some refactoring first. Let me check if the parsing logic can be simplified.

[gbreynoo]

• Removed unused function removeHyphens
• Fixed matchFlagWithArg so 3 hyphens are no longer excepted before an argument
• getRspQuoting now uses ArraryRef instead of SmallVector

[gbreynoo]

llvm/tools/llvm-ar/llvm-ar.cpp

1165

By moving to iterators my hope was this area would be easier to read, however it is still not ideal. Any suggestions for further refactoring would be appreciated.

[gbreynoo]

ping

[sbc100]

It looks like there is some inconsistency about what default to use.

clang seems to default to GNU style, even when running on windows: https://github.com/llvm/llvm-project/blob/master/clang/tools/driver/driver.cpp#L386
This can be overridden by --driver-mode=cl or --rsp-quoting=windows, but the default with no arguments seems to be GNU.

The wasm version of lld (which I work on) also defaults to GNU style (https://github.com/llvm/llvm-project/blob/9989b859efccafacb0cc1f8d393d8b9fc49f4037/lld/wasm/Driver.cpp#L168) although the other lld flavors have logic that matches that in this change.

Do wasm-ld and clang need updating too?

[ruiu]

lld's default choices are different among platforms as we are trying to match the native linkers' behaviors. So is true to clang; if you invoke clang as clang-cl (which is a Microsoft-compatible compiler driver), the value is set to Windows by default. I think we didn't have any other policy than this, as we are creating drop-in replacements.

As to wasm-ld, I don't think you need to care about compatibility, as this is the first (and perhaps the only?) linker for wasm. So I would stick with GNU-style quoting instead of make it vary depending on platform to keep it simple.

[gbreynoo]

Hi sbc100, thanks for highlighting this inconsistency. I think ideally for the sake of consistency, wasm-ld and clang would be updated too. I wonder if it would be worth the effort to have some kind of shared implementation for this?

Regarding Rui's point, I think there is a balance with giving Windows users the command line behaviour they expect. Is it least surprising to them that there is difference in default behaviour between platforms or that the default behaviour for Windows is not Windows style?

[sbc100]

This issue we in emscripten is really consistency. We have a compiler driver that invokes clang, llvm-ar, llvm-nm, and wasm-ld. We have logic that uses response files for long command lines. I think it's reasonable to expect that all the tools should have the same default, no? Otherwise each subprocess callsite needs to encode the rsp style .. per tool and per platform.

[ruiu]

Yeah, that's another type of consistency, and if you feel that that consistency is more important, feel free to make a change to wasm-ld so that the tool defaults to runtime OS's rsp-quoting style.

llvm/test/tools/llvm-ar/response-utf8.test

+## Check that response files can cope with non-ascii characters.
+
+# RUN: echo 'contents' > %t-£.txt
+
+# RUN: rm -f %t-£.a
+# RUN: echo 'r %t-£.a %t-£.txt' > %t-replace.txt
+# RUN: llvm-ar @%t-replace.txt
+
+# RUN: echo 'p %t-£.a %t-£.txt' > %t-print.txt
+# RUN: llvm-ar @%t-print.txt | FileCheck %s
+# CHECK: contents

llvm/test/tools/llvm-ar/response.test

+## llvm-ar should be able to consume response files.
+
+# RUN: echo 'contents' > %t.txt
+# RUN: echo 'rc %t1.a %t.txt' > %t.response1.txt
+# RUN: llvm-ar @%t.response1.txt
+# RUN: llvm-ar p %t1.a | FileCheck %s --check-prefix=CONTENTS
+
+## Quotes and Spaces.
+# RUN: echo 'contents' > '%t space.txt'
+## Python is used here to ensure the quotes are written to the response file
+# RUN: %python -c "import os; open(r'%t.response2.txt', 'w').write(r'%t2.a \"%t space.txt\"'+ '\n')"
+# RUN: llvm-ar rc @%t.response2.txt
+# RUN: llvm-ar p %t2.a | FileCheck %s --check-prefix=CONTENTS
+
+## Arguments after the response file.
+# RUN: echo 'rc %t3.a' > %t.response3.txt
+# RUN: llvm-ar @%t.response3.txt %t.txt
+# RUN: llvm-ar p %t3.a | FileCheck %s --check-prefix=CONTENTS
+
+# CONTENTS: contents
+
+## rsp-quoting
+# RUN: not llvm-ar --rsp-quoting=foobar @%t.response1.txt 2>&1 | \
+# RUN:   FileCheck  %s --check-prefix=ERROR
+# ERROR: Invalid response file quoting style foobar
+
+# RUN: echo -e 'rc %/t.a blah\\foo' > %t-rsp.txt
+# RUN: not llvm-ar --rsp-quoting=windows @%t-rsp.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=WIN
+# WIN: error: blah\foo: {{[Nn]}}o such file or directory
+
+# RUN: not llvm-ar -rsp-quoting posix @%t-rsp.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=POSIX
+# POSIX: error: blahfoo: {{[Nn]}}o such file or directory

llvm/tools/llvm-ar/llvm-ar.cpp

 OPTIONS:
   --format              - archive format to create
     =default            -   default
     =gnu                -   gnu
     =darwin             -   darwin
     =bsd                -   bsd
   --plugin=<string>     - ignored for compatibility
   -h --help             - display this help and exit
+  --rsp-quoting         - quoting style for response files
+    =posix              -   posix
+    =windows            -   windows
   --version             - print the version and exit
   @<file>               - read options from <file>
 
 OPERATIONS:
   d - delete [files] from the archive
   m - move [files] in the archive
   p - print [files] found in the archive
   q - quick append [files] to the archive

 static SmallVector<const char *, 256> PositionalArgs;
 
 static bool MRI;
 
 namespace {
 enum Format { Default, GNU, BSD, DARWIN, Unknown };
 }
 
 static Format FormatType = Default;
 
 static std::string Options;
 
 // This enumeration delineates the kinds of operations on an archive
 // that are permitted.
 enum ArchiveOperation {
   Print,           ///< Print the contents of the archive
   Delete,          ///< Delete the specified members

 // Get the archive file name from the command line
 static void getArchive() {
   if (PositionalArgs.empty())
     badUsage("an archive name must be specified");
   ArchiveName = PositionalArgs[0];
   PositionalArgs.erase(PositionalArgs.begin());
 }
 
 static object::Archive &readLibrary(const Twine &Library) {
   auto BufOrErr = MemoryBuffer::getFile(Library, -1, false);
   failIfError(BufOrErr.getError(), "could not open library " + Library);
   ArchiveBuffers.push_back(std::move(*BufOrErr));
   auto LibOrErr =
       object::Archive::create(ArchiveBuffers.back()->getMemBufferRef());
   failIfError(errorToErrorCode(LibOrErr.takeError()),
               "could not parse library");
   Archives.push_back(std::move(*LibOrErr));
case 'L':
       break;
     case 'V':
       cl::PrintVersionMessage();
       exit(0);
     case 'h':
       printHelpMessage();
       exit(0);
     default:
       badUsage(std::string("unknown option ") + Options[i]);
     }
   }
 
   // At this point, the next thing on the command line must be
   // the archive name.
   getArchive();
 
   // Everything on the command line at this point is a member.
static bool handleGenericOption(StringRef arg) {
   }
   if (arg == "-version" || arg == "--version") {
     cl::PrintVersionMessage();
     return true;
   }
   return false;
 }
 
-static int ar_main(int argc, char **argv) {
-  SmallVector<const char *, 0> Argv(argv, argv + argc);
-  StringSaver Saver(Alloc);
-  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv);
-  for (size_t i = 1; i < Argv.size(); ++i) {
-    StringRef Arg = Argv[i];
-    const char *match = nullptr;
-    auto MatchFlagWithArg = [&](const char *expected) {
-      size_t len = strlen(expected);
-      if (Arg == expected) {
-        if (++i >= Argv.size())
-          fail(std::string(expected) + " requires an argument");
-        match = Argv[i];
-        return true;
-      }
-      if (Arg.startswith(expected) && Arg.size() > len && Arg[len] == '=') {
-        match = Arg.data() + len + 1;
-        return true;
-      }
-      return false;
-    };
-    if (handleGenericOption(Argv[i]))
+static StringRef removeHyphens(StringRef Arg) {
+  if (Arg.startswith("--"))
+    return Arg.substr(2);
+
+  if (Arg.startswith("-"))
+    return Arg.substr(1);
+
+  return Arg;
+
+static const char *
+matchFlagWithArg(StringRef Expected,
+                 SmallVector<const char *, 0>::iterator &ArgIt,
+                 SmallVector<const char *, 0> Args) {
+  StringRef Arg = removeHyphens(*ArgIt);
+  size_t len = Expected.size();
+  if (Arg == Expected) {
+    if (++ArgIt == Args.end())
+      fail(std::string(Expected) + " requires an argument");
+
+    return *ArgIt;
+  }
+  if (Arg.startswith(Expected) && Arg.size() > len && Arg[len] == '=')
+    return Arg.data() + len + 1;
+
+  return nullptr;
+}
+
+static cl::TokenizerCallback getRspQuoting(SmallVector<const char *, 0> Argv) {
+  cl::TokenizerCallback Ret =
+      Triple(sys::getProcessTriple()).getOS() == Triple::Win32
+          ? cl::TokenizeWindowsCommandLine
+          : cl::TokenizeGNUCommandLine;
+
+  for (SmallVector<const char *, 0>::iterator ArgIt = Argv.begin();
+       ArgIt != Argv.end(); ++ArgIt) {
+    if (const char *Match = matchFlagWithArg("rsp-quoting", ArgIt, Argv)) {
+      StringRef MatchRef = Match;
+      if (MatchRef == "posix")
+        Ret = cl::TokenizeGNUCommandLine;
+      else if (MatchRef == "windows")
+        Ret = cl::TokenizeWindowsCommandLine;
+      else
+        fail(std::string("Invalid response file quoting style ") + Match);
+  }
+
+  return Ret;
+}
+
+static int ar_main(int argc, char **argv) {
+  SmallVector<const char *, 0> Argv(argv + 1, argv + argc);
+  StringSaver Saver(Alloc);
+
+  cl::ExpandResponseFiles(Saver, getRspQuoting(Argv), Argv);
+
+  for (SmallVector<const char *, 0>::iterator ArgIt = Argv.begin();
+       ArgIt != Argv.end(); ++ArgIt) {
+    const char *Match = nullptr;
+
+    if (handleGenericOption(*ArgIt))
       return 0;
-    if (Arg == "--") {
-      for (; i < Argv.size(); ++i)
-        PositionalArgs.push_back(Argv[i]);
+    if (strcmp(*ArgIt, "--") == 0) {
+      ++ArgIt;
+      for (; ArgIt != Argv.end(); ++ArgIt)
+        PositionalArgs.push_back(*ArgIt);
       break;
     }
-    if (Arg[0] == '-') {
-      if (Arg.startswith("--"))
-        Arg = Argv[i] + 2;
+
+    if (*ArgIt[0] != '-') {
+      if (Options.empty())
+        Options += *ArgIt;
       else
-        Arg = Argv[i] + 1;
-      if (Arg == "M") {
-        MRI = true;
-      } else if (MatchFlagWithArg("format")) {
-        FormatType = StringSwitch<Format>(match)
-                         .Case("default", Default)
-                         .Case("gnu", GNU)
-                         .Case("darwin", DARWIN)
-                         .Case("bsd", BSD)
-                         .Default(Unknown);
-        if (FormatType == Unknown)
-          fail(std::string("Invalid format ") + match);
-      } else if (MatchFlagWithArg("plugin")) {
-        // Ignored.
-      } else {
-        Options += Argv[i] + 1;
-      }
-    } else if (Options.empty()) {
-      Options += Argv[i];
-    } else {
-      PositionalArgs.push_back(Argv[i]);
+        PositionalArgs.push_back(*ArgIt);
+      continue;
+    }
+
+    if (strcmp(*ArgIt, "-M") == 0) {
+      continue;
+    }
+
+    Match = matchFlagWithArg("format", ArgIt, Argv);
+    if (Match) {
+      FormatType = StringSwitch<Format>(Match)
+        fail(std::string("Invalid format ") + Match);
+      continue;
     }
+
+    if (matchFlagWithArg("plugin", ArgIt, Argv) ||
+        matchFlagWithArg("rsp-quoting", ArgIt, Argv))
+      continue;
+
+    Options += *ArgIt + 1;
   }
+
   ArchiveOperation Operation = parseCommandLine();
   return performOperation(Operation, nullptr);
 }
 
 static int ranlib_main(int argc, char **argv) {
   bool ArchiveSpecified = false;
   for (int i = 1; i < argc; ++i) {
     StringRef arg(argv[i]);