[llvm-ar] Accept file paths with windows format slashes

[gbreynoo]

Summary

The internal representation of llvm-ar archives uses linux style slashes for paths, no matter the OS. In the case of windows this meant file paths input intending to match existing members would only match if linux style slashes where used. This change allows either slash direction to be input by the user.

This change includes removing an unnecessary call to normalisePath and moving the call of another.

[jhenderson]

llvm/test/tools/llvm-ar/windows-path.test

1 ↗

(On Diff #213341)

Nit: missing trailing full-stop.

[MaskRay]

llvm/test/tools/llvm-ar/windows-path.test

15 ↗

(On Diff #213341)

What does a\..\a\foo.txt do?

[gbreynoo]

llvm/test/tools/llvm-ar/windows-path.test

15 ↗

(On Diff #213341)

I've added this to the test and behaviour is as expected.

[rupprecht]

I think it'd be good to also write a test that verifies the internal representation is always forward slashes on windows by running FileCheck directly on the thin archive instead of using llvm-ar t to look at it (e.g. see thin-archive.test). Or do we already have a test for that?

[gbreynoo]

In D65743#1619508, @rupprecht wrote:

I think it'd be good to also write a test that verifies the internal representation is always forward slashes on windows by running FileCheck directly on the thin archive instead of using llvm-ar t to look at it (e.g. see thin-archive.test). Or do we already have a test for that?

mri-thin-archive.test does this but it may be best to have more cases in which the internal representation is checked.

llvm/trunk/test/tools/llvm-ar/windows-path.test

+# Test that windows path seperators are handled correctly.
+REQUIRES: system-windows
+
+# Note: many of these tests depend on relative paths, so we have to cd to a
+# test directory first.
+RUN: mkdir -p %t && cd %t
+RUN: rm -rf a b && mkdir a b
+RUN: echo hello-a      > a/foo.txt
+RUN: echo hello-b      > b/foo.txt
+RUN: echo hello-parent > foo.txt
+
+# P is implied when using thin archives.
+# Create an archive.
+RUN: rm -f archive.a
+RUN: llvm-ar rcST archive.a a\..\a\foo.txt
+RUN: llvm-ar rcST archive.a foo.txt
+RUN: llvm-ar rcST archive.a b\foo.txt b/foo.txt
+RUN: llvm-ar dT archive.a foo.txt
+RUN: llvm-ar t archive.a | FileCheck %s --check-prefix=ARCHIVE --implicit-check-not {{.}}
+
+ARCHIVE:      a/foo.txt
+ARCHIVE-NEXT: b/foo.txt
+ARCHIVE-NEXT: b/foo.txt
+
+RUN: llvm-ar t archive.a | FileCheck %s --check-prefix=LIST-BOTH --implicit-check-not {{.}}
+RUN: llvm-ar t archive.a a\foo.txt | FileCheck %s --check-prefix=LIST-A --implicit-check-not {{.}}
+RUN: llvm-ar t archive.a b\foo.txt | FileCheck %s --check-prefix=LIST-B --implicit-check-not {{.}}
+
+LIST-BOTH:       a/foo.txt
+LIST-BOTH-NEXT:  b/foo.txt
+LIST-BOTH-NEXT:  b/foo.txt
+LIST-A:          a/foo.txt
+LIST-B:          b/foo.txt
+
+# Nesting a thin archive with a name conflict.
+RUN: rm -f a\nested.a b\nested.a nested.a
+RUN: llvm-ar rcST a\nested.a a\foo.txt
+RUN: llvm-ar rcST b\nested.a b\foo.txt
+RUN: llvm-ar rcST nested.a a\nested.a foo.txt b\nested.a
+RUN: llvm-ar t nested.a | FileCheck %s --check-prefix=NESTED --implicit-check-not {{.}}
+
+NESTED:      a/foo.txt
+NESTED-NEXT: foo.txt
+NESTED-NEXT: b/foo.txt

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

if (!sys::path::is_absolute(Name)) {
       StringRef ParentDir = sys::path::parent_path(ArchiveName);
       if (!ParentDir.empty())
         outs() << sys::path::convert_to_slash(ParentDir) << '/';
     }
   }
   outs() << Name << "\n";
 }
 
-static StringRef normalizePath(StringRef Path) {
-  return CompareFullPath ? Path : sys::path::filename(Path);
+static std::string normalizePath(StringRef Path) {
+  return CompareFullPath ? sys::path::convert_to_slash(Path)
+                         : sys::path::filename(Path);
 }
 
 // Implement the 'x' operation. This function extracts files back to the file
 // system.
 static void doExtract(StringRef Name, const object::Archive::Child &C) {
   // Retain the original mode.
   Expected<sys::fs::perms> ModeOrErr = C.getAccessMode();
   failIfError(ModeOrErr.takeError());
if (CountParam && ++MemberCount[Name] != CountParam)
       return IA_AddOldMember;
     return IA_Delete;
   }
 
   if (Operation == Move)
     return IA_MoveOldMember;
 
   if (Operation == ReplaceOrInsert) {
-    StringRef PosName = normalizePath(RelPos);
     if (!OnlyUpdate) {
-      if (PosName.empty())
+      if (RelPos.empty())
         return IA_AddNewMember;
       return IA_MoveNewMember;
     }
 
     // We could try to optimize this to a fstat, but it is not a common
     // operation.
     sys::fs::file_status Status;
     failIfError(sys::fs::status(*MI, Status), *MI);
     auto ModTimeOrErr = Member.getLastModified();
     failIfError(ModTimeOrErr.takeError());
     if (Status.getLastModificationTime() < ModTimeOrErr.get()) {
-      if (PosName.empty())
+      if (RelPos.empty())
         return IA_AddOldMember;
       return IA_MoveOldMember;
     }
 
-    if (PosName.empty())
+    if (RelPos.empty())
       return IA_AddNewMember;
     return IA_MoveNewMember;
   }
   llvm_unreachable("No such operation");
 }
 
 // We have to walk this twice and computing it is not trivial, so creating an
 // explicit std::vector is actually fairly efficient.
 static std::vector<NewArchiveMember>
 computeNewArchiveMembers(ArchiveOperation Operation,
                          object::Archive *OldArchive) {
   std::vector<NewArchiveMember> Ret;
   std::vector<NewArchiveMember> Moved;
   int InsertPos = -1;
-  StringRef PosName = normalizePath(RelPos);
   if (OldArchive) {
+    std::string PosName = normalizePath(RelPos);
     Error Err = Error::success();
     StringMap<int> MemberCount;
     for (auto &Child : OldArchive->children(Err)) {
       int Pos = Ret.size();
       Expected<StringRef> NameOrErr = Child.getName();
       failIfError(NameOrErr.takeError());
       StringRef Name = NameOrErr.get();
       if (Name == PosName) {
case MRICommand::Create:
       Create = true;
       if (!ArchiveName.empty())
         fail("Editing multiple archives not supported");
       if (Saved)
         fail("File already saved");
       ArchiveName = Rest;
       break;
     case MRICommand::Delete: {
-      StringRef Name = normalizePath(Rest);
+      std::string Name = normalizePath(Rest);
       llvm::erase_if(NewMembers,
                      [=](NewArchiveMember &M) { return M.MemberName == Name; });
       break;
     }
     case MRICommand::Save:
       Saved = true;
       break;
     case MRICommand::End: