From ce7f3f7548dfb6a4f3696a9663d084b1e4124ad2 Mon Sep 17 00:00:00 2001 From: Hans Wennborg Date: Fri, 8 Feb 2019 10:16:45 +0000 Subject: [PATCH] Revert r353424 "[llvm-ar][libObject] Fix relative paths when nesting thin archives." This broke the Chromium build on Windows, see https://crbug.com/930058 > Summary: > When adding one thin archive to another, we currently chop off the relative path to the flattened members. For instance, when adding `foo/child.a` (which contains `x.txt`) to `parent.a`, whe > lattening it we should add it as `foo/x.txt` (which exists) instead of `x.txt` (which does not exist). > > As a note, this also undoes the `IsNew` parameter of handling relative paths in r288280. The unit test there still passes. > > This was reported as part of testing the kernel build with llvm-ar: https://patchwork.kernel.org/patch/10767545/ (see the second point). > > Reviewers: mstorsjo, pcc, ruiu, davide, david2050 > > Subscribers: hiraditya, llvm-commits > > Tags: #llvm > > Differential Revision: https://reviews.llvm.org/D57842 This reverts commit bf990ab5aab03aa0aac53c9ef47ef264307804ed. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@353507 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Object/ArchiveWriter.h | 1 + lib/Object/ArchiveWriter.cpp | 71 +++++++++++++++---- .../flatten-thin-archive-directories.test | 15 ---- test/tools/llvm-ar/flatten-thin-archive.test | 2 +- tools/llvm-ar/llvm-ar.cpp | 57 ++------------- 5 files changed, 67 insertions(+), 79 deletions(-) delete mode 100644 test/tools/llvm-ar/flatten-thin-archive-directories.test diff --git a/include/llvm/Object/ArchiveWriter.h b/include/llvm/Object/ArchiveWriter.h index cca97e73484..4e796eb1adf 100644 --- a/include/llvm/Object/ArchiveWriter.h +++ b/include/llvm/Object/ArchiveWriter.h @@ -26,6 +26,7 @@ struct NewArchiveMember { sys::TimePoint ModTime; unsigned UID = 0, GID = 0, Perms = 0644; + bool IsNew = false; NewArchiveMember() = default; NewArchiveMember(MemoryBufferRef BufRef); diff --git a/lib/Object/ArchiveWriter.cpp b/lib/Object/ArchiveWriter.cpp index 5650ca88f0c..d27233ec2f4 100644 --- a/lib/Object/ArchiveWriter.cpp +++ b/lib/Object/ArchiveWriter.cpp @@ -48,6 +48,7 @@ NewArchiveMember::getOldMember(const object::Archive::Child &OldMember, return BufOrErr.takeError(); NewArchiveMember M; + assert(M.IsNew == false); M.Buf = MemoryBuffer::getMemBuffer(*BufOrErr, false); M.MemberName = M.Buf->getBufferIdentifier(); if (!Deterministic) { @@ -97,6 +98,7 @@ Expected NewArchiveMember::getFile(StringRef FileName, return errorCodeToError(std::error_code(errno, std::generic_category())); NewArchiveMember M; + M.IsNew = true; M.Buf = std::move(*MemberBufferOrErr); M.MemberName = M.Buf->getBufferIdentifier(); if (!Deterministic) { @@ -189,6 +191,35 @@ static bool useStringTable(bool Thin, StringRef Name) { return Thin || Name.size() >= 16 || Name.contains('/'); } +// Compute the relative path from From to To. +static std::string computeRelativePath(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; + } + + SmallString<128> Relative; + for (auto FromE = sys::path::end(DirFrom); FromI != FromE; ++FromI) + sys::path::append(Relative, ".."); + + for (auto ToE = sys::path::end(To); ToI != ToE; ++ToI) + sys::path::append(Relative, *ToI); + +#ifdef _WIN32 + // Replace backslashes with slashes so that the path is portable between *nix + // and Windows. + std::replace(Relative.begin(), Relative.end(), '\\', '/'); +#endif + + return Relative.str(); +} + static bool is64BitKind(object::Archive::Kind Kind) { switch (Kind) { case object::Archive::K_GNU: @@ -203,11 +234,27 @@ static bool is64BitKind(object::Archive::Kind Kind) { llvm_unreachable("not supported for writting"); } -static void -printMemberHeader(raw_ostream &Out, uint64_t Pos, raw_ostream &StringTable, - StringMap &MemberNames, object::Archive::Kind Kind, - bool Thin, const NewArchiveMember &M, - sys::TimePoint ModTime, unsigned Size) { +static void addToStringTable(raw_ostream &Out, StringRef ArcName, + const NewArchiveMember &M, bool Thin) { + StringRef ID = M.Buf->getBufferIdentifier(); + if (Thin) { + if (M.IsNew) + Out << computeRelativePath(ArcName, ID); + else + Out << ID; + } else + Out << M.MemberName; + Out << "/\n"; +} + +static void printMemberHeader(raw_ostream &Out, uint64_t Pos, + raw_ostream &StringTable, + StringMap &MemberNames, + object::Archive::Kind Kind, bool Thin, + StringRef ArcName, const NewArchiveMember &M, + sys::TimePoint ModTime, + unsigned Size) { + if (isBSDLike(Kind)) return printBSDMemberHeader(Out, Pos, M.MemberName, ModTime, M.UID, M.GID, M.Perms, Size); @@ -218,12 +265,12 @@ printMemberHeader(raw_ostream &Out, uint64_t Pos, raw_ostream &StringTable, uint64_t NamePos; if (Thin) { NamePos = StringTable.tell(); - StringTable << M.MemberName << "/\n"; + addToStringTable(StringTable, ArcName, M, Thin); } else { auto Insertion = MemberNames.insert({M.MemberName, uint64_t(0)}); if (Insertion.second) { Insertion.first->second = StringTable.tell(); - StringTable << M.MemberName << "/\n"; + addToStringTable(StringTable, ArcName, M, Thin); } NamePos = Insertion.first->second; } @@ -385,8 +432,8 @@ getSymbols(MemoryBufferRef Buf, raw_ostream &SymNames, bool &HasObject) { static Expected> computeMemberData(raw_ostream &StringTable, raw_ostream &SymNames, - object::Archive::Kind Kind, bool Thin, bool Deterministic, - ArrayRef NewMembers) { + object::Archive::Kind Kind, bool Thin, StringRef ArcName, + bool Deterministic, ArrayRef NewMembers) { static char PaddingData[8] = {'\n', '\n', '\n', '\n', '\n', '\n', '\n', '\n'}; // This ignores the symbol table, but we only need the value mod 8 and the @@ -473,8 +520,8 @@ computeMemberData(raw_ostream &StringTable, raw_ostream &SymNames, ModTime = sys::toTimePoint(FilenameCount[M.MemberName]++); else ModTime = M.ModTime; - printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, M, - ModTime, Buf.getBufferSize() + MemberPadding); + printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, ArcName, + M, ModTime, Buf.getBufferSize() + MemberPadding); Out.flush(); Expected> Symbols = @@ -506,7 +553,7 @@ Error llvm::writeArchive(StringRef ArcName, raw_svector_ostream StringTable(StringTableBuf); Expected> DataOrErr = computeMemberData( - StringTable, SymNames, Kind, Thin, Deterministic, NewMembers); + StringTable, SymNames, Kind, Thin, ArcName, Deterministic, NewMembers); if (Error E = DataOrErr.takeError()) return E; std::vector &Data = *DataOrErr; diff --git a/test/tools/llvm-ar/flatten-thin-archive-directories.test b/test/tools/llvm-ar/flatten-thin-archive-directories.test deleted file mode 100644 index 6b5fc28658a..00000000000 --- a/test/tools/llvm-ar/flatten-thin-archive-directories.test +++ /dev/null @@ -1,15 +0,0 @@ -# This test creates a thin archive in a directory and adds it to a thin archive -# in the parent directory. The relative path should be included when flattening -# the archive. - -RUN: mkdir -p %t/foo -RUN: touch %t/foo/a.txt -RUN: rm -f %t/archive.a %t/foo/archive.a - -# These tests must be run in the same directory as %t/archive.a. cd %t is -# included on each line to make debugging this test case easier. -RUN: cd %t && llvm-ar rcST foo/archive.a foo/a.txt -RUN: cd %t && llvm-ar rcST archive.a foo/archive.a -RUN: cd %t && llvm-ar t archive.a | FileCheck %s --match-full-lines - -CHECK: foo/a.txt diff --git a/test/tools/llvm-ar/flatten-thin-archive.test b/test/tools/llvm-ar/flatten-thin-archive.test index a80edd9d50c..430f48fe7c4 100644 --- a/test/tools/llvm-ar/flatten-thin-archive.test +++ b/test/tools/llvm-ar/flatten-thin-archive.test @@ -6,7 +6,7 @@ # flattened members appearing together. RUN: touch %t-a.txt %t-b.txt %t-c.txt %t-d.txt %t-e.txt -RUN: rm -f %t-a-plus-b.a %t-d-plus-e.a %t.a +RUN: rm -f %t-a-plus-b.a %t.a RUN: llvm-ar rcsT %t-a-plus-b.a %t-a.txt %t-b.txt RUN: llvm-ar rcs %t-d-plus-e.a %t-d.txt %t-e.txt RUN: llvm-ar rcsT %t.a %t-a-plus-b.a %t-c.txt %t-d-plus-e.a diff --git a/tools/llvm-ar/llvm-ar.cpp b/tools/llvm-ar/llvm-ar.cpp index 6e0be0a8158..0119ec502e5 100644 --- a/tools/llvm-ar/llvm-ar.cpp +++ b/tools/llvm-ar/llvm-ar.cpp @@ -193,9 +193,6 @@ static std::string ArchiveName; // on the command line. static std::vector Members; -// Static buffer to hold StringRefs. -static BumpPtrAllocator Alloc; - // Extract the member filename from the command line for the [relpos] argument // associated with a, b, and i modifiers static void getRelPos() { @@ -540,35 +537,6 @@ static void performReadOperation(ArchiveOperation Operation, exit(1); } -// Compute the relative path from From to To. -static std::string computeRelativePath(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; - } - - SmallString<128> Relative; - for (auto FromE = sys::path::end(DirFrom); FromI != FromE; ++FromI) - sys::path::append(Relative, ".."); - - for (auto ToE = sys::path::end(To); ToI != ToE; ++ToI) - sys::path::append(Relative, *ToI); - -#ifdef _WIN32 - // Replace backslashes with slashes so that the path is portable between *nix - // and Windows. - std::replace(Relative.begin(), Relative.end(), '\\', '/'); -#endif - - return Relative.str(); -} - static void addChildMember(std::vector &Members, const object::Archive::Child &M, bool FlattenArchive = false) { @@ -577,15 +545,6 @@ static void addChildMember(std::vector &Members, Expected 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 FileNameOrErr = M.getFullName(); - failIfError(FileNameOrErr.takeError()); - NMOrErr->MemberName = - Saver.save(computeRelativePath(ArchiveName, *FileNameOrErr)); - } if (FlattenArchive && identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) { Expected FileNameOrErr = M.getFullName(); @@ -609,13 +568,6 @@ static void addMember(std::vector &Members, Expected 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(computeRelativePath(ArchiveName, FileName)) - : sys::path::filename(NMOrErr->MemberName); if (FlattenArchive && identify_magic(NMOrErr->Buf->getBuffer()) == file_magic::archive) { object::Archive &Lib = readLibrary(FileName); @@ -629,6 +581,8 @@ static void addMember(std::vector &Members, return; } } + // Use the basename of the object path for the member name. + NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName); Members.push_back(std::move(*NMOrErr)); } @@ -718,7 +672,7 @@ computeNewArchiveMembers(ArchiveOperation Operation, computeInsertAction(Operation, Child, Name, MemberI); switch (Action) { case IA_AddOldMember: - addChildMember(Ret, Child, /*FlattenArchive=*/Thin); + addChildMember(Ret, Child); break; case IA_AddNewMember: addMember(Ret, *MemberI); @@ -726,7 +680,7 @@ computeNewArchiveMembers(ArchiveOperation Operation, case IA_Delete: break; case IA_MoveOldMember: - addChildMember(Moved, Child, /*FlattenArchive=*/Thin); + addChildMember(Moved, Child); break; case IA_MoveNewMember: addMember(Moved, *MemberI); @@ -945,7 +899,7 @@ static void runMRIScript() { { Error Err = Error::success(); for (auto &Member : Lib.children(Err)) - addChildMember(NewMembers, Member, /*FlattenArchive=*/Thin); + addChildMember(NewMembers, Member); failIfError(std::move(Err)); } break; @@ -997,6 +951,7 @@ static bool handleGenericOption(StringRef arg) { static int ar_main(int argc, char **argv) { SmallVector Argv(argv, argv + argc); + BumpPtrAllocator Alloc; StringSaver Saver(Alloc); cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Argv); for (size_t i = 1; i < Argv.size(); ++i) { -- 2.50.1