Page MenuHomePhabricator

Fix relative thin archive path handling
ClosedPublic

Authored by gbreynoo on Mar 18 2019, 7:45 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

gbreynoo created this revision.Mar 18 2019, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2019, 7:45 AM
ruiu added inline comments.Mar 18 2019, 10:28 AM
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 added inline comments.Mar 18 2019, 11:40 AM
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 updated this revision to Diff 191488.Mar 20 2019, 7:32 AM

Update after Rui's comment.

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

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 added a comment.Mar 20 2019, 9:15 AM

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 updated this revision to Diff 191529.Mar 20 2019, 10:30 AM
ormris removed a subscriber: ormris.Mar 20 2019, 10:35 AM
gbreynoo marked 2 inline comments as done.Mar 20 2019, 10:54 AM

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.

ping

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

Thanks

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 added a comment.Apr 17 2019, 7:26 PM

Sorry, I though that I replied to this thread.

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?

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 updated this revision to Diff 201186.May 24 2019, 4:04 AM

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 accepted this revision.May 24 2019, 5:35 AM
This revision is now accepted and ready to land.May 24 2019, 5:35 AM
This revision was automatically updated to reflect the committed changes.

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.

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

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

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