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

8

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

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

502

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

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

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

514–515

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

lib/Object/ArchiveWriter.cpp

computeMemberData(raw_ostream &StringTable, raw_ostream &SymNames,
   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;
+  auto getDotlessAbsolutePath =
+      [](SmallVectorImpl<char> &P) -> std::error_code {
+    std::error_code Ret = sys::fs::make_absolute(P);
+    if (!Ret) {
+      sys::path::remove_dots(P, /*removedotdot*/ true);
+#ifdef _WIN32
+      // Canonicalise '\\' to '/' as sys::path::const_iterator may return a UNC
+      // specifier. It also returns the first directory separator after a drive
+      // letter or UNC specifier as a component of the path.
+      std::replace(P.begin(), P.end(), '\\', '/');
+#endif
+    }
+    return Ret;
+  };
 
-  StringRef DirFrom = sys::path::parent_path(From);
-  auto FromI = sys::path::begin(DirFrom);
-  auto ToI = sys::path::begin(To);
+  SmallString<128> PathTo = To;
+  SmallString<128> DirFrom = sys::path::parent_path(From);
+  if (getDotlessAbsolutePath(PathTo) || getDotlessAbsolutePath(DirFrom))
+    return sys::path::convert_to_slash(To);
+
+  auto FromS = sys::path::begin(DirFrom);
+  auto FromI = FromS;
+  auto ToI = sys::path::begin(PathTo);
   while (*FromI == *ToI) {
     ++FromI;
     ++ToI;
   }
 
+  // Can't construct a relative path between different roots
+  if ((FromI == FromS) && !sys::path::root_name(PathTo).empty())
+    return PathTo.str();
+
   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");
 

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/

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
+cd %t/foo && llvm-ar dT %t/relative-2.ar delete.o
+
+FileCheck -input-file=%t/relative-2.ar --check-prefixes=THIN,CHECK %s -DPATH=
+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
 No newline at end of file

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<StringRef> NameOrErr = M.getName();
+    failIfError(NameOrErr.takeError());
     Expected<std::string> FileNameOrErr = M.getFullName();
     failIfError(FileNameOrErr.takeError());
-    NMOrErr->MemberName =
-        Saver.save(computeArchiveRelativePath(ArchiveName, *FileNameOrErr));
+
+    std::string MemberPath;
+    if (sys::path::is_absolute(*NameOrErr) &&
+        sys::path::is_absolute(*FileNameOrErr)) {
+      MemberPath = sys::path::convert_to_slash(*FileNameOrErr);
+    } else {
+      MemberPath = computeArchiveRelativePath(ArchiveName, *FileNameOrErr);
+    }
+
+    NMOrErr->MemberName = Saver.save(MemberPath);
   }
   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))
+  auto getThinMemberPath = [](StringRef FileName) -> std::string {
+    if (sys::path::is_absolute(FileName)) {
+      return sys::path::convert_to_slash(FileName);
+    }
+    return computeArchiveRelativePath(ArchiveName, FileName);
+  };
+
+  NMOrErr->MemberName = Thin ? Saver.save(getThinMemberPath(FileName))
                              : sys::path::filename(NMOrErr->MemberName);
   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))