[llvm-ar] Implement the P modifier.

[rupprecht]

Summary

GNU ar has a P modifier that changes filename comparisons to use full paths instead of the basename. As noted in the GNU docs, regular archives are not created with full path names, so P is used to deal with archives created by other archive programs (e.g. see the updated absolute-paths.test test case).

Since thin archives use full path names -- paths are relative to the archive -- it seems very error prone to not imply P when dealing with thin archives, so P is implied in those cases. (I think this is a deviation from GNU ar that makes sense).

This fixes PR37436 via https://github.com/ClangBuiltLinux/linux/issues/33.

[ruiu]

I played with ar and found that the following behavior is pretty odd.

Create an archive

$ mkdir x
$ echo 123 > x/foo.txt
$ ar rcS archive.a x/foo.txt

Directory name is not stored to archive.a

$ cat archive.a
!<arch>
foo.txt/ 0 0 0 644 4 `
123

But you can extract foo.txt with any directory name

$ ar x archive.a bar/foo.txt
$ cat foo.txt
123

If I understand correctly, ar by default ignores directory names in many situations like the above, and "P" flag is to make it to not ignore directory names in a pathname. It looks like the default behavior is broken. Is that important for compatibility? I wonder if there's a chance to make "P" the default behavior.

[rupprecht]

In D57927#1389723, @ruiu wrote:

I played with ar and found that the following behavior is pretty odd.

Create an archive

$ mkdir x
$ echo 123 > x/foo.txt
$ ar rcS archive.a x/foo.txt

Directory name is not stored to archive.a

$ cat archive.a
!<arch>
foo.txt/ 0 0 0 644 4 `
123

But you can extract foo.txt with any directory name

$ ar x archive.a bar/foo.txt
$ cat foo.txt
123

If I understand correctly, ar by default ignores directory names in many situations like the above, and "P" flag is to make it to not ignore directory names in a pathname. It looks like the default behavior is broken. Is that important for compatibility? I wonder if there's a chance to make "P" the default behavior.

Yes, the more I dug into pathnames, the more confusing it got... just to clarify, directory names are never stored in the non-thin archive format; the P option is only for whether full path names are used for comparisons (e.g. reading/extracting a single member from an archive, deleting a single member from an archive, etc.)

Your example is odd, but I don't think it would be invoked this way. Although making P the default would make your example make sense, the counterexample would be confusing in a more normal case:

$ mkdir x
$ echo 123 > x/foo.txt
$ ar rcSP archive.a x/foo.txt

# Directory name still not stored even though P is present
$ cat archive.a
!<arch>
foo.txt/        0           0     0     644     4         `
123

# The file we just added is there, right?
$ ar tP archive.a x/foo.txt
no entry x/foo.txt in archive

# Nope, we have to chop off the directory ourselves to find it.
$ ar tP archive.a foo.txt
foo.txt

[ruiu]

LGTM

Hmm, that's right. That's probably a good reason why we can't make P the default behavior.

ar doesn't store directory names to an archive file, but it tries to hide that fact from the user in some degree, but the mismatch leaks out in many situations like the above. Probably, ar shouldn't have tried to do that from the beginning, but we are perhaps 50+ years too late to fix it.

[thakis]

Hmm, that's right. That's probably a good reason why we can't make P the default behavior.

Doesn't that make the output dependent on the build directory path? In Chromium, we're very careful to try and make the build independent of the build directory name for deterministic builds. I think changing the default here is bad for this reason. If you have your mind set on this however, is there an opt out flag? (We don't use llvm-ar in chromium at the moment so this doesn't affect us, but it's maybe a concern you didn't think about, so I figured I'd mention it.)

[rupprecht]

In D57927#1405047, @thakis wrote:

Hmm, that's right. That's probably a good reason why we can't make P the default behavior.

Doesn't that make the output dependent on the build directory path? In Chromium, we're very careful to try and make the build independent of the build directory name for deterministic builds. I think changing the default here is bad for this reason. If you have your mind set on this however, is there an opt out flag? (We don't use llvm-ar in chromium at the moment so this doesn't affect us, but it's maybe a concern you didn't think about, so I figured I'd mention it.)

The member paths in thin archives are relative to the archive itself. So suppose you have two build trees and you want to cmp the difference of every file in each tree to make sure they're bit-for-bit identical. Won't the archive members be the same relative path to the archive within each build tree? If so, the thin archive contents will be bit-for-bit identical.

[thakis]

In D57927#1405102, @rupprecht wrote:

In D57927#1405047, @thakis wrote:

Hmm, that's right. That's probably a good reason why we can't make P the default behavior.

Doesn't that make the output dependent on the build directory path? In Chromium, we're very careful to try and make the build independent of the build directory name for deterministic builds. I think changing the default here is bad for this reason. If you have your mind set on this however, is there an opt out flag? (We don't use llvm-ar in chromium at the moment so this doesn't affect us, but it's maybe a concern you didn't think about, so I figured I'd mention it.)

The member paths in thin archives are relative to the archive itself. So suppose you have two build trees and you want to cmp the difference of every file in each tree to make sure they're bit-for-bit identical. Won't the archive members be the same relative path to the archive within each build tree? If so, the thin archive contents will be bit-for-bit identical.

Ah, "full path" doesn't mean "absolute path" here. Then it's all good, thanks!

llvm/test/tools/llvm-ar/absolute-paths.test

 MSVC's lib.exe produces archives with absolute paths to the members. It's useful
 for llvm-ar to extract them to their basename in the CWD, since usually the
 directories in the path in the archive won't exist during archive extraction.
 
 Get a temp clean cwd to extract into.
 RUN: rm -rf %t && mkdir %t && cd %t
 
 RUN: llvm-ar t %S/Inputs/absolute-paths.lib | FileCheck %s --check-prefix=CHECK-LIST
 CHECK-LIST: C:/src/llvm-project/build/dne/b.o
 CHECK-LIST: C:/src/llvm-project/build/dne/a.o
 
 Check that a.o comes out and defines foo.
-RUN: llvm-ar x %S/Inputs/absolute-paths.lib 'C:/src/llvm-project/build/dne/a.o'
+RUN: llvm-ar xP %S/Inputs/absolute-paths.lib 'C:/src/llvm-project/build/dne/a.o'
 RUN: llvm-nm a.o | FileCheck %s --check-prefix=CHECK-A
 CHECK-A: T foo
 
 Check that b.o comes out and defines bar.
-RUN: llvm-ar x %S/Inputs/absolute-paths.lib C:/src/llvm-project/build/dne/b.o
+RUN: llvm-ar xP %S/Inputs/absolute-paths.lib C:/src/llvm-project/build/dne/b.o
 RUN: llvm-nm b.o | FileCheck %s --check-prefix=CHECK-B
 CHECK-B: T bar
+
+xP above is only required because we were explicitly extracting items from an
+archive with absolute paths. Extracting all objects doesn't need P because we
+aren't explicitly requesting any individual object.
+RUN: rm -f a.o b.o
+RUN: llvm-ar x %S/Inputs/absolute-paths.lib
+RUN: llvm-nm a.o | FileCheck %s --check-prefix=CHECK-A
+RUN: llvm-nm b.o | FileCheck %s --check-prefix=CHECK-B

llvm/test/tools/llvm-ar/full-path-option.test

+# 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
+
+# Sanity test that P is accepted.
+RUN: rm -f noop.a && llvm-ar rcP noop.a foo.txt
+RUN: llvm-ar p noop.a | FileCheck %s --check-prefix=ACCEPT
+
+ACCEPT: hello-parent
+
+# Regular (non-thin) archives cannot be created with full path names as
+# members, but P can still affect how lookup works (assuming we're reading an
+# archive not created by GNU ar or llvm-ar).
+# Looking up a/foo.txt in a regular archive will fail with P because it is
+# added to the archive as foo.txt.
+RUN: rm -f display.a
+RUN: llvm-ar rcS display.a a/foo.txt foo.txt b/foo.txt
+RUN: llvm-ar t display.a a/foo.txt | FileCheck %s --check-prefix=DISPLAY-FOUND --match-full-lines
+RUN: not llvm-ar tP display.a a/foo.txt 2>&1 | FileCheck %s --check-prefix=DISPLAY-NOT-FOUND
+
+DISPLAY-FOUND: foo.txt
+DISPLAY-NOT-FOUND: 'a/foo.txt' was not found
+
+# Deleting will fail with P because the members exist as foo.txt, not a/foo.txt.
+RUN: rm -f del1.a
+RUN: llvm-ar rcS del1.a foo.txt
+RUN: llvm-ar dP del1.a a/foo.txt
+RUN: llvm-ar t del1.a a/foo.txt | FileCheck %s --check-prefix=DISPLAY-FOUND --match-full-lines
+RUN: llvm-ar d del1.a a/foo.txt
+RUN: not llvm-ar t del1.a a/foo.txt 2>&1 | FileCheck %s --check-prefix=DISPLAY-NOT-FOUND
+
+# Run several checks that P is implied when using thin archives. None of these
+# checks explicitly use P.
+
+# Creating an archive in one step.
+RUN: rm -f add.a
+RUN: llvm-ar rcST add.a a/foo.txt foo.txt b/foo.txt
+RUN: llvm-ar t add.a | FileCheck %s --check-prefix=ADD --match-full-lines
+
+ADD:      a/foo.txt
+ADD-NEXT: foo.txt
+ADD-NEXT: b/foo.txt
+
+# Create an archive incrementally.
+RUN: rm -f add-inc.a
+RUN: llvm-ar rcST add-inc.a a/foo.txt
+RUN: llvm-ar rcST add-inc.a foo.txt
+RUN: llvm-ar rcST add-inc.a b/foo.txt
+RUN: llvm-ar t add-inc.a | FileCheck %s --check-prefix=ADD-INC --match-full-lines
+
+ADD-INC:      a/foo.txt
+ADD-INC-NEXT: foo.txt
+ADD-INC-NEXT: 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 --match-full-lines
+
+NESTED:      a/foo.txt
+NESTED-NEXT: foo.txt
+NESTED-NEXT: b/foo.txt
+
+# Printing members.
+RUN: rm -f add.a
+RUN: llvm-ar rcST add.a a/foo.txt foo.txt b/foo.txt
+RUN: llvm-ar p add.a foo.txt | FileCheck %s --check-prefix=PRINT-PARENT --match-full-lines
+RUN: llvm-ar p add.a a/foo.txt | FileCheck %s --check-prefix=PRINT-A --match-full-lines
+RUN: llvm-ar p add.a b/foo.txt | FileCheck %s --check-prefix=PRINT-B --match-full-lines
+PRINT-PARENT: hello-parent
+PRINT-A:      hello-a
+PRINT-B:      hello-b
+
+# Listing members.
+RUN: rm -f add.a
+RUN: llvm-ar rcST add.a a/foo.txt foo.txt b/foo.txt
+RUN: llvm-ar t add.a foo.txt | FileCheck %s --check-prefix=LIST-PARENT --match-full-lines
+RUN: llvm-ar t add.a a/foo.txt | FileCheck %s --check-prefix=LIST-A --match-full-lines
+RUN: llvm-ar t add.a b/foo.txt | FileCheck %s --check-prefix=LIST-B --match-full-lines
+LIST-PARENT:     foo.txt
+LIST-PARENT-NOT: a/foo.txt
+LIST-PARENT-NOT: b/foo.txt
+LIST-A:          a/foo.txt
+LIST-B:          b/foo.txt
+
+# Deleting members.
+RUN: rm -f del1.a
+RUN: llvm-ar rcST del1.a a/foo.txt foo.txt b/foo.txt
+RUN: llvm-ar d del1.a foo.txt
+RUN: llvm-ar t del1.a | FileCheck %s --check-prefix=DEL-1 --match-full-lines
+
+DEL-1-NOT:  foo.txt
+DEL-1:      a/foo.txt
+DEL-1-NEXT: b/foo.txt
+
+RUN: rm -f del2.a
+RUN: llvm-ar rcST del2.a a/foo.txt foo.txt b/foo.txt
+RUN: llvm-ar d del2.a a/foo.txt
+RUN: llvm-ar t del2.a | FileCheck %s --check-prefix=DEL-2 --match-full-lines
+DEL-2-NOT:  a/foo.txt
+DEL-2:      foo.txt
+DEL-2-NEXT: b/foo.txt

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

MODIFIERS:
   [a] - put [files] after [relpos]
   [b] - put [files] before [relpos] (same as [i])
   [c] - do not warn if archive had to be created
   [D] - use zero for timestamps and uids/gids (default)
   [i] - put [files] before [relpos] (same as [b])
   [l] - ignored for compatibility
   [L] - add archive's contents
   [o] - preserve original dates
+  [P] - use full names when matching (implied for thin archives)
   [s] - create an archive index (cf. ranlib)
   [S] - do not build a symbol table
   [T] - create a thin archive
   [u] - update only [files] newer than archive contents
   [U] - use actual timestamps and uids/gids
   [v] - be verbose about actions taken
 )";
 
enum ArchiveOperation {
   QuickAppend,     ///< Quickly append to end of archive
   ReplaceOrInsert, ///< Replace or Insert members
   DisplayTable,    ///< Display the table of contents
   Extract,         ///< Extract files back to file system
   CreateSymTab     ///< Create a symbol table in an existing archive
 };
 
 // Modifiers to follow operation to vary behavior
-static bool AddAfter = false;      ///< 'a' modifier
-static bool AddBefore = false;     ///< 'b' modifier
-static bool Create = false;        ///< 'c' modifier
-static bool OriginalDates = false; ///< 'o' modifier
-static bool OnlyUpdate = false;    ///< 'u' modifier
-static bool Verbose = false;       ///< 'v' modifier
-static bool Symtab = true;         ///< 's' modifier
-static bool Deterministic = true;  ///< 'D' and 'U' modifiers
-static bool Thin = false;          ///< 'T' modifier
-static bool AddLibrary = false;    ///< 'L' modifier
+static bool AddAfter = false;        ///< 'a' modifier
+static bool AddBefore = false;       ///< 'b' modifier
+static bool Create = false;          ///< 'c' modifier
+static bool OriginalDates = false;   ///< 'o' modifier
+static bool CompareFullPath = false; ///< 'P' modifier
+static bool OnlyUpdate = false;      ///< 'u' modifier
+static bool Verbose = false;         ///< 'v' modifier
+static bool Symtab = true;           ///< 's' modifier
+static bool Deterministic = true;    ///< 'D' and 'U' modifiers
+static bool Thin = false;            ///< 'T' modifier
+static bool AddLibrary = false;      ///< 'L' modifier
 
 // Relative Positional Argument (for insert/move). This variable holds
 // the name of the archive member to which the 'a', 'b' or 'i' modifier
 // refers. Only one of 'a', 'b' or 'i' can be specified so we only need
 // one variable.
 static std::string RelPos;
 
 // This variable holds the name of the archive file as given on the
for (unsigned i = 0; i < Options.size(); ++i) {
     case 'c':
       Create = true;
       break;
     case 'l': /* accepted but unused */
       break;
     case 'o':
       OriginalDates = true;
       break;
+    case 'P':
+      CompareFullPath = true;
+      break;
     case 's':
       Symtab = true;
       MaybeJustCreateSymTab = true;
       break;
     case 'S':
       Symtab = false;
       break;
     case 'u':
for (unsigned i = 0; i < Options.size(); ++i) {
     case 'D':
       Deterministic = true;
       break;
     case 'U':
       Deterministic = false;
       break;
     case 'T':
       Thin = true;
+      // Thin archives store path names, so P should be forced.
+      CompareFullPath = true;
       break;
     case 'L':
       AddLibrary = true;
       break;
     default:
       fail(std::string("unknown option ") + Options[i]);
     }
   }
static void doDisplayTable(StringRef Name, const object::Archive::Child &C) {
   if (C.getParent()->isThin()) {
     StringRef ParentDir = sys::path::parent_path(ArchiveName);
     if (!ParentDir.empty())
       outs() << ParentDir << '/';
   }
   outs() << Name << "\n";
 }
 
+static StringRef normalizePath(StringRef Path) {
+  return CompareFullPath ? 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());
   sys::fs::perms Mode = ModeOrErr.get();
 
static void performReadOperation(ArchiveOperation Operation,
   {
     Error Err = Error::success();
     for (auto &C : OldArchive->children(Err)) {
       Expected<StringRef> NameOrErr = C.getName();
       failIfError(NameOrErr.takeError());
       StringRef Name = NameOrErr.get();
 
       if (Filter) {
-        auto I = find(Members, Name);
+        auto I = find_if(Members, [Name](StringRef Path) {
+          return Name == normalizePath(Path);
+        });
         if (I == Members.end())
           continue;
         Members.erase(I);
       }
 
       switch (Operation) {
       default:
         llvm_unreachable("Not a read operation");

 
 static InsertAction computeInsertAction(ArchiveOperation Operation,
                                         const object::Archive::Child &Member,
                                         StringRef Name,
                                         std::vector<StringRef>::iterator &Pos) {
   if (Operation == QuickAppend || Members.empty())
     return IA_AddOldMember;
 
-  auto MI = find_if(Members, [Name](StringRef Path) {
-    return Name == sys::path::filename(Path);
-  });
+  auto MI = find_if(
+      Members, [Name](StringRef Path) { return Name == normalizePath(Path); });
 
   if (MI == Members.end())
     return IA_AddOldMember;
 
   Pos = MI;
 
   if (Operation == Delete)
     return IA_Delete;
 
   if (Operation == Move)
     return IA_MoveOldMember;
 
   if (Operation == ReplaceOrInsert) {
-    StringRef PosName = sys::path::filename(RelPos);
+    StringRef PosName = normalizePath(RelPos);
     if (!OnlyUpdate) {
       if (PosName.empty())
         return IA_AddNewMember;
       return IA_MoveNewMember;
     }
 
     // We could try to optimize this to a fstat, but it is not a common
     // 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 = sys::path::filename(RelPos);
+  StringRef PosName = normalizePath(RelPos);
   if (OldArchive) {
     Error Err = Error::success();
     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) {
if (EC && EC != errc::no_such_file_or_directory)
     fail("error opening '" + ArchiveName + "': " + EC.message() + "!");
 
   if (!EC) {
     Error Err = Error::success();
     object::Archive Archive(Buf.get()->getMemBufferRef(), Err);
     EC = errorToErrorCode(std::move(Err));
     failIfError(EC,
                 "error loading '" + ArchiveName + "': " + EC.message() + "!");
+    if (Archive.isThin())
+      CompareFullPath = true;
     performOperation(Operation, &Archive, std::move(Buf.get()), NewMembers);
     return 0;
   }
 
   assert(EC == errc::no_such_file_or_directory);
 
   if (!shouldCreateArchive(Operation)) {
     failIfError(EC, Twine("error loading '") + ArchiveName + "'");
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 = sys::path::filename(Rest);
+      StringRef Name = normalizePath(Rest);
       llvm::erase_if(NewMembers,
                      [=](NewArchiveMember &M) { return M.MemberName == Name; });
       break;
     }
     case MRICommand::Save:
       Saved = true;
       break;
     case MRICommand::End: