[llvm] r362407 - [llvm-ar] Fix relative thin archive path handling
Owen Reynolds via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 9 03:28:10 PDT 2019
Hi David,
Sorry for the late response, I realise I did not reply or comment regarding
this fix on the original review for the commit.
The commit above was made on June 3rd (362407) and reverted shortly after
due to the mismatch issue you highlighted. On the 4th I committed a fix
(362484) that uses a different call to mismatch that I believe is in c++11:
template< class InputIt1, class InputIt2 >
std::pair<InputIt1,InputIt2>
mismatch( InputIt1 first1, InputIt1 last1,
InputIt2 first2 );
I'd appreciate your thoughts on this however and will revert if similar
issues still occur.
Thanks,
Owen
On Mon, Jul 8, 2019 at 10:46 PM David Blaikie <dblaikie at gmail.com> wrote:
> Ping
>
> On Mon, Jun 3, 2019 at 9:06 AM David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Mon, Jun 3, 2019 at 8:22 AM Owen Reynolds via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: gbreynoo
>>> Date: Mon Jun 3 08:26:07 2019
>>> New Revision: 362407
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=362407&view=rev
>>> Log:
>>> [llvm-ar] Fix relative thin archive path handling
>>>
>>> This fixes some thin archive relative path issues, paths are shortened
>>> where possible and paths are output correctly when using the display table
>>> command.
>>>
>>> Differential Revision: https://reviews.llvm.org/D59491
>>>
>>> Added:
>>> llvm/trunk/test/tools/llvm-ar/reduce-thin-path.test
>>> llvm/trunk/test/tools/llvm-ar/thin-archive.test
>>> Modified:
>>> llvm/trunk/include/llvm/Object/ArchiveWriter.h
>>> llvm/trunk/lib/Object/ArchiveWriter.cpp
>>> llvm/trunk/lib/ToolDrivers/llvm-lib/LibDriver.cpp
>>> llvm/trunk/test/tools/llvm-objcopy/ELF/archive-unknown-members.test
>>> llvm/trunk/test/tools/llvm-readobj/thin-archive-paths.test
>>> llvm/trunk/tools/llvm-ar/llvm-ar.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/Object/ArchiveWriter.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/ArchiveWriter.h?rev=362407&r1=362406&r2=362407&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Object/ArchiveWriter.h (original)
>>> +++ llvm/trunk/include/llvm/Object/ArchiveWriter.h Mon Jun 3 08:26:07
>>> 2019
>>> @@ -36,7 +36,7 @@ struct NewArchiveMember {
>>> 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,
>>>
>>> Modified: llvm/trunk/lib/Object/ArchiveWriter.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/ArchiveWriter.cpp?rev=362407&r1=362406&r2=362407&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Object/ArchiveWriter.cpp (original)
>>> +++ llvm/trunk/lib/Object/ArchiveWriter.cpp Mon Jun 3 08:26:07 2019
>>> @@ -494,29 +494,46 @@ computeMemberData(raw_ostream &StringTab
>>> }
>>>
>>> namespace llvm {
>>> +
>>> +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.
>>> -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;
>>> - }
>>> +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));
>>>
>>
>> Looks like std::mismatch isn't in C++11, so cannot/should not be used in
>> LLVM at the moment? Could you revert this patch, or if there's a quick fix
>> (O(minutes)), fix forward?
>>
>>
>>> + 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,
>>>
>>> Modified: llvm/trunk/lib/ToolDrivers/llvm-lib/LibDriver.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ToolDrivers/llvm-lib/LibDriver.cpp?rev=362407&r1=362406&r2=362407&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/ToolDrivers/llvm-lib/LibDriver.cpp (original)
>>> +++ llvm/trunk/lib/ToolDrivers/llvm-lib/LibDriver.cpp Mon Jun 3
>>> 08:26:07 2019
>>> @@ -211,9 +211,14 @@ int llvm::libDriverMain(ArrayRef<const c
>>> // 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,
>>>
>>> Added: llvm/trunk/test/tools/llvm-ar/reduce-thin-path.test
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-ar/reduce-thin-path.test?rev=362407&view=auto
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/tools/llvm-ar/reduce-thin-path.test (added)
>>> +++ llvm/trunk/test/tools/llvm-ar/reduce-thin-path.test Mon Jun 3
>>> 08:26:07 2019
>>> @@ -0,0 +1,10 @@
>>> +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/
>>>
>>> Added: llvm/trunk/test/tools/llvm-ar/thin-archive.test
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-ar/thin-archive.test?rev=362407&view=auto
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/tools/llvm-ar/thin-archive.test (added)
>>> +++ llvm/trunk/test/tools/llvm-ar/thin-archive.test Mon Jun 3 08:26:07
>>> 2019
>>> @@ -0,0 +1,45 @@
>>> +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
>>>
>>> Modified:
>>> llvm/trunk/test/tools/llvm-objcopy/ELF/archive-unknown-members.test
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/ELF/archive-unknown-members.test?rev=362407&r1=362406&r2=362407&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/tools/llvm-objcopy/ELF/archive-unknown-members.test
>>> (original)
>>> +++ llvm/trunk/test/tools/llvm-objcopy/ELF/archive-unknown-members.test
>>> Mon Jun 3 08:26:07 2019
>>> @@ -23,10 +23,10 @@
>>> # 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
>>>
>>> Modified: llvm/trunk/test/tools/llvm-readobj/thin-archive-paths.test
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-readobj/thin-archive-paths.test?rev=362407&r1=362406&r2=362407&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/test/tools/llvm-readobj/thin-archive-paths.test (original)
>>> +++ llvm/trunk/test/tools/llvm-readobj/thin-archive-paths.test Mon Jun
>>> 3 08:26:07 2019
>>> @@ -23,11 +23,11 @@
>>> # 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
>>>
>>> Modified: llvm/trunk/tools/llvm-ar/llvm-ar.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-ar/llvm-ar.cpp?rev=362407&r1=362406&r2=362407&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/tools/llvm-ar/llvm-ar.cpp (original)
>>> +++ llvm/trunk/tools/llvm-ar/llvm-ar.cpp Mon Jun 3 08:26:07 2019
>>> @@ -464,9 +464,11 @@ static void doDisplayTable(StringRef Nam
>>> }
>>>
>>> if (C.getParent()->isThin()) {
>>> - StringRef ParentDir = sys::path::parent_path(ArchiveName);
>>> - if (!ParentDir.empty())
>>> - outs() << ParentDir << '/';
>>> + if (!sys::path::is_absolute(Name)) {
>>> + StringRef ParentDir = sys::path::parent_path(ArchiveName);
>>> + if (!ParentDir.empty())
>>> + outs() << sys::path::convert_to_slash(ParentDir) << '/';
>>> + }
>>> }
>>> outs() << Name << "\n";
>>> }
>>> @@ -593,10 +595,18 @@ static void addChildMember(std::vector<N
>>> // 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)
>>> {
>>> @@ -625,9 +635,19 @@ static void addMember(std::vector<NewArc
>>> // 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);
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190709/def1c3bc/attachment-0001.html>
More information about the llvm-commits
mailing list