[llvm] r362407 - [llvm-ar] Fix relative thin archive path handling
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 9 16:17:37 PDT 2019
Ah, that makes sense - thanks!
On Tue, Jul 9, 2019 at 3:28 AM Owen Reynolds <gbreynoo at gmail.com> wrote:
> 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/5f08e06c/attachment.html>
More information about the llvm-commits
mailing list