This fixes some thin archive relative path issues, paths are shortened where possible and paths are output correctly when using the display table command.
rupprecht ruiu espindola alexshap jhenderson
- rL362407: [llvm-ar] Fix relative thin archive path handling
rGfade9cbed763: [llvm-ar] Fix relative thin archive path handling
rL362484: [llvm-ar] Reapply Fix relative thin archive path handling
rG5d5078e341f5: [llvm-ar] Reapply Fix relative thin archive path handling
In response to rupprecht:
Currently on Windows absolute paths in an archive can have mixed slashes e.g.
And to select the member the path must be matched exactly.
llvm-ar -t 'absolute archive path' outputs the above as:
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:
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:
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.
If so, I guess the following scheme may be simpler than the current one:
- Canonicalize both P1 and P2 by converting backslashes to slashes.
- 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.
This lambda doesn't capture anything, you probably should consider moving this outside of this function.
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.
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.
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 ../.
The lambda was to keep it local to this function, i'd rather keep it unless others really disagree.
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.
Hi Rui, thanks for the feedback. I'm unsure what changes would better fit what we have outlined in past comments:
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,
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.