[llvm] r277177 - The next step along the way to getting good error messages for bad archives.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 1 15:00:23 PDT 2016
Re: Stylistic things you're more comfortable with. In general, I'm not
insisting on any changes, just mentioning it & totally up to you. I'll
mention some of the benefits/reasons (beyond project consistency) I'd
encourage them:
* Declarations in conditionals: This keeps the scope of the variables to
their use, which is generally preferable. (also takes up one less line of
code)
* op* rather than .get() - shorter and pretty common across a variety of
APIs (see std-or-llvm Optional for an example that isn't a pointer)
As for the "this was the way it was" - fair point. I just committed those
myself in r277394
And as for the final point/discussion... *skip ahead to the end*
On Mon, Aug 1, 2016 at 2:45 PM Kevin Enderby <enderby at apple.com> wrote:
> Hi David,
>
> Thanks for your time to take a look at these changes and your thoughtful
> comments. There were some fixes to this in r277223.
>
> I’ve added my thoughts and comments below.
>
> On Aug 1, 2016, at 10:32 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Fri, Jul 29, 2016 at 10:51 AM Kevin Enderby via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: enderby
>> Date: Fri Jul 29 12:44:13 2016
>> New Revision: 277177
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=277177&view=rev
>> Log:
>> The next step along the way to getting good error messages for bad
>> archives.
>>
>> As mentioned in commit log for r276686 this next step is adding a new
>> method in the ArchiveMemberHeader class to get the full name that
>> does proper error checking, and can be use for error messages.
>>
>> To do this the name of ArchiveMemberHeader::getName() is changed to
>> ArchiveMemberHeader::getRawName() to be consistent with
>> Archive::Child::getRawName(). Then the “new” method is the addition
>> of a new implementation of ArchiveMemberHeader::getName() which gets
>> the full name and provides proper error checking. Which is mostly a
>> rewrite
>> of what was Archive::Child::getName() and cleaning up incorrect uses of
>> llvm_unreachable() in the code which were actually just cases of errors
>> in the input Archives.
>>
>> Then Archive::Child::getName() is changed to return Expected<> and use
>> the new implementation of ArchiveMemberHeader::getName() .
>>
>> Also needed to change Archive::getMemoryBufferRef() with these
>> changes to return Expected<> as well to propagate Errors up.
>> As well as changing Archive::isThinMember() to return Expected<> .
>>
>> Added:
>> llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a
>> llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a (with props)
>> llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a
>> llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a
>> llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a
>> Modified:
>> llvm/trunk/include/llvm/Object/Archive.h
>> llvm/trunk/lib/Object/Archive.cpp
>> llvm/trunk/lib/Object/ArchiveWriter.cpp
>> llvm/trunk/test/tools/llvm-objdump/malformed-archives.test
>> llvm/trunk/tools/dsymutil/BinaryHolder.cpp
>> llvm/trunk/tools/llvm-ar/llvm-ar.cpp
>> llvm/trunk/tools/llvm-nm/llvm-nm.cpp
>> llvm/trunk/tools/llvm-objdump/MachODump.cpp
>> llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp
>> llvm/trunk/tools/llvm-size/llvm-size.cpp
>>
>> Modified: llvm/trunk/include/llvm/Object/Archive.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Archive.h?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Object/Archive.h (original)
>> +++ llvm/trunk/include/llvm/Object/Archive.h Fri Jul 29 12:44:13 2016
>> @@ -36,7 +36,10 @@ public:
>> // ArchiveMemberHeader() = default;
>>
>> /// Get the name without looking up long names.
>> - llvm::StringRef getName() const;
>> + Expected<llvm::StringRef> getRawName() const;
>> +
>> + /// Get the name looking up long names.
>> + Expected<llvm::StringRef> getName(uint64_t Size) const;
>>
>> /// Members are not larger than 4GB.
>> Expected<uint32_t> getSize() const;
>> @@ -82,7 +85,7 @@ public:
>> /// \brief Offset from Data to the start of the file.
>> uint16_t StartOfFile;
>>
>> - bool isThinMember() const;
>> + Expected<bool> isThinMember() const;
>>
>> public:
>> Child(const Archive *Parent, const char *Start, Error *Err);
>> @@ -96,9 +99,9 @@ public:
>> const Archive *getParent() const { return Parent; }
>> Expected<Child> getNext() const;
>>
>> - ErrorOr<StringRef> getName() const;
>> + Expected<StringRef> getName() const;
>> ErrorOr<std::string> getFullName() const;
>> - StringRef getRawName() const { return Header.getName(); }
>> + Expected<StringRef> getRawName() const { return Header.getRawName();
>> }
>> sys::TimeValue getLastModified() const {
>> return Header.getLastModified();
>> }
>> @@ -118,7 +121,7 @@ public:
>> ErrorOr<StringRef> getBuffer() const;
>> uint64_t getChildOffset() const;
>>
>> - ErrorOr<MemoryBufferRef> getMemoryBufferRef() const;
>> + Expected<MemoryBufferRef> getMemoryBufferRef() const;
>>
>> Expected<std::unique_ptr<Binary>>
>> getAsBinary(LLVMContext *Context = nullptr) const;
>> @@ -238,6 +241,7 @@ public:
>>
>> bool hasSymbolTable() const;
>> StringRef getSymbolTable() const { return SymbolTable; }
>> + StringRef getStringTable() const { return StringTable; }
>> uint32_t getNumberOfSymbols() const;
>>
>> std::vector<std::unique_ptr<MemoryBuffer>> takeThinBuffers() {
>>
>> Modified: llvm/trunk/lib/Object/Archive.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Archive.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Object/Archive.cpp (original)
>> +++ llvm/trunk/lib/Object/Archive.cpp Fri Jul 29 12:44:13 2016
>> @@ -43,17 +43,17 @@ ArchiveMemberHeader::ArchiveMemberHeader
>> return;
>> ErrorAsOutParameter ErrAsOutParam(Err);
>>
>> - // TODO: For errors messages with the ArchiveMemberHeader class use the
>> - // archive member name instead of the the offset to the archive member
>> header.
>> - // When there is also error getting the member name then use the
>> offset to
>> - // the member in the message.
>> -
>> if (Size < sizeof(ArMemHdrType)) {
>> if (Err) {
>> - uint64_t Offset = RawHeaderPtr - Parent->getData().data();
>> - *Err = malformedError("remaining size of archive too small for
>> next "
>> - "archive member header at offset " +
>> - Twine(Offset));
>> + Twine Msg("remaining size of archive too small for next archive
>> member "
>> + "header ");
>> + Expected<StringRef> NameOrErr = getName(Size);
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> + uint64_t Offset = RawHeaderPtr - Parent->getData().data();
>> + *Err = malformedError(Msg + "at offset " + Twine(Offset));
>> + } else
>> + *Err = malformedError(Msg + "for " + Twine(NameOrErr.get()));
>>
>
> If you like, this code might be a bit tidier by rolling in the
> initialization into the conditional:
>
> if (Expected<StringRef> NameOrErr = getName(Size))
> *Err = malformedError(...)
> else {
> consumeError(...)
>
> …
>
> }
>
>
> I can see that. But being a bit more old school I like to see definitions
> and initializations on their own line. But it does seems like lots of code
> in llvm puts the definitions and initialization into the conditional. I
> personally find it a bit harder to read. But happy to change to that.
>
>
> Also, not sure, but this code might be a bit easier less indented:
>
> if (Size < sizeof....) {
> if (!Err)
> return;
> /* populate *Err */
> return;
> }
>
> It's the same number of lines, but reduces indentation (but does duplicate
> the return which might make it harder to read - not sure)
>
>
> Yep I can see doing that either way. And your way does the early return
> which seems to be more what llvm likes. Seems a bit harder to read to me
> as I like the populating an Err after an "if (Err)” when the Error is
> optional.
>
> Also "Twine(NameOrErr.get())" doesn't need the Twine() wrapper
>
>
> Yep. And that was fixed with the follow on changes in r277223.
>
> and you can use op* rather than .get() if you like (this applies to all
> uses of Expected::get in this patch/generally), so you can rewrite it as
> "*NameOrErr”.
>
>
> Yep I have seen that done both ways. I find the overloading the op* vs
> op.get() to be much harder to read especially when there are pointers in
> the body of code. When I see a .get() I read that quicker as to what is
> going on. But again I’m a bit old school, but would be happy to use the
> op* if that is what people prefer.
>
>
> (all points similarly below)
>
>
>> }
>> return;
>> }
>> @@ -64,18 +64,35 @@ ArchiveMemberHeader::ArchiveMemberHeader
>> OS.write_escaped(llvm::StringRef(ArMemHdr->Terminator,
>> sizeof(ArMemHdr->Terminator)));
>> OS.flush();
>> - uint64_t Offset = RawHeaderPtr - Parent->getData().data();
>> - *Err = malformedError("terminator characters in archive member \""
>> + Buf +
>> - "\" not the correct \"`\\n\" values for the "
>> - "archive member header at offset " +
>> Twine(Offset));
>> + Twine Msg("terminator characters in archive member \"" + Buf + "\"
>> not "
>> + "the correct \"`\\n\" values for the archive member
>> header ");
>> + Expected<StringRef> NameOrErr = getName(Size);
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> + uint64_t Offset = RawHeaderPtr - Parent->getData().data();
>> + *Err = malformedError(Msg + "at offset " + Twine(Offset));
>> + } else
>> + *Err = malformedError(Msg + "for " + Twine(NameOrErr.get()));
>> }
>> return;
>> }
>> }
>>
>> -StringRef ArchiveMemberHeader::getName() const {
>> +// This gets the raw name from the ArMemHdr->Name field and checks that
>> it is
>> +// valid for the kind of archive. If it is not valid it returns an
>> Error.
>> +Expected<StringRef> ArchiveMemberHeader::getRawName() const {
>> char EndCond;
>> - if (ArMemHdr->Name[0] == '/' || ArMemHdr->Name[0] == '#')
>> + auto Kind = Parent->kind();
>> + if (Kind == Archive::K_BSD || Kind == Archive::K_DARWIN64) {
>> + if (ArMemHdr->Name[0] == ' ') {
>> + uint64_t Offset = reinterpret_cast<const char *>(ArMemHdr) -
>> + Parent->getData().data();
>> + return malformedError("name contains a leading space for archive
>> member "
>> + "header at offset " + Twine(Offset));
>> + }
>> + EndCond = ' ';
>> + }
>> + else if (ArMemHdr->Name[0] == '/' || ArMemHdr->Name[0] == '#')
>> EndCond = ' ';
>> else
>> EndCond = '/';
>> @@ -88,6 +105,105 @@ StringRef ArchiveMemberHeader::getName()
>> return llvm::StringRef(ArMemHdr->Name, end);
>> }
>>
>> +// This gets the name looking up long names. Size is the size of the
>> archive
>> +// member including the header, so the size of any name following the
>> header
>> +// is checked to make sure it does not overflow.
>> +Expected<StringRef> ArchiveMemberHeader::getName(uint64_t Size) const {
>> +
>> + // This can be called from the ArchiveMemberHeader constructor when the
>> + // archive header is truncated to produce an error message with the
>> name.
>> + // Make sure the name field is not truncated.
>> + if (Size < offsetof(ArMemHdrType, Name) + sizeof(ArMemHdr->Name)) {
>> + uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -
>> + Parent->getData().data();
>> + return malformedError("archive header truncated before the name
>> field "
>> + "for archive member header at offset " +
>> + Twine(ArchiveOffset));
>> + }
>> +
>> + // The raw name itself can be invalid.
>> + Expected<StringRef> NameOrErr = getRawName();
>> + if (!NameOrErr)
>> + return NameOrErr.takeError();
>> + StringRef Name = NameOrErr.get();
>> +
>> + // Check if it's a special name.
>> + if (Name[0] == '/') {
>> + if (Name.size() == 1) // Linker member.
>> + return Name;
>> + if (Name.size() == 2 && Name[1] == '/') // String table.
>> + return Name;
>> + // It's a long name.
>> + // Get the string table offset.
>> + std::size_t StringOffset;
>> + if (Name.substr(1).rtrim(' ').getAsInteger(10, StringOffset)) {
>> + std::string Buf;
>> + raw_string_ostream OS(Buf);
>> + OS.write_escaped(Name.substr(1).rtrim(' '),
>> + sizeof(Name.substr(1)).rtrim('
>> '));
>> + OS.flush();
>> + uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -
>> + Parent->getData().data();
>> + return malformedError("long name offset characters after the '/'
>> are "
>> + "not all decimal numbers: '" + Buf + "' for "
>> + "archive member header at offset " +
>> + Twine(ArchiveOffset));
>> + }
>> +
>> + // Verify it.
>> + if (StringOffset >= Parent->getStringTable().size()) {
>> + uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -
>> + Parent->getData().data();
>> + return malformedError("long name offset " + Twine(StringOffset) +
>> " past "
>> + "the end of the string table for archive
>> member "
>> + "header at offset " + Twine(ArchiveOffset));
>> + }
>> + const char *addr = Parent->getStringTable().begin() + StringOffset;
>> +
>> + // GNU long file names end with a "/\n".
>> + if (Parent->kind() == Archive::K_GNU ||
>> + Parent->kind() == Archive::K_MIPS64) {
>> + StringRef::size_type End = StringRef(addr).find('\n');
>> + return StringRef(addr, End - 1);
>> + }
>> + return StringRef(addr);
>>
>
> "return addr;" should suffice here?
>
>
> It might, as it might implicitly convert the const char * to the method’s
> return type of StringRef. But as noted in the commit log, new
> implementation of ArchiveMemberHeader::getName() is just a re-write of what
> was Archive::Child::getName(). And the code in Archive::Child::getName()
> that was deleted (see below) as exactly as above:
>
> - return StringRef(addr);
>
>
>
>> + } else if (Name.startswith("#1/")) {
>>
>
> Drop the else-after-return
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
>
>
> Sure I can make that change. But again the code in
> Archive::Child::getName() that was deleted (again see below) was exactly as
> above:
>
> - } else if (name.startswith("#1/")) {
>
>
>
>> + uint64_t NameLength;
>
>
>> + if (Name.substr(3).rtrim(' ').getAsInteger(10, NameLength)) {
>> + std::string Buf;
>> + raw_string_ostream OS(Buf);
>> + OS.write_escaped(Name.substr(3).rtrim(' '),
>> + sizeof(Name.substr(3)).rtrim('
>> '));
>> + OS.flush();
>> + uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -
>> + Parent->getData().data();
>> + return malformedError("long name length characters after the #1/
>> are "
>> + "not all decimal numbers: '" + Buf + "' for "
>> + "archive member header at offset " +
>> + Twine(ArchiveOffset));
>> + }
>> + if (getSizeOf() + NameLength > Size) {
>> + uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -
>> + Parent->getData().data();
>> + return malformedError("long name length: " + Twine(NameLength) +
>> + " extends past the end of the member or
>> archive "
>> + "for archive member header at offset " +
>> + Twine(ArchiveOffset));
>> + }
>> + return StringRef(reinterpret_cast<const char *>(ArMemHdr) +
>> getSizeOf(),
>> + NameLength).rtrim('\0');
>> + } else {
>>
>
> Another else-after-return
>
>
> Again from the original code (see below), just re-written:
>
> - return Data.substr(Header.getSizeOf(), name_size).rtrim('\0');
> - } else {
>
>
>
>> + // It is not a long name so trim the blanks at the end of the name.
>
>
>> + if (Name[Name.size() - 1] != '/') {
>
> + return Name.rtrim(' ');
>> + }
>>
>
> Probably drop the {} on a single line statement.
>
>
> And another from the original code (see below):
>
> - // It is not a long name so trim the blanks at the end of the name.
> - if (name[name.size() - 1] != '/') {
> - return name.rtrim(' ');
> - }
>
>
>
>> + }
>
>
>> + // It's a simple name.
>> + if (Name[Name.size() - 1] == '/')
>> + return Name.substr(0, Name.size() - 1);
>> + return Name;
>> +}
>> +
>> Expected<uint32_t> ArchiveMemberHeader::getSize() const {
>> uint32_t Ret;
>> if (llvm::StringRef(ArMemHdr->Size,
>> @@ -166,7 +282,14 @@ Archive::Child::Child(const Archive *Par
>>
>> uint64_t Size = Header.getSizeOf();
>> Data = StringRef(Start, Size);
>> - if (!isThinMember()) {
>> + Expected<bool> isThinOrErr = isThinMember();
>> + if (!isThinOrErr) {
>> + if (Err)
>> + *Err = isThinOrErr.takeError();
>> + return;
>> + }
>> + bool isThin = isThinOrErr.get();
>> + if (!isThin) {
>> Expected<uint64_t> MemberSize = getRawSize();
>> if (!MemberSize) {
>> if (Err)
>> @@ -180,11 +303,30 @@ Archive::Child::Child(const Archive *Par
>> // Setup StartOfFile and PaddingBytes.
>> StartOfFile = Header.getSizeOf();
>> // Don't include attached name.
>> - StringRef Name = getRawName();
>> + Expected<StringRef> NameOrErr = getRawName();
>> + if (!NameOrErr){
>> + if (Err)
>> + *Err = NameOrErr.takeError();
>> + return;
>> + }
>> + StringRef Name = NameOrErr.get();
>> if (Name.startswith("#1/")) {
>> uint64_t NameSize;
>> - if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize))
>> - llvm_unreachable("Long name length is not an integer");
>> + if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize)) {
>> + if (Err) {
>> + std::string Buf;
>> + raw_string_ostream OS(Buf);
>> + OS.write_escaped(Name.substr(3).rtrim(' '),
>> + sizeof(Name.substr(3)).rtrim('
>> '));
>> + OS.flush();
>> + uint64_t Offset = Start - Parent->getData().data();
>> + *Err = malformedError("long name length characters after the #1/
>> are "
>> + "not all decimal numbers: '" + Buf + "'
>> for "
>> + "archive member header at offset " +
>> + Twine(Offset));
>> + return;
>>
>
> Is this return meant to be outside the conditional like the other similar
> cases you've added? (ie: the 'if (Name... )' is presumably the failure test
> - then conditionally populate the Err if it was passed, then exit
> regardless)
>
>
> I can see that. But I have really no idea where this is called when the
> Err is a nullptr and what it expects when there is an error. In the old
> code it would have called llvm_unreachable() regardless if Err is a nullptr
> or not. Then would fall through (when not compiled for DEBUG) to the line
> of code below to increment StartOfFile with then uninitialized value of
> NameSize. So I was trying to leave that logical path unchanged as it was
> as I don’t understand how it is used by its callers.
>
> I just spoke to Lang Hames on this and his thoughts are that constructor
> for Archive::Child::Child() should have an assert at the top such that
> unless Parent an Start are both nullptr then Err can’t be nullptr. Then
> the code above, and throughout the Archive::Child::Child() constructor can
> assume Err is not nullptr and such tests are removed. What do you and
> others think about that approach?
>
I don't know enough about the API here and why !Parent && !Start are
relevant properties with regard to error handling. If you two reckon that's
relevant, sure - check that property. Especially if it allows you to check
that property and bail out early, then provide a stronger invariant for the
rest of the code.
(don't get me wrong - happy to chat about the design further/don't mean to
be dismissive, just that I don't have enough context myself right now &
realize explaining it to me might not be worthwhile if you two have it
covered.
- Dave
>
>
>
>> + }
>> + }
>> StartOfFile += NameSize;
>> }
>> }
>> @@ -203,16 +345,22 @@ Expected<uint64_t> Archive::Child::getRa
>> return Header.getSize();
>> }
>>
>> -bool Archive::Child::isThinMember() const {
>> - StringRef Name = Header.getName();
>> +Expected<bool> Archive::Child::isThinMember() const {
>> + Expected<StringRef> NameOrErr = Header.getRawName();
>> + if (!NameOrErr)
>> + return NameOrErr.takeError();
>> + StringRef Name = NameOrErr.get();
>> return Parent->IsThin && Name != "/" && Name != "//";
>> }
>>
>> ErrorOr<std::string> Archive::Child::getFullName() const {
>> - assert(isThinMember());
>> - ErrorOr<StringRef> NameOrErr = getName();
>> - if (std::error_code EC = NameOrErr.getError())
>> - return EC;
>> + Expected<bool> isThin = isThinMember();
>> + if (!isThin)
>> + return errorToErrorCode(isThin.takeError());
>> + assert(isThin.get());
>
> + Expected<StringRef> NameOrErr = getName();
>> + if (!NameOrErr)
>> + return errorToErrorCode(NameOrErr.takeError());
>> StringRef Name = *NameOrErr;
>> if (sys::path::is_absolute(Name))
>> return Name;
>> @@ -224,7 +372,11 @@ ErrorOr<std::string> Archive::Child::get
>> }
>>
>> ErrorOr<StringRef> Archive::Child::getBuffer() const {
>> - if (!isThinMember()) {
>> + Expected<bool> isThinOrErr = isThinMember();
>> + if (!isThinOrErr)
>> + return errorToErrorCode(isThinOrErr.takeError());
>> + bool isThin = isThinOrErr.get();
>> + if (!isThin) {
>> Expected<uint32_t> Size = getSize();
>> if (!Size)
>> return errorToErrorCode(Size.takeError());
>> @@ -257,8 +409,9 @@ Expected<Archive::Child> Archive::Child:
>> if (NextLoc > Parent->Data.getBufferEnd()) {
>> Twine Msg("offset to next archive member past the end of the archive
>> after "
>> "member ");
>> - ErrorOr<StringRef> NameOrErr = getName();
>> - if (NameOrErr.getError()) {
>> + Expected<StringRef> NameOrErr = getName();
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> uint64_t Offset = Data.data() - Parent->getData().data();
>> return malformedError(Msg + "at offset " + Twine(Offset));
>> } else
>> @@ -279,64 +432,34 @@ uint64_t Archive::Child::getChildOffset(
>> return offset;
>> }
>>
>> -ErrorOr<StringRef> Archive::Child::getName() const {
>> - StringRef name = getRawName();
>> - // Check if it's a special name.
>> - if (name[0] == '/') {
>> - if (name.size() == 1) // Linker member.
>> - return name;
>> - if (name.size() == 2 && name[1] == '/') // String table.
>> - return name;
>> - // It's a long name.
>> - // Get the offset.
>> - std::size_t offset;
>> - if (name.substr(1).rtrim(' ').getAsInteger(10, offset))
>> - llvm_unreachable("Long name offset is not an integer");
>> -
>> - // Verify it.
>> - if (offset >= Parent->StringTable.size())
>> - return object_error::parse_failed;
>> - const char *addr = Parent->StringTable.begin() + offset;
>> -
>> - // GNU long file names end with a "/\n".
>> - if (Parent->kind() == K_GNU || Parent->kind() == K_MIPS64) {
>> - StringRef::size_type End = StringRef(addr).find('\n');
>> - return StringRef(addr, End - 1);
>> - }
>> - return StringRef(addr);
>> - } else if (name.startswith("#1/")) {
>> - uint64_t name_size;
>> - if (name.substr(3).rtrim(' ').getAsInteger(10, name_size))
>> - llvm_unreachable("Long name length is not an ingeter");
>> - return Data.substr(Header.getSizeOf(), name_size).rtrim('\0');
>> - } else {
>> - // It is not a long name so trim the blanks at the end of the name.
>> - if (name[name.size() - 1] != '/') {
>> - return name.rtrim(' ');
>> - }
>> - }
>> - // It's a simple name.
>> - if (name[name.size() - 1] == '/')
>> - return name.substr(0, name.size() - 1);
>> - return name;
>> +Expected<StringRef> Archive::Child::getName() const {
>> + Expected<uint64_t> RawSizeOrErr = getRawSize();
>> + if (!RawSizeOrErr)
>> + return RawSizeOrErr.takeError();
>> + uint64_t RawSize = RawSizeOrErr.get();
>> + Expected<StringRef> NameOrErr = Header.getName(Header.getSizeOf() +
>> RawSize);
>> + if (!NameOrErr)
>> + return NameOrErr.takeError();
>> + StringRef Name = NameOrErr.get();
>> + return Name;
>> }
>>
>> -ErrorOr<MemoryBufferRef> Archive::Child::getMemoryBufferRef() const {
>> - ErrorOr<StringRef> NameOrErr = getName();
>> - if (std::error_code EC = NameOrErr.getError())
>> - return EC;
>> +Expected<MemoryBufferRef> Archive::Child::getMemoryBufferRef() const {
>> + Expected<StringRef> NameOrErr = getName();
>> + if (!NameOrErr)
>> + return NameOrErr.takeError();
>> StringRef Name = NameOrErr.get();
>> ErrorOr<StringRef> Buf = getBuffer();
>> if (std::error_code EC = Buf.getError())
>> - return EC;
>> + return errorCodeToError(EC);
>> return MemoryBufferRef(*Buf, Name);
>> }
>>
>> Expected<std::unique_ptr<Binary>>
>> Archive::Child::getAsBinary(LLVMContext *Context) const {
>> - ErrorOr<MemoryBufferRef> BuffOrErr = getMemoryBufferRef();
>> - if (std::error_code EC = BuffOrErr.getError())
>> - return errorCodeToError(EC);
>> + Expected<MemoryBufferRef> BuffOrErr = getMemoryBufferRef();
>> + if (!BuffOrErr)
>> + return BuffOrErr.takeError();
>>
>> auto BinaryOrErr = createBinary(BuffOrErr.get(), Context);
>> if (BinaryOrErr)
>> @@ -372,17 +495,20 @@ Archive::Archive(MemoryBufferRef Source,
>> return;
>> }
>>
>> + // Make sure Format is initialized before any call to
>> + // ArchiveMemberHeader::getName() is made. This could be a valid empty
>> + // archive which is the same in all formats. So claiming it to be gnu
>> to is
>> + // fine if not totally correct before we look for a string table or
>> table of
>> + // contents.
>> + Format = K_GNU;
>> +
>> // Get the special members.
>> child_iterator I = child_begin(Err, false);
>> if (Err)
>> return;
>> child_iterator E = child_end();
>>
>> - // This is at least a valid empty archive. Since an empty archive is
>> the
>> - // same in all formats, just claim it to be gnu to make sure Format is
>> - // initialized.
>> - Format = K_GNU;
>> -
>> + // See if this is a valid empty archive and if so return.
>> if (I == E) {
>> Err = Error::success();
>> return;
>> @@ -397,7 +523,12 @@ Archive::Archive(MemoryBufferRef Source,
>> return false;
>> };
>>
>> - StringRef Name = C->getRawName();
>> + Expected<StringRef> NameOrErr = C->getRawName();
>> + if (!NameOrErr) {
>> + Err = NameOrErr.takeError();
>> + return;
>> + }
>> + StringRef Name = NameOrErr.get();
>>
>> // Below is the pattern that is used to figure out the archive format
>> // GNU archive format
>> @@ -437,9 +568,9 @@ Archive::Archive(MemoryBufferRef Source,
>> if (Name.startswith("#1/")) {
>> Format = K_BSD;
>> // We know this is BSD, so getName will work since there is no
>> string table.
>> - ErrorOr<StringRef> NameOrErr = C->getName();
>> - if (auto ec = NameOrErr.getError()) {
>> - Err = errorCodeToError(ec);
>> + Expected<StringRef> NameOrErr = C->getName();
>> + if (!NameOrErr) {
>> + Err = NameOrErr.takeError();
>> return;
>> }
>> Name = NameOrErr.get();
>> @@ -481,7 +612,12 @@ Archive::Archive(MemoryBufferRef Source,
>> Err = Error::success();
>> return;
>> }
>> - Name = C->getRawName();
>> + Expected<StringRef> NameOrErr = C->getRawName();
>> + if (!NameOrErr) {
>> + Err = NameOrErr.takeError();
>> + return;
>> + }
>> + Name = NameOrErr.get();
>> }
>>
>> if (Name == "//") {
>> @@ -522,7 +658,12 @@ Archive::Archive(MemoryBufferRef Source,
>> return;
>> }
>>
>> - Name = C->getRawName();
>> + NameOrErr = C->getRawName();
>> + if (!NameOrErr) {
>> + Err = NameOrErr.takeError();
>> + return;
>> + }
>> + Name = NameOrErr.get();
>>
>> if (Name == "//") {
>> // The string table is never an external member, so we just assert
>> on the
>>
>> Modified: llvm/trunk/lib/Object/ArchiveWriter.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/ArchiveWriter.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Object/ArchiveWriter.cpp (original)
>> +++ llvm/trunk/lib/Object/ArchiveWriter.cpp Fri Jul 29 12:44:13 2016
>> @@ -40,9 +40,9 @@ NewArchiveMember::NewArchiveMember(Memor
>> Expected<NewArchiveMember>
>> NewArchiveMember::getOldMember(const object::Archive::Child &OldMember,
>> bool Deterministic) {
>> - ErrorOr<llvm::MemoryBufferRef> BufOrErr =
>> OldMember.getMemoryBufferRef();
>> + Expected<llvm::MemoryBufferRef> BufOrErr =
>> OldMember.getMemoryBufferRef();
>> if (!BufOrErr)
>> - return errorCodeToError(BufOrErr.getError());
>> + return BufOrErr.takeError();
>>
>> NewArchiveMember M;
>> M.Buf = MemoryBuffer::getMemBuffer(*BufOrErr, false);
>>
>> Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a?rev=277177&view=auto
>>
>> ==============================================================================
>> --- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a (added)
>> +++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a Fri Jul 29
>> 12:44:13 2016
>> @@ -0,0 +1,13 @@
>> +!<arch>
>> +// 26 `
>> +1234567890123456hello.c/
>> +
>> +/507 0 0 0 644 102 `
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +int
>> +main()
>> +{
>> + printf("Hello World\n");
>> + return EXIT_SUCCESS;
>> +}
>>
>> Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a?rev=277177&view=auto
>>
>> ==============================================================================
>> Binary file - no diff available.
>>
>> Propchange: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a
>>
>> ------------------------------------------------------------------------------
>> svn:mime-type = application/octet-stream
>>
>> Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a?rev=277177&view=auto
>>
>> ==============================================================================
>> --- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a (added)
>> +++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a Fri Jul 29
>> 12:44:13 2016
>> @@ -0,0 +1,10 @@
>> +!<arch>
>> +#1/@123$ 1469564779 124 0 100644 102 `
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +int
>> +main()
>> +{
>> + printf("Hello World\n");
>> + return EXIT_SUCCESS;
>> +}
>>
>> Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a?rev=277177&view=auto
>>
>> ==============================================================================
>> --- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a (added)
>> +++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a Fri Jul 29
>> 12:44:13 2016
>> @@ -0,0 +1,13 @@
>> +!<arch>
>> +foo.c 1444941645 124 0 100644 17 `
>> +void foo(void){}
>> +
>> +#1/1234 1469564779 124 0 100644 126 `
>> +1234567890123456Xhello.c#include <stdio.h>
>> +#include <stdlib.h>
>> +int
>> +main()
>> +{
>> + printf("Hello World\n");
>> + return EXIT_SUCCESS;
>> +}
>>
>> Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a?rev=277177&view=auto
>>
>> ==============================================================================
>> --- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a (added)
>> +++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a Fri Jul 29
>> 12:44:13 2016
>> @@ -0,0 +1,13 @@
>> +!<arch>
>> +// 26 `
>> +1234567890123456hello.c/
>> +
>> +/&a25* 0 0 0 644 102 `
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +int
>> +main()
>> +{
>> + printf("Hello World\n");
>> + return EXIT_SUCCESS;
>> +}
>>
>> Modified: llvm/trunk/test/tools/llvm-objdump/malformed-archives.test
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/malformed-archives.test?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/tools/llvm-objdump/malformed-archives.test (original)
>> +++ llvm/trunk/test/tools/llvm-objdump/malformed-archives.test Fri Jul 29
>> 12:44:13 2016
>> @@ -23,10 +23,38 @@
>> # RUN: %p/Inputs/libbogus4.a \
>> # RUN: 2>&1 | FileCheck -check-prefix=bogus4 %s
>>
>> -# bogus4: libbogus4.a': truncated or malformed archive (remaining size
>> of archive too small for next archive member header at offset 170)
>> +# bogus4: libbogus4.a': truncated or malformed archive (remaining size
>> of archive too small for next archive member header for foo.c)
>>
>> # RUN: not llvm-objdump -macho -archive-headers \
>> # RUN: %p/Inputs/libbogus5.a \
>> # RUN: 2>&1 | FileCheck -check-prefix=bogus5 %s
>>
>> -# bogus5: libbogus5.a': truncated or malformed archive (terminator
>> characters in archive member "@\n" not the correct "`\n" values for the
>> archive member header at offset 8)
>> +# bogus5: libbogus5.a': truncated or malformed archive (terminator
>> characters in archive member "@\n" not the correct "`\n" values for the
>> archive member header for hello.c)
>> +
>> +# RUN: not llvm-objdump -macho -archive-headers \
>> +# RUN: %p/Inputs/libbogus6.a \
>> +# RUN: 2>&1 | FileCheck -check-prefix=bogus6 %s
>> +
>> +# bogus6: libbogus6.a': truncated or malformed archive (name contains a
>> leading space for archive member header at offset 96)
>> +
>> +# RUN: not llvm-objdump -macho -archive-headers \
>> +# RUN: %p/Inputs/libbogus7.a \
>> +# RUN: 2>&1 | FileCheck -check-prefix=bogus7 %s
>> +
>> +# bogus7: libbogus7.a': truncated or malformed archive (long name length
>> characters after the #1/ are not all decimal numbers: '@123$' for archive
>> member header at offset 8)
>> +
>> +# RUN: not llvm-objdump -macho -archive-headers \
>> +# RUN: %p/Inputs/libbogus8.a \
>> +# RUN: 2>&1 | FileCheck -check-prefix=bogus8 %s
>> +
>> +# bogus8: libbogus8.a(???) truncated or malformed archive (long name
>> length: 1234 extends past the end of the member or archive for archive
>> member header at offset 86)
>> +
>> +# RUN: not llvm-objdump -s %p/Inputs/libbogus9.a \
>> +# RUN: 2>&1 | FileCheck -check-prefix=bogus9 %s
>> +
>> +# bogus9: libbogus9.a(???) truncated or malformed archive (long name
>> offset characters after the '/' are not all decimal numbers: '&a25*' for
>> archive member header at offset 94)
>> +
>> +# RUN: not llvm-objdump -s %p/Inputs/libbogus10.a \
>> +# RUN: 2>&1 | FileCheck -check-prefix=bogus10 %s
>> +
>> +# bogus10: libbogus10.a(???) truncated or malformed archive (long name
>> offset 507 past the end of the string table for archive member header at
>> offset 94)
>>
>> Modified: llvm/trunk/tools/dsymutil/BinaryHolder.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/BinaryHolder.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/dsymutil/BinaryHolder.cpp (original)
>> +++ llvm/trunk/tools/dsymutil/BinaryHolder.cpp Fri Jul 29 12:44:13 2016
>> @@ -115,8 +115,8 @@ BinaryHolder::GetArchiveMemberBuffers(St
>> if (Verbose)
>> outs() << "\tfound member in current archive.\n";
>> auto ErrOrMem = Child.getMemoryBufferRef();
>> - if (auto Err = ErrOrMem.getError())
>> - return Err;
>> + if (!ErrOrMem)
>> + return errorToErrorCode(ErrOrMem.takeError());
>> Buffers.push_back(*ErrOrMem);
>> }
>> }
>>
>> 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=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-ar/llvm-ar.cpp (original)
>> +++ llvm/trunk/tools/llvm-ar/llvm-ar.cpp Fri Jul 29 12:44:13 2016
>> @@ -409,8 +409,8 @@ static void performReadOperation(Archive
>> {
>> Error Err;
>> for (auto &C : OldArchive->children(Err)) {
>> - ErrorOr<StringRef> NameOrErr = C.getName();
>> - failIfError(NameOrErr.getError());
>> + Expected<StringRef> NameOrErr = C.getName();
>> + failIfError(NameOrErr.takeError());
>> StringRef Name = NameOrErr.get();
>>
>> if (Filter) {
>> @@ -537,8 +537,8 @@ computeNewArchiveMembers(ArchiveOperatio
>> Error Err;
>> for (auto &Child : OldArchive->children(Err)) {
>> int Pos = Ret.size();
>> - ErrorOr<StringRef> NameOrErr = Child.getName();
>> - failIfError(NameOrErr.getError());
>> + Expected<StringRef> NameOrErr = Child.getName();
>> + failIfError(NameOrErr.takeError());
>> StringRef Name = NameOrErr.get();
>> if (Name == PosName) {
>> assert(AddAfter || AddBefore);
>>
>> Modified: llvm/trunk/tools/llvm-nm/llvm-nm.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-nm/llvm-nm.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-nm/llvm-nm.cpp (original)
>> +++ llvm/trunk/tools/llvm-nm/llvm-nm.cpp Fri Jul 29 12:44:13 2016
>> @@ -198,13 +198,14 @@ static void error(llvm::Error E, StringR
>> HadError = true;
>> errs() << ToolName << ": " << FileName;
>>
>> - ErrorOr<StringRef> NameOrErr = C.getName();
>> + Expected<StringRef> NameOrErr = C.getName();
>> // TODO: if we have a error getting the name then it would be nice to
>> print
>> // the index of which archive member this is and or its offset in the
>> // archive instead of "???" as the name.
>> - if (NameOrErr.getError())
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> errs() << "(" << "???" << ")";
>> - else
>> + } else
>> errs() << "(" << NameOrErr.get() << ")";
>>
>> if (!ArchitectureName.empty())
>> @@ -1099,9 +1100,11 @@ static void dumpSymbolNamesFromFile(std:
>> ErrorOr<Archive::Child> C = I->getMember();
>> if (error(C.getError()))
>> return;
>> - ErrorOr<StringRef> FileNameOrErr = C->getName();
>> - if (error(FileNameOrErr.getError()))
>> + Expected<StringRef> FileNameOrErr = C->getName();
>> + if (!FileNameOrErr) {
>> + error(FileNameOrErr.takeError(), Filename);
>> return;
>> + }
>> StringRef SymName = I->getName();
>> outs() << SymName << " in " << FileNameOrErr.get() << "\n";
>> }
>>
>> Modified: llvm/trunk/tools/llvm-objdump/MachODump.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/MachODump.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-objdump/MachODump.cpp (original)
>> +++ llvm/trunk/tools/llvm-objdump/MachODump.cpp Fri Jul 29 12:44:13 2016
>> @@ -1521,16 +1521,23 @@ static void printArchiveChild(StringRef
>> }
>>
>> if (verbose) {
>> - ErrorOr<StringRef> NameOrErr = C.getName();
>> - if (NameOrErr.getError()) {
>> - StringRef RawName = C.getRawName();
>> + Expected<StringRef> NameOrErr = C.getName();
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> + Expected<StringRef> NameOrErr = C.getRawName();
>> + if (!NameOrErr)
>> + report_error(Filename, C, NameOrErr.takeError(),
>> ArchitectureName);
>> + StringRef RawName = NameOrErr.get();
>> outs() << RawName << "\n";
>> } else {
>> StringRef Name = NameOrErr.get();
>> outs() << Name << "\n";
>> }
>> } else {
>> - StringRef RawName = C.getRawName();
>> + Expected<StringRef> NameOrErr = C.getRawName();
>> + if (!NameOrErr)
>> + report_error(Filename, C, NameOrErr.takeError(), ArchitectureName);
>> + StringRef RawName = NameOrErr.get();
>> outs() << RawName << "\n";
>> }
>> }
>>
>> Modified: llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp (original)
>> +++ llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp Fri Jul 29 12:44:13
>> 2016
>> @@ -312,13 +312,14 @@ LLVM_ATTRIBUTE_NORETURN void llvm::repor
>> const
>> object::Archive::Child &C,
>> llvm::Error E,
>> StringRef
>> ArchitectureName) {
>> - ErrorOr<StringRef> NameOrErr = C.getName();
>> + Expected<StringRef> NameOrErr = C.getName();
>> // TODO: if we have a error getting the name then it would be nice to
>> print
>> // the index of which archive member this is and or its offset in the
>> // archive instead of "???" as the name.
>> - if (NameOrErr.getError())
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> llvm::report_error(ArchiveName, "???", std::move(E),
>> ArchitectureName);
>> - else
>> + } else
>> llvm::report_error(ArchiveName, NameOrErr.get(), std::move(E),
>> ArchitectureName);
>> }
>>
>> Modified: llvm/trunk/tools/llvm-size/llvm-size.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-size/llvm-size.cpp?rev=277177&r1=277176&r2=277177&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-size/llvm-size.cpp (original)
>> +++ llvm/trunk/tools/llvm-size/llvm-size.cpp Fri Jul 29 12:44:13 2016
>> @@ -114,13 +114,14 @@ static void error(llvm::Error E, StringR
>> HadError = true;
>> errs() << ToolName << ": " << FileName;
>>
>> - ErrorOr<StringRef> NameOrErr = C.getName();
>> + Expected<StringRef> NameOrErr = C.getName();
>> // TODO: if we have a error getting the name then it would be nice to
>> print
>> // the index of which archive member this is and or its offset in the
>> // archive instead of "???" as the name.
>> - if (NameOrErr.getError())
>> + if (!NameOrErr) {
>> + consumeError(NameOrErr.takeError());
>> errs() << "(" << "???" << ")";
>> - else
>> + } else
>> errs() << "(" << NameOrErr.get() << ")";
>>
>> if (!ArchitectureName.empty())
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://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/20160801/d533b73d/attachment.html>
More information about the llvm-commits
mailing list