From 8ec68fad33e4f5b9899f200b32a264df1a55be48 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Wed, 29 Jun 2016 22:27:42 +0000 Subject: [PATCH] Object: Replace NewArchiveIterator with a simpler NewArchiveMember class. NFCI. The NewArchiveIterator class has a problem: it requires too much context. Any memory buffers added to the archive must be stored within an Archive::Member, which must have an associated Archive. This makes it harder than necessary to create new archive members (or new archives entirely) from scratch using memory buffers. This patch replaces NewArchiveIterator with a NewArchiveMember class that stores just the memory buffer and the information that goes into the archive member header. Differential Revision: http://reviews.llvm.org/D21721 llvm-svn: 274183 --- llvm/include/llvm/Object/ArchiveWriter.h | 26 ++-- llvm/lib/LibDriver/LibDriver.cpp | 16 ++- llvm/lib/Object/ArchiveWriter.cpp | 174 ++++++++--------------- llvm/tools/llvm-ar/llvm-ar.cpp | 121 +++++++--------- 4 files changed, 137 insertions(+), 200 deletions(-) diff --git a/llvm/include/llvm/Object/ArchiveWriter.h b/llvm/include/llvm/Object/ArchiveWriter.h index a3cc64b52167..cabccc89685b 100644 --- a/llvm/include/llvm/Object/ArchiveWriter.h +++ b/llvm/include/llvm/Object/ArchiveWriter.h @@ -20,27 +20,23 @@ namespace llvm { -class NewArchiveIterator { - bool IsNewMember; - StringRef Name; +struct NewArchiveMember { + std::unique_ptr Buf; + sys::TimeValue ModTime = sys::TimeValue::PosixZeroTime(); + unsigned UID = 0, GID = 0, Perms = 0644; - object::Archive::Child OldMember; + NewArchiveMember() = default; + NewArchiveMember(MemoryBufferRef BufRef); -public: - NewArchiveIterator(const object::Archive::Child &OldMember, StringRef Name); - NewArchiveIterator(StringRef FileName); - bool isNewMember() const; - StringRef getName() const; + static Expected + getOldMember(const object::Archive::Child &OldMember, bool Deterministic); - const object::Archive::Child &getOld() const; - - StringRef getNew() const; - llvm::ErrorOr getFD(sys::fs::file_status &NewStatus) const; - const sys::fs::file_status &getStatus() const; + static Expected getFile(StringRef FileName, + bool Deterministic); }; std::pair -writeArchive(StringRef ArcName, std::vector &NewMembers, +writeArchive(StringRef ArcName, std::vector &NewMembers, bool WriteSymtab, object::Archive::Kind Kind, bool Deterministic, bool Thin, std::unique_ptr OldArchiveBuf = nullptr); } diff --git a/llvm/lib/LibDriver/LibDriver.cpp b/llvm/lib/LibDriver/LibDriver.cpp index 671cc73c576f..ea6d921d0f8e 100644 --- a/llvm/lib/LibDriver/LibDriver.cpp +++ b/llvm/lib/LibDriver/LibDriver.cpp @@ -57,10 +57,10 @@ public: } static std::string getOutputPath(llvm::opt::InputArgList *Args, - const llvm::NewArchiveIterator &FirstMember) { + const llvm::NewArchiveMember &FirstMember) { if (auto *Arg = Args->getLastArg(OPT_out)) return Arg->getValue(); - SmallString<128> Val = FirstMember.getNew(); + SmallString<128> Val = StringRef(FirstMember.Buf->getBufferIdentifier()); llvm::sys::path::replace_extension(Val, ".lib"); return Val.str(); } @@ -128,14 +128,22 @@ int llvm::libDriverMain(llvm::ArrayRef ArgsArr) { std::vector SearchPaths = getSearchPaths(&Args, Saver); - std::vector Members; + std::vector Members; for (auto *Arg : Args.filtered(OPT_INPUT)) { Optional Path = findInputFile(Arg->getValue(), SearchPaths); if (!Path.hasValue()) { llvm::errs() << Arg->getValue() << ": no such file or directory\n"; return 1; } - Members.emplace_back(Saver.save(*Path)); + Expected MOrErr = + NewArchiveMember::getFile(Saver.save(*Path), /*Deterministic=*/true); + if (!MOrErr) { + handleAllErrors(MOrErr.takeError(), [&](const llvm::ErrorInfoBase &EIB) { + llvm::errs() << Arg->getValue() << ": " << EIB.message() << "\n"; + }); + return 1; + } + Members.emplace_back(std::move(*MOrErr)); } std::pair Result = diff --git a/llvm/lib/Object/ArchiveWriter.cpp b/llvm/lib/Object/ArchiveWriter.cpp index 93e285cf6740..53573262104c 100644 --- a/llvm/lib/Object/ArchiveWriter.cpp +++ b/llvm/lib/Object/ArchiveWriter.cpp @@ -34,45 +34,61 @@ using namespace llvm; -NewArchiveIterator::NewArchiveIterator(const object::Archive::Child &OldMember, - StringRef Name) - : IsNewMember(false), Name(Name), OldMember(OldMember) {} +NewArchiveMember::NewArchiveMember(MemoryBufferRef BufRef) + : Buf(MemoryBuffer::getMemBuffer(BufRef, false)) {} -NewArchiveIterator::NewArchiveIterator(StringRef FileName) - : IsNewMember(true), Name(FileName), OldMember(nullptr, nullptr, nullptr) {} +Expected +NewArchiveMember::getOldMember(const object::Archive::Child &OldMember, + bool Deterministic) { + ErrorOr BufOrErr = OldMember.getMemoryBufferRef(); + if (!BufOrErr) + return errorCodeToError(BufOrErr.getError()); -StringRef NewArchiveIterator::getName() const { return Name; } - -bool NewArchiveIterator::isNewMember() const { return IsNewMember; } - -const object::Archive::Child &NewArchiveIterator::getOld() const { - assert(!IsNewMember); - return OldMember; + NewArchiveMember M; + M.Buf = MemoryBuffer::getMemBuffer(*BufOrErr, false); + if (!Deterministic) { + M.ModTime = OldMember.getLastModified(); + M.UID = OldMember.getUID(); + M.GID = OldMember.getGID(); + M.Perms = OldMember.getAccessMode(); + } + return std::move(M); } -StringRef NewArchiveIterator::getNew() const { - assert(IsNewMember); - return Name; -} +Expected NewArchiveMember::getFile(StringRef FileName, + bool Deterministic) { + sys::fs::file_status Status; + int FD; + if (auto EC = sys::fs::openFileForRead(FileName, FD)) + return errorCodeToError(EC); + assert(FD != -1); -llvm::ErrorOr -NewArchiveIterator::getFD(sys::fs::file_status &NewStatus) const { - assert(IsNewMember); - int NewFD; - if (auto EC = sys::fs::openFileForRead(Name, NewFD)) - return EC; - assert(NewFD != -1); - - if (auto EC = sys::fs::status(NewFD, NewStatus)) - return EC; + if (auto EC = sys::fs::status(FD, Status)) + return errorCodeToError(EC); // Opening a directory doesn't make sense. Let it fail. // Linux cannot open directories with open(2), although // cygwin and *bsd can. - if (NewStatus.type() == sys::fs::file_type::directory_file) - return make_error_code(errc::is_a_directory); + if (Status.type() == sys::fs::file_type::directory_file) + return errorCodeToError(make_error_code(errc::is_a_directory)); - return NewFD; + ErrorOr> MemberBufferOrErr = + MemoryBuffer::getOpenFile(FD, FileName, Status.getSize(), false); + if (!MemberBufferOrErr) + return errorCodeToError(MemberBufferOrErr.getError()); + + if (close(FD) != 0) + return errorCodeToError(std::error_code(errno, std::generic_category())); + + NewArchiveMember M; + M.Buf = std::move(*MemberBufferOrErr); + if (!Deterministic) { + M.ModTime = Status.getLastModificationTime(); + M.UID = Status.getUser(); + M.GID = Status.getGroup(); + M.Perms = Status.permissions(); + } + return std::move(M); } template @@ -178,12 +194,13 @@ static std::string computeRelativePath(StringRef From, StringRef To) { } static void writeStringTable(raw_fd_ostream &Out, StringRef ArcName, - ArrayRef Members, + ArrayRef Members, std::vector &StringMapIndexes, bool Thin) { unsigned StartOffset = 0; - for (const NewArchiveIterator &I : Members) { - StringRef Name = sys::path::filename(I.getName()); + for (const NewArchiveMember &M : Members) { + StringRef Path = M.Buf->getBufferIdentifier(); + StringRef Name = sys::path::filename(Path); if (!useStringTable(Thin, Name)) continue; if (StartOffset == 0) { @@ -194,7 +211,7 @@ static void writeStringTable(raw_fd_ostream &Out, StringRef ArcName, StringMapIndexes.push_back(Out.tell() - StartOffset); if (Thin) - Out << computeRelativePath(ArcName, I.getName()); + Out << computeRelativePath(ArcName, Path); else Out << Name; @@ -221,8 +238,7 @@ static sys::TimeValue now(bool Deterministic) { // Returns the offset of the first reference to a member offset. static ErrorOr writeSymbolTable(raw_fd_ostream &Out, object::Archive::Kind Kind, - ArrayRef Members, - ArrayRef Buffers, + ArrayRef Members, std::vector &MemberOffsetRefs, bool Deterministic) { unsigned HeaderStartOffset = 0; unsigned BodyStartOffset = 0; @@ -230,7 +246,7 @@ writeSymbolTable(raw_fd_ostream &Out, object::Archive::Kind Kind, raw_svector_ostream NameOS(NameBuf); LLVMContext Context; for (unsigned MemberNum = 0, N = Members.size(); MemberNum < N; ++MemberNum) { - MemoryBufferRef MemberBuffer = Buffers[MemberNum]; + MemoryBufferRef MemberBuffer = Members[MemberNum].Buf->getMemBufferRef(); Expected> ObjOrErr = object::SymbolicFile::createSymbolicFile( MemberBuffer, sys::fs::file_magic::unknown, &Context); @@ -305,7 +321,7 @@ writeSymbolTable(raw_fd_ostream &Out, object::Archive::Kind Kind, std::pair llvm::writeArchive(StringRef ArcName, - std::vector &NewMembers, + std::vector &NewMembers, bool WriteSymtab, object::Archive::Kind Kind, bool Deterministic, bool Thin, std::unique_ptr OldArchiveBuf) { @@ -330,43 +346,10 @@ llvm::writeArchive(StringRef ArcName, std::vector Members; std::vector NewMemberStatus; - for (NewArchiveIterator &Member : NewMembers) { - MemoryBufferRef MemberRef; - - if (Member.isNewMember()) { - StringRef Filename = Member.getNew(); - NewMemberStatus.resize(NewMemberStatus.size() + 1); - sys::fs::file_status &Status = NewMemberStatus.back(); - ErrorOr FD = Member.getFD(Status); - if (auto EC = FD.getError()) - return std::make_pair(Filename, EC); - ErrorOr> MemberBufferOrErr = - MemoryBuffer::getOpenFile(FD.get(), Filename, Status.getSize(), - false); - if (auto EC = MemberBufferOrErr.getError()) - return std::make_pair(Filename, EC); - if (close(FD.get()) != 0) - return std::make_pair(Filename, - std::error_code(errno, std::generic_category())); - Buffers.push_back(std::move(MemberBufferOrErr.get())); - MemberRef = Buffers.back()->getMemBufferRef(); - } else { - const object::Archive::Child &OldMember = Member.getOld(); - assert((!Thin || OldMember.getParent()->isThin()) && - "Thin archives cannot refers to member of other archives"); - ErrorOr MemberBufferOrErr = - OldMember.getMemoryBufferRef(); - if (auto EC = MemberBufferOrErr.getError()) - return std::make_pair("", EC); - MemberRef = MemberBufferOrErr.get(); - } - Members.push_back(MemberRef); - } - unsigned MemberReferenceOffset = 0; if (WriteSymtab) { ErrorOr MemberReferenceOffsetOrErr = writeSymbolTable( - Out, Kind, NewMembers, Members, MemberOffsetRefs, Deterministic); + Out, Kind, NewMembers, MemberOffsetRefs, Deterministic); if (auto EC = MemberReferenceOffsetOrErr.getError()) return std::make_pair(ArcName, EC); MemberReferenceOffset = MemberReferenceOffsetOrErr.get(); @@ -376,55 +359,18 @@ llvm::writeArchive(StringRef ArcName, if (Kind != object::Archive::K_BSD) writeStringTable(Out, ArcName, NewMembers, StringMapIndexes, Thin); - unsigned MemberNum = 0; - unsigned NewMemberNum = 0; std::vector::iterator StringMapIndexIter = StringMapIndexes.begin(); std::vector MemberOffset; - for (const NewArchiveIterator &I : NewMembers) { - MemoryBufferRef File = Members[MemberNum++]; + for (const NewArchiveMember &M : NewMembers) { + MemoryBufferRef File = M.Buf->getMemBufferRef(); unsigned Pos = Out.tell(); MemberOffset.push_back(Pos); - sys::TimeValue ModTime; - unsigned UID; - unsigned GID; - unsigned Perms; - if (Deterministic) { - ModTime.fromEpochTime(0); - UID = 0; - GID = 0; - Perms = 0644; - } else if (I.isNewMember()) { - const sys::fs::file_status &Status = NewMemberStatus[NewMemberNum]; - ModTime = Status.getLastModificationTime(); - UID = Status.getUser(); - GID = Status.getGroup(); - Perms = Status.permissions(); - } else { - const object::Archive::Child &OldMember = I.getOld(); - ModTime = OldMember.getLastModified(); - UID = OldMember.getUID(); - GID = OldMember.getGID(); - Perms = OldMember.getAccessMode(); - } - - if (I.isNewMember()) { - StringRef FileName = I.getNew(); - const sys::fs::file_status &Status = NewMemberStatus[NewMemberNum++]; - printMemberHeader(Out, Kind, Thin, sys::path::filename(FileName), - StringMapIndexIter, ModTime, UID, GID, Perms, - Status.getSize()); - } else { - const object::Archive::Child &OldMember = I.getOld(); - ErrorOr Size = OldMember.getSize(); - if (std::error_code EC = Size.getError()) - return std::make_pair("", EC); - StringRef FileName = I.getName(); - printMemberHeader(Out, Kind, Thin, sys::path::filename(FileName), - StringMapIndexIter, ModTime, UID, GID, Perms, - Size.get()); - } + printMemberHeader(Out, Kind, Thin, + sys::path::filename(M.Buf->getBufferIdentifier()), + StringMapIndexIter, M.ModTime, M.UID, M.GID, M.Perms, + M.Buf->getBufferSize()); if (!Thin) Out << File.getBuffer(); diff --git a/llvm/tools/llvm-ar/llvm-ar.cpp b/llvm/tools/llvm-ar/llvm-ar.cpp index f918a3e51364..d52355216b0d 100644 --- a/llvm/tools/llvm-ar/llvm-ar.cpp +++ b/llvm/tools/llvm-ar/llvm-ar.cpp @@ -65,6 +65,18 @@ static void failIfError(std::error_code EC, Twine Context = "") { fail(Context + ": " + EC.message()); } +static void failIfError(Error E, Twine Context = "") { + if (!E) + return; + + handleAllErrors(std::move(E), [&](const llvm::ErrorInfoBase &EIB) { + std::string ContextStr = Context.str(); + if (ContextStr == "") + fail(EIB.message()); + fail(Context + ": " + EIB.message()); + }); +} + // llvm-ar/llvm-ranlib remaining positional arguments. static cl::list RestOfArgs(cl::Positional, cl::ZeroOrMore, @@ -429,25 +441,28 @@ static void performReadOperation(ArchiveOperation Operation, std::exit(1); } -static void addMember(std::vector &Members, +static void addMember(std::vector &Members, StringRef FileName, int Pos = -1) { - NewArchiveIterator NI(FileName); + Expected NMOrErr = + NewArchiveMember::getFile(FileName, Deterministic); + failIfError(NMOrErr.takeError(), FileName); if (Pos == -1) - Members.push_back(NI); + Members.push_back(std::move(*NMOrErr)); else - Members[Pos] = NI; + Members[Pos] = std::move(*NMOrErr); } -static void addMember(std::vector &Members, - const object::Archive::Child &M, StringRef Name, - int Pos = -1) { +static void addMember(std::vector &Members, + const object::Archive::Child &M, int Pos = -1) { if (Thin && !M.getParent()->isThin()) fail("Cannot convert a regular archive to a thin one"); - NewArchiveIterator NI(M, Name); + Expected NMOrErr = + NewArchiveMember::getOldMember(M, Deterministic); + failIfError(NMOrErr.takeError()); if (Pos == -1) - Members.push_back(NI); + Members.push_back(std::move(*NMOrErr)); else - Members[Pos] = NI; + Members[Pos] = std::move(*NMOrErr); } enum InsertAction { @@ -508,11 +523,11 @@ static InsertAction computeInsertAction(ArchiveOperation Operation, // We have to walk this twice and computing it is not trivial, so creating an // explicit std::vector is actually fairly efficient. -static std::vector +static std::vector computeNewArchiveMembers(ArchiveOperation Operation, object::Archive *OldArchive) { - std::vector Ret; - std::vector Moved; + std::vector Ret; + std::vector Moved; int InsertPos = -1; StringRef PosName = sys::path::filename(RelPos); if (OldArchive) { @@ -536,7 +551,7 @@ computeNewArchiveMembers(ArchiveOperation Operation, computeInsertAction(Operation, Child, Name, MemberI); switch (Action) { case IA_AddOldMember: - addMember(Ret, Child, Name); + addMember(Ret, Child); break; case IA_AddNewMeber: addMember(Ret, *MemberI); @@ -544,7 +559,7 @@ computeNewArchiveMembers(ArchiveOperation Operation, case IA_Delete: break; case IA_MoveOldMember: - addMember(Moved, Child, Name); + addMember(Moved, Child); break; case IA_MoveNewMember: addMember(Moved, *MemberI); @@ -565,10 +580,15 @@ computeNewArchiveMembers(ArchiveOperation Operation, InsertPos = Ret.size(); assert(unsigned(InsertPos) <= Ret.size()); - Ret.insert(Ret.begin() + InsertPos, Moved.begin(), Moved.end()); - - Ret.insert(Ret.begin() + InsertPos, Members.size(), NewArchiveIterator("")); int Pos = InsertPos; + for (auto &M : Moved) { + Ret.insert(Ret.begin() + Pos, std::move(M)); + ++Pos; + } + + for (unsigned I = 0; I != Members.size(); ++I) + Ret.insert(Ret.begin() + InsertPos, NewArchiveMember()); + Pos = InsertPos; for (auto &Member : Members) { addMember(Ret, Member, Pos); ++Pos; @@ -582,56 +602,26 @@ static object::Archive::Kind getDefaultForHost() { : object::Archive::K_GNU; } -static object::Archive::Kind -getKindFromMember(const NewArchiveIterator &Member) { - auto getKindFromMemberInner = - [](MemoryBufferRef Buffer) -> object::Archive::Kind { - Expected> OptionalObject = - object::ObjectFile::createObjectFile(Buffer); +static object::Archive::Kind getKindFromMember(const NewArchiveMember &Member) { + Expected> OptionalObject = + object::ObjectFile::createObjectFile(Member.Buf->getMemBufferRef()); - if (OptionalObject) - return isa(**OptionalObject) - ? object::Archive::K_BSD - : object::Archive::K_GNU; + if (OptionalObject) + return isa(**OptionalObject) + ? object::Archive::K_BSD + : object::Archive::K_GNU; - // squelch the error in case we had a non-object file - consumeError(OptionalObject.takeError()); - return getDefaultForHost(); - }; - - if (Member.isNewMember()) { - object::Archive::Kind Kind = getDefaultForHost(); - - sys::fs::file_status Status; - if (auto OptionalFD = Member.getFD(Status)) { - if (auto MB = MemoryBuffer::getOpenFile(*OptionalFD, Member.getName(), - Status.getSize(), false)) - Kind = getKindFromMemberInner((*MB)->getMemBufferRef()); - - if (close(*OptionalFD) != 0) - failIfError(std::error_code(errno, std::generic_category()), - "failed to close file"); - } - - return Kind; - } else { - const object::Archive::Child &OldMember = Member.getOld(); - if (OldMember.getParent()->isThin()) - return object::Archive::Kind::K_GNU; - - auto OptionalMB = OldMember.getMemoryBufferRef(); - failIfError(OptionalMB.getError()); - - return getKindFromMemberInner(*OptionalMB); - } + // squelch the error in case we had a non-object file + consumeError(OptionalObject.takeError()); + return getDefaultForHost(); } static void performWriteOperation(ArchiveOperation Operation, object::Archive *OldArchive, std::unique_ptr OldArchiveBuf, - std::vector *NewMembersP) { - std::vector NewMembers; + std::vector *NewMembersP) { + std::vector NewMembers; if (!NewMembersP) NewMembers = computeNewArchiveMembers(Operation, OldArchive); @@ -681,7 +671,7 @@ static void createSymbolTable(object::Archive *OldArchive) { static void performOperation(ArchiveOperation Operation, object::Archive *OldArchive, std::unique_ptr OldArchiveBuf, - std::vector *NewMembers) { + std::vector *NewMembers) { switch (Operation) { case Print: case DisplayTable: @@ -704,7 +694,7 @@ static void performOperation(ArchiveOperation Operation, } static int performOperation(ArchiveOperation Operation, - std::vector *NewMembers) { + std::vector *NewMembers) { // Create or open the archive object. ErrorOr> Buf = MemoryBuffer::getFile(ArchiveName, -1, false); @@ -744,7 +734,7 @@ static void runMRIScript() { failIfError(Buf.getError()); const MemoryBuffer &Ref = *Buf.get(); bool Saved = false; - std::vector NewMembers; + std::vector NewMembers; std::vector> ArchiveBuffers; std::vector> Archives; @@ -776,10 +766,7 @@ static void runMRIScript() { object::Archive &Lib = *Archives.back(); for (auto &MemberOrErr : Lib.children()) { failIfError(MemberOrErr.getError()); - auto &Member = MemberOrErr.get(); - ErrorOr NameOrErr = Member.getName(); - failIfError(NameOrErr.getError()); - addMember(NewMembers, Member, *NameOrErr); + addMember(NewMembers, *MemberOrErr); } break; }