Fix relative thin archive path handling

[gbreynoo]

Summary

This fixes some thin archive relative path issues, paths are shortened where possible and paths are output correctly when using the display table command.

[ruiu]

include/llvm/Object/ArchiveWriter.h

39–40 ↗

(On Diff #191083)

This function is supposed to compute an archive relative path (as the name says), and it now has ForceRelative flag? That's confusing.

[rupprecht]

test/tools/llvm-ar/thin-archive.test

7 ↗

(On Diff #191083)

I'm not able to reproduce any differences against GNU ar based on these test cases.

Can you clarify what it is that you're fixing?

[gbreynoo]

Update after Rui's comment.

The thought was originally to avoid changes to addMember but this may be clearer.

[gbreynoo]

In response to rupprecht:

Currently on Windows absolute paths in an archive can have mixed slashes e.g.
C:\llvm-build\test\tools\llvm-ar\Output\thin-archive.test.tmp/foo/elf.o/
And to select the member the path must be matched exactly.

llvm-ar -t 'absolute archive path' outputs the above as:
C:\llvm-build\test\tools\llvm-ar\Output\thin-archive.test.tmp/C:\llvm-build\test\tools\llvm-ar\Output\thin-archive.test.tmp/foo/elf.o

Relative member paths will be converted to absolute paths in some cases.

Relative paths currently are not computed correctly in some cases, for example:
when in directory foo which contains elf.o
llvm-ar.exe rTc ../relative-1.ar elf.o
The member path becomes ../elf.o rather than foo/elf.o

These relative path issues are also found when flattening thin-archives e.g.

The following files are present:

• reduce-thin-path.test.tmp/elf.o
• reduce-thin-path.test.tmp/baz/internal.a

internal.a has a member ../elf.o/

When in directory reduce-thin-path.test.tmp/foo call below:
llvm-ar.exe" "rTc" "C:\llvm-build\test\tools\llvm-ar\Output\reduce-thin-path.test.tmp/foo/bar/external.ar" "../baz/internal.ar"

The member is squashed to the following:
../baz\../elf.o/
However it should be ../../elf.o

After some experimenting in gnu-ar there may be similar issues regarding relative thin archive paths but I don't believe we should follow suit if we can help it.

[ruiu]

Let P1 and P2 be pathnames. IIUC, the problem we want to solve is to how to compute a relative path from the last directory of P1 to P2. Is this correct?

If so, I guess the following scheme may be simpler than the current one:

1. Canonicalize both P1 and P2 by converting backslashes to slashes.
2. Remove common prefixes from P1 and P2
3. Count the number of slashes in P1. Let N be that number.
4. Concatenate "../../../../...." (repeated N times) and P2. This is the result.

Here is an example when P1 = "foo/bar/baz/quux" and P2="foo/x/y".

1. Canonicalize them (in this case this is a no-op)
2. P1 becomes "bar/baz/quux", P2 becomes "x/y".
3. N = 2
4. "../../x/y"

lib/Object/ArchiveWriter.cpp

499–500 ↗

(On Diff #191488)

This lambda doesn't capture anything, you probably should consider moving this outside of this function.

502 ↗

(On Diff #191488)

Flip the condition and return early. Err would be a better name, as it can be read as "if error". Currently it is "if ret" which is a bit less readable.

514–515 ↗

(On Diff #191488)

Create instances of SmallString and populate their contents as a side-effect seems a bit awkward. You could instead make your helper function (which is currently named getDotlessAbsolutePath) to return an std::string.

[gbreynoo]

The problem with working out the paths is if either are in the form foo/../bar/. I don't believe your above suggestion would work for paths with internal ../.

lib/Object/ArchiveWriter.cpp

499–500 ↗

(On Diff #191488)

The lambda was to keep it local to this function, i'd rather keep it unless others really disagree.

514–515 ↗

(On Diff #191488)

Made a change as you've suggested but the use of SmallString is due to remove__dots() and I didn't want a second conversion to std::string.

[gbreynoo]

ping

If the reasons for these changes are unclear I'll be happy to clarify.

Thanks

[rupprecht]

Sorry, this patch dropped off my radar. I'm not sure I'll get to it this or next week, but I'll make sure to review it eventually (unless ruiu does it sooner).

[ruiu]

Sorry, I though that I replied to this thread.

In D59491#1436706, @gbreynoo wrote:

The problem with working out the paths is if either are in the form foo/../bar/. I don't believe your above suggestion would work for paths with internal ../.

You can just call remove_dots to remove all internal .., no?

[gbreynoo]

Hi Rui, thanks for the feedback. I'm unsure what changes would better fit what we have outlined in past comments:

Suggested implementation:

Canonicalize both P1 and P2 by converting backslashes to slashes(including calls to remove_dots).
Remove common prefixes from P1 and P2
Count the number of slashes in P1. Let N be that number.
Concatenate "../../../../...." (repeated N times) and P2. This is the result.

My understanding of the current implementation:

Calls to getDotlessAbsolute path to canonicalize, includes call to remove_dots()
In cases where this isnt possible return P2
Remove common prefixes using two iterators
Construct a path with the number of "../" required using one iterator, I thought this would be more efficient than constructing the remaining P1 path to count slashes
Append the remaining P2 path using the iterator.

Any further suggestions would be appreciated,

Thanks

[gbreynoo]

I've attempted to make "computeArchiveRelativePath" clearer and errors are now propagated to the caller to deal with rather than just returning the "To" argument.

[ruiu]

LGTM

[hintonda]

This change seems to have caused the following error:

Writer.cpp.o.d -o lib/Object/CMakeFiles/LLVMObject.dir/ArchiveWriter.cpp.o -c /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/lib/Object/ArchiveWriter.cpp
In file included from /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/lib/Object/ArchiveWriter.cpp:13:
In file included from /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/include/llvm/Object/ArchiveWriter.h:16:
In file included from /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/include/llvm/ADT/StringRef.h:12:
In file included from /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/include/llvm/ADT/STLExtras.h:20:
In file included from /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/include/llvm/ADT/SmallVector.h:19:
In file included from /Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/include/llvm/Support/MathExtras.h:18:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/algorithm:1278:14: error: type 'llvm::sys::path::const_iterator' does not provide a call operator

if (!__pred(*__first1, *__first2))
     ^~~~~~

/Users/dhinton/projects/llvm_project/monorepo/llvm-project/llvm/lib/Object/ArchiveWriter.cpp:523:12: note: in instantiation of function template specialization 'std::__1::mismatch<llvm::sys::path::const_itera
tor, llvm::sys::path::const_iterator, llvm::sys::path::const_iterator>' requested here

std::mismatch(sys::path::begin(DirFrom), sys::path::end(DirFrom),
     ^

1 error generated.

[gribozavr]

Sorry, I reverted this change in r362413, it was breaking the build.

[gbreynoo]

Thanks hintonda and gribozavr, I will look into that build issue before I recommit.

[gbreynoo]

Fixed a reapplied:
rG5d5078e341f5: [llvm-ar] Reapply Fix relative thin archive path handling

llvm/trunk/include/llvm/Object/ArchiveWriter.h

struct NewArchiveMember {
 
   static Expected<NewArchiveMember>
   getOldMember(const object::Archive::Child &OldMember, bool Deterministic);
 
   static Expected<NewArchiveMember> getFile(StringRef FileName,
                                             bool Deterministic);
 };
 
-std::string computeArchiveRelativePath(StringRef From, StringRef To);
+Expected<std::string> computeArchiveRelativePath(StringRef From, StringRef To);
 
 Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,
                    bool Deterministic, bool Thin,
                    std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
 }
 
 #endif

llvm/trunk/lib/Object/ArchiveWriter.cpp

computeMemberData(raw_ostream &StringTable, raw_ostream &SymNames,
   // tools, older versions of which expect a symbol table in a non-empty
   // archive, regardless of whether there are any symbols in it.
   if (HasObject && SymNames.tell() == 0)
     SymNames << '\0' << '\0' << '\0';
   return Ret;
 }
 
 namespace llvm {
-// Compute the relative path from From to To.
-std::string computeArchiveRelativePath(StringRef From, StringRef To) {
-  if (sys::path::is_absolute(From) || sys::path::is_absolute(To))
-    return To;
-
-  StringRef DirFrom = sys::path::parent_path(From);
-  auto FromI = sys::path::begin(DirFrom);
-  auto ToI = sys::path::begin(To);
-  while (*FromI == *ToI) {
-    ++FromI;
-    ++ToI;
-  }
+
+static ErrorOr<SmallString<128>> canonicalizePath(StringRef P) {
+  SmallString<128> Ret = P;
+  std::error_code Err = sys::fs::make_absolute(Ret);
+  if (Err)
+    return Err;
+  sys::path::remove_dots(Ret, /*removedotdot*/ true);
+  return Ret;
 
+// Compute the relative path from From to To.
+Expected<std::string> computeArchiveRelativePath(StringRef From, StringRef To) {
+  ErrorOr<SmallString<128>> PathToOrErr = canonicalizePath(To);
+  ErrorOr<SmallString<128>> DirFromOrErr = canonicalizePath(From);
+  if (!PathToOrErr || !DirFromOrErr)
+    return errorCodeToError(std::error_code(errno, std::generic_category()));
+
+  const SmallString<128> &PathTo = *PathToOrErr;
+  const SmallString<128> &DirFrom = sys::path::parent_path(*DirFromOrErr);
+
+  // Can't construct a relative path between different roots
+  if (sys::path::root_name(PathTo) != sys::path::root_name(DirFrom))
+    return sys::path::convert_to_slash(PathTo);
+
+  // Skip common prefixes
+  auto FromTo =
+      std::mismatch(sys::path::begin(DirFrom), sys::path::end(DirFrom),
+                    sys::path::begin(PathTo), sys::path::end(PathTo));
+  auto FromI = FromTo.first;
+  auto ToI = FromTo.second;
+
+  // Construct relative path
   SmallString<128> Relative;
   for (auto FromE = sys::path::end(DirFrom); FromI != FromE; ++FromI)
-    sys::path::append(Relative, "..");
+    sys::path::append(Relative, sys::path::Style::posix, "..");
 
-  for (auto ToE = sys::path::end(To); ToI != ToE; ++ToI)
-    sys::path::append(Relative, *ToI);
+  for (auto ToE = sys::path::end(PathTo); ToI != ToE; ++ToI)
+    sys::path::append(Relative, sys::path::Style::posix, *ToI);
 
-  // Replace backslashes with slashes so that the path is portable between *nix
-  // and Windows.
-  return sys::path::convert_to_slash(Relative);
+  return Relative.str();
 }
 
 Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,
                    bool Deterministic, bool Thin,
                    std::unique_ptr<MemoryBuffer> OldArchiveBuf) {
   assert((!Thin || !isBSDLike(Kind)) && "Only the gnu format has a thin mode");
 

llvm/trunk/lib/ToolDrivers/llvm-lib/LibDriver.cpp

for (auto *Arg : Args.filtered(OPT_INPUT)) {
     Members.emplace_back(std::move(*MOrErr));
   }
 
   // Create an archive file.
   std::string OutputPath = getOutputPath(&Args, Members[0]);
   // llvm-lib uses relative paths for both regular and thin archives, unlike
   // standard GNU ar, which only uses relative paths for thin archives and
   // basenames for regular archives.
-  for (NewArchiveMember &Member : Members)
-    Member.MemberName =
-        Saver.save(computeArchiveRelativePath(OutputPath, Member.MemberName));
+  for (NewArchiveMember &Member : Members) {
+    if (sys::path::is_relative(Member.MemberName)) {
+      Expected<std::string> PathOrErr =
+          computeArchiveRelativePath(OutputPath, Member.MemberName);
+      if (PathOrErr)
+        Member.MemberName = Saver.save(*PathOrErr);
+    }
+  }
 
   if (Error E =
           writeArchive(OutputPath, Members,
                        /*WriteSymtab=*/true, object::Archive::K_GNU,
                        /*Deterministic*/ true, Args.hasArg(OPT_llvmlibthin))) {
     handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
       llvm::errs() << OutputPath << ": " << EI.message() << "\n";
     });
     return 1;
   }
 
   return 0;
 }

llvm/trunk/test/tools/llvm-ar/reduce-thin-path.test

+RUN: rm -rf %t && mkdir -p %t/foo/bar/
+RUN: mkdir -p %t/baz/
+RUN: yaml2obj %S/Inputs/elf.yaml -o %t/elf.o
+
+RUN: cd %t && llvm-ar rTc %t/baz/internal.ar elf.o
+RUN: cd %t/foo && llvm-ar rTc %t/foo/bar/external.ar ../baz/internal.ar
+
+RUN: FileCheck -input-file=%t/foo/bar/external.ar %s
+
+CHECK: {{^}}../../elf.o/

llvm/trunk/test/tools/llvm-ar/thin-archive.test

+RUN: rm -rf %t && mkdir -p %t/foo/bar/
+
+RUN: yaml2obj %S/Inputs/elf.yaml -o %t/foo/elf.o
+RUN: cp %t/foo/elf.o %t/foo/bar/elf.o
+RUN: cp %t/foo/bar/elf.o %t/delete.o
+
+Test that modules can be added with absolute paths when the archive is created using an absolute path
+
+RUN: llvm-ar rTc %t/absolute-1.ar %t/foo/elf.o %t/delete.o %t/foo/bar/elf.o
+RUN: llvm-ar dT %t/absolute-1.ar delete.o
+
+RUN: FileCheck -input-file=%t/absolute-1.ar --check-prefixes=THIN,CHECK %s -DPATH=%/t/
+RUN: llvm-ar t %t/absolute-1.ar | FileCheck %s -DPATH=%/t/
+
+Test that modules can be added with absolute paths when the archive is created using a relative path
+
+RUN: llvm-ar rTc Output/%basename_t.tmp/absolute-2.ar %t/foo/elf.o %t/delete.o %t/foo/bar/elf.o
+RUN: llvm-ar dT Output/%basename_t.tmp/absolute-2.ar %t/delete.o
+
+RUN: FileCheck -input-file=%t/absolute-2.ar --check-prefixes=THIN,CHECK %s -DPATH=%/t/
+RUN: llvm-ar t %t/absolute-2.ar | FileCheck %s -DPATH=%/t/
+
+These tests must be run in %t/foo. cd %t is included on each line to make debugging this test case easier.
+
+Test that modules can be added with relative paths when the archive is created using a relative path
+
+RUN: cd %t/foo && llvm-ar rTc ../relative-1.ar elf.o ../delete.o bar/elf.o
+RUN: cd %t/foo && llvm-ar dT ../relative-1.ar delete.o
+
+RUN: FileCheck -input-file=%t/relative-1.ar --check-prefixes=THIN,CHECK %s -DPATH=
+RUN: llvm-ar t %t/relative-1.ar | FileCheck %s -DPATH=%/t/
+
+Test that modules can be added with relative paths when the archive is created using a absolute path
+
+RUN: cd %t/foo && llvm-ar rTc %t/relative-2.ar elf.o ../delete.o bar/elf.o
+RUN: cd %t/foo && llvm-ar dT %t/relative-2.ar delete.o
+
+RUN: FileCheck -input-file=%t/relative-2.ar --check-prefixes=THIN,CHECK %s -DPATH=
+RUN: llvm-ar t %t/relative-2.ar | FileCheck %s -DPATH=%/t/
+
+THIN: !<thin>
+
+CHECK-NOT: delete.o
+CHECK: {{^}}[[PATH]]foo/elf.o
+CHECK: {{^}}[[PATH]]foo/bar/elf.o

llvm/trunk/test/tools/llvm-objcopy/ELF/archive-unknown-members.test

 # CHECK: error: '[[ARCHIVE]](archive-unknown-members.test)': The file was not recognized as a valid object file
 
 # RUN: rm -f %t.thin1.a %t.thin2.a
 # RUN: cp %t.o %t1.o
 # RUN: cp %t.o %t2.o
 # RUN: llvm-ar rcT %t.thin1.a %t1.o %s
 # RUN: llvm-ar rcT %t.thin2.a %t2.o %s
 
-# RUN: not llvm-objcopy --strip-debug %t.thin1.a 2>&1 \
-# RUN:   | FileCheck %s --check-prefix=THIN -DARCHIVE=%t.thin1.a -DMEMBER=%s
-# RUN: not llvm-strip --strip-debug %t.thin2.a 2>&1 \
-# RUN:   | FileCheck %s --check-prefix=THIN -DARCHIVE=%t.thin2.a -DMEMBER=%s
+# RUN: not llvm-objcopy --strip-debug %/t.thin1.a 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=THIN -DARCHIVE=%/t.thin1.a -DMEMBER=%/s
+# RUN: not llvm-strip --strip-debug %/t.thin2.a 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=THIN -DARCHIVE=%/t.thin2.a -DMEMBER=%/s
 ## Verify that the first member was not modified, if a later member could not
 ## be recognized.
 # RUN: cmp %t.o %t1.o
 # RUN: cmp %t.o %t2.o
 
 # THIN: error: '[[ARCHIVE]]([[MEMBER]])': The file was not recognized as a valid object file
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name: .debug_foo
     Type: SHT_PROGBITS

llvm/trunk/test/tools/llvm-readobj/thin-archive-paths.test

 # RUN: not llvm-readelf --file-headers a/relative.a 2>&1 | FileCheck %s --check-prefix=ERR1
 # ERR1: error: 'a/relative.a': 'b/1.o': {{[Nn]}}o such file or directory
 
 # RUN: mkdir -p %t/c
 # RUN: cp %p/Inputs/trivial.obj.elf-x86-64 a/b/1.o
 # RUN: llvm-ar rcT c/absolute.a %t/a/b/1.o
 
 # Show that absolute paths in the file header printing are correct.
-# RUN: llvm-readobj --file-headers c/absolute.a | FileCheck %s --check-prefix=ABS -DDIR=%t
+# RUN: llvm-readobj --file-headers c/absolute.a | FileCheck %s --check-prefix=ABS -DDIR=%/t
 # ABS: File: [[DIR]]/a/b/1.o
 
 # Show that absolute paths in an error message for both archive and member are correct.
 # RUN: rm a/b/1.o
-# RUN: not llvm-readobj --file-headers %t/c/absolute.a 2>&1 | FileCheck %s --check-prefix=ERR2 -DDIR=%t
-# RUN: not llvm-readelf --file-headers %t/c/absolute.a 2>&1 | FileCheck %s --check-prefix=ERR2 -DDIR=%t
+# RUN: not llvm-readobj --file-headers %/t/c/absolute.a 2>&1 | FileCheck %s --check-prefix=ERR2 -DDIR=%/t
+# RUN: not llvm-readelf --file-headers %/t/c/absolute.a 2>&1 | FileCheck %s --check-prefix=ERR2 -DDIR=%/t
 # ERR2: error: '[[DIR]]/c/absolute.a': '[[DIR]]/a/b/1.o': {{[Nn]}}o such file or directory

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

if (Verbose) {
     // nanoseconds.
     // TODO: fix format_provider<TimePoint<>> to allow other units.
     sys::TimePoint<> ModTimeInNs = ModTimeOrErr.get();
     outs() << ' ' << formatv("{0:%b %e %H:%M %Y}", ModTimeInNs);
     outs() << ' ';
   }
 
   if (C.getParent()->isThin()) {
+    if (!sys::path::is_absolute(Name)) {
       StringRef ParentDir = sys::path::parent_path(ArchiveName);
       if (!ParentDir.empty())
-      outs() << ParentDir << '/';
+        outs() << sys::path::convert_to_slash(ParentDir) << '/';
+    }
   }
   outs() << Name << "\n";
 }
 
 static StringRef normalizePath(StringRef Path) {
   return CompareFullPath ? Path : sys::path::filename(Path);
 }
 
if (Thin && !M.getParent()->isThin())
     fail("Cannot convert a regular archive to a thin one");
   Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getOldMember(M, Deterministic);
   failIfError(NMOrErr.takeError());
   // If the child member we're trying to add is thin, use the path relative to
   // the archive it's in, so the file resolves correctly.
   if (Thin && FlattenArchive) {
     StringSaver Saver(Alloc);
-    Expected<std::string> FileNameOrErr = M.getFullName();
+    Expected<std::string> FileNameOrErr = M.getName();
     failIfError(FileNameOrErr.takeError());
-    NMOrErr->MemberName =
-        Saver.save(computeArchiveRelativePath(ArchiveName, *FileNameOrErr));
+    if (sys::path::is_absolute(*FileNameOrErr)) {
+      NMOrErr->MemberName = Saver.save(sys::path::convert_to_slash(*FileNameOrErr));
+    } else {
+      FileNameOrErr = M.getFullName();
+      failIfError(FileNameOrErr.takeError());
+      Expected<std::string> PathOrErr =
+          computeArchiveRelativePath(ArchiveName, *FileNameOrErr);
+      NMOrErr->MemberName = Saver.save(
+          PathOrErr ? *PathOrErr : sys::path::convert_to_slash(*FileNameOrErr));
+    }
   }
   if (FlattenArchive &&
       identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
     Expected<std::string> FileNameOrErr = M.getFullName();
     failIfError(FileNameOrErr.takeError());
     object::Archive &Lib = readLibrary(*FileNameOrErr);
     // When creating thin archives, only flatten if the member is also thin.
     if (!Thin || Lib.isThin()) {
static void addMember(std::vector<NewArchiveMember> &Members,
                       StringRef FileName, bool FlattenArchive = false) {
   Expected<NewArchiveMember> NMOrErr =
       NewArchiveMember::getFile(FileName, Deterministic);
   failIfError(NMOrErr.takeError(), FileName);
   StringSaver Saver(Alloc);
   // For regular archives, use the basename of the object path for the member
   // name. For thin archives, use the full relative paths so the file resolves
   // correctly.
-  NMOrErr->MemberName =
-      Thin ? Saver.save(computeArchiveRelativePath(ArchiveName, FileName))
-           : sys::path::filename(NMOrErr->MemberName);
+  if (!Thin) {
+    NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName);
+  } else {
+    if (sys::path::is_absolute(FileName))
+      NMOrErr->MemberName = Saver.save(sys::path::convert_to_slash(FileName));
+    else {
+      Expected<std::string> PathOrErr =
+          computeArchiveRelativePath(ArchiveName, FileName);
+      NMOrErr->MemberName = Saver.save(
+          PathOrErr ? *PathOrErr : sys::path::convert_to_slash(FileName));
+    }
+  }
+
   if (FlattenArchive &&
       identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) {
     object::Archive &Lib = readLibrary(FileName);
     // When creating thin archives, only flatten if the member is also thin.
     if (!Thin || Lib.isThin()) {
       Error Err = Error::success();
       // Only Thin archives are recursively flattened.
       for (auto &Child : Lib.children(Err))