<div dir="ltr">Hi David,<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.800000190734863px">I don't know enough about the API here and why !Parent && !Start are relevant properties with regard to error handling.</span></blockquote><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">Short version: Archive::ArchiveMemberHeader can either point at a header (which may be malformed), or it can be set to a sentinel value by pointing it at null. In the latter case Error is permitted to be null, since no error could occur. So the invariant is "If I'm pointed at real data, there must be a non-null Error pointer available to report malformed data on".</span></div><div><br></div><div><span style="font-size:12.800000190734863px">- Lang. </span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 1, 2016 at 3:00 PM, David Blaikie via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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:<br>* 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)<br>* 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)<br><br>As for the "this was the way it was" - fair point. I just committed those myself in r277394<br><br>And as for the final point/discussion... *skip ahead to the end*<br><br><div class="gmail_quote"><div><div class="h5"><div dir="ltr">On Mon, Aug 1, 2016 at 2:45 PM Kevin Enderby <<a href="mailto:enderby@apple.com" target="_blank">enderby@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi David,<div><br></div><div>Thanks for your time to take a look at these changes and your thoughtful comments. There were some fixes to this in <span style="font-family:'Helvetica Neue'">r277223.</span></div><div><font face="Helvetica Neue"><br></font></div><div><font face="Helvetica Neue">I’ve added my thoughts and comments below.</font></div><div><br></div><div><div><blockquote type="cite"></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>On Aug 1, 2016, at 10:32 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016 at 10:51 AM Kevin Enderby via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: enderby<br>
Date: Fri Jul 29 12:44:13 2016<br>
New Revision: 277177<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=277177&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=277177&view=rev</a><br>
Log:<br>
The next step along the way to getting good error messages for bad archives.<br>
<br>
As mentioned in commit log for r276686 this next step is adding a new<br>
method in the ArchiveMemberHeader class to get the full name that<br>
does proper error checking, and can be use for error messages.<br>
<br>
To do this the name of ArchiveMemberHeader::getName() is changed to<br>
ArchiveMemberHeader::getRawName() to be consistent with<br>
Archive::Child::getRawName(). Then the “new” method is the addition<br>
of a new implementation of ArchiveMemberHeader::getName() which gets<br>
the full name and provides proper error checking. Which is mostly a rewrite<br>
of what was Archive::Child::getName() and cleaning up incorrect uses of<br>
llvm_unreachable() in the code which were actually just cases of errors<br>
in the input Archives.<br>
<br>
Then Archive::Child::getName() is changed to return Expected<> and use<br>
the new implementation of ArchiveMemberHeader::getName() .<br>
<br>
Also needed to change Archive::getMemoryBufferRef() with these<br>
changes to return Expected<> as well to propagate Errors up.<br>
As well as changing Archive::isThinMember() to return Expected<> .<br>
<br>
Added:<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a (with props)<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a<br>
Modified:<br>
llvm/trunk/include/llvm/Object/Archive.h<br>
llvm/trunk/lib/Object/Archive.cpp<br>
llvm/trunk/lib/Object/ArchiveWriter.cpp<br>
llvm/trunk/test/tools/llvm-objdump/malformed-archives.test<br>
llvm/trunk/tools/dsymutil/BinaryHolder.cpp<br>
llvm/trunk/tools/llvm-ar/llvm-ar.cpp<br>
llvm/trunk/tools/llvm-nm/llvm-nm.cpp<br>
llvm/trunk/tools/llvm-objdump/MachODump.cpp<br>
llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
llvm/trunk/tools/llvm-size/llvm-size.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Object/Archive.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Archive.h?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Archive.h?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Object/Archive.h (original)<br>
+++ llvm/trunk/include/llvm/Object/Archive.h Fri Jul 29 12:44:13 2016<br>
@@ -36,7 +36,10 @@ public:<br>
// ArchiveMemberHeader() = default;<br>
<br>
/// Get the name without looking up long names.<br>
- llvm::StringRef getName() const;<br>
+ Expected<llvm::StringRef> getRawName() const;<br>
+<br>
+ /// Get the name looking up long names.<br>
+ Expected<llvm::StringRef> getName(uint64_t Size) const;<br>
<br>
/// Members are not larger than 4GB.<br>
Expected<uint32_t> getSize() const;<br>
@@ -82,7 +85,7 @@ public:<br>
/// \brief Offset from Data to the start of the file.<br>
uint16_t StartOfFile;<br>
<br>
- bool isThinMember() const;<br>
+ Expected<bool> isThinMember() const;<br>
<br>
public:<br>
Child(const Archive *Parent, const char *Start, Error *Err);<br>
@@ -96,9 +99,9 @@ public:<br>
const Archive *getParent() const { return Parent; }<br>
Expected<Child> getNext() const;<br>
<br>
- ErrorOr<StringRef> getName() const;<br>
+ Expected<StringRef> getName() const;<br>
ErrorOr<std::string> getFullName() const;<br>
- StringRef getRawName() const { return Header.getName(); }<br>
+ Expected<StringRef> getRawName() const { return Header.getRawName(); }<br>
sys::TimeValue getLastModified() const {<br>
return Header.getLastModified();<br>
}<br>
@@ -118,7 +121,7 @@ public:<br>
ErrorOr<StringRef> getBuffer() const;<br>
uint64_t getChildOffset() const;<br>
<br>
- ErrorOr<MemoryBufferRef> getMemoryBufferRef() const;<br>
+ Expected<MemoryBufferRef> getMemoryBufferRef() const;<br>
<br>
Expected<std::unique_ptr<Binary>><br>
getAsBinary(LLVMContext *Context = nullptr) const;<br>
@@ -238,6 +241,7 @@ public:<br>
<br>
bool hasSymbolTable() const;<br>
StringRef getSymbolTable() const { return SymbolTable; }<br>
+ StringRef getStringTable() const { return StringTable; }<br>
uint32_t getNumberOfSymbols() const;<br>
<br>
std::vector<std::unique_ptr<MemoryBuffer>> takeThinBuffers() {<br>
<br>
Modified: llvm/trunk/lib/Object/Archive.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Archive.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Archive.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Object/Archive.cpp (original)<br>
+++ llvm/trunk/lib/Object/Archive.cpp Fri Jul 29 12:44:13 2016<br>
@@ -43,17 +43,17 @@ ArchiveMemberHeader::ArchiveMemberHeader<br>
return;<br>
ErrorAsOutParameter ErrAsOutParam(Err);<br>
<br>
- // TODO: For errors messages with the ArchiveMemberHeader class use the<br>
- // archive member name instead of the the offset to the archive member header.<br>
- // When there is also error getting the member name then use the offset to<br>
- // the member in the message.<br>
-<br>
if (Size < sizeof(ArMemHdrType)) {<br>
if (Err) {<br>
- uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
- *Err = malformedError("remaining size of archive too small for next "<br>
- "archive member header at offset " +<br>
- Twine(Offset));<br>
+ Twine Msg("remaining size of archive too small for next archive member "<br>
+ "header ");<br>
+ Expected<StringRef> NameOrErr = getName(Size);<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
+ uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
+ *Err = malformedError(Msg + "at offset " + Twine(Offset));<br>
+ } else<br>
+ *Err = malformedError(Msg + "for " + Twine(NameOrErr.get()));<br></blockquote><div><br></div></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div>If you like, this code might be a bit tidier by rolling in the initialization into the conditional:<br><br>if (Expected<StringRef> NameOrErr = getName(Size))<br> *Err = malformedError(...)<br>else {<br> consumeError(...)<br></div></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> …</div></div></div></div></blockquote><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div>}<br></div></div></div></div></blockquote><div><br></div>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.</div><div></div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div><br>Also, not sure, but this code might be a bit easier less indented:<br><br>if (Size < sizeof....) {<br> if (!Err)<br> return;<br> /* populate *Err */<br> return;<br>}<br><br>It's the same number of lines, but reduces indentation (but does duplicate the return which might make it harder to read - not sure)<br></div></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div>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.</div><div></div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div>Also "Twine(NameOrErr.get())" doesn't need the Twine() wrapper</div></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div>Yep. And that was fixed with the follow on changes in <span style="font-family:'Helvetica Neue'">r277223.</span></div><div></div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> 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”.<br></div></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div>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.</div><div></div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div><br>(all points similarly below)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
return;<br>
}<br>
@@ -64,18 +64,35 @@ ArchiveMemberHeader::ArchiveMemberHeader<br>
OS.write_escaped(llvm::StringRef(ArMemHdr->Terminator,<br>
sizeof(ArMemHdr->Terminator)));<br>
OS.flush();<br>
- uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
- *Err = malformedError("terminator characters in archive member \"" + Buf +<br>
- "\" not the correct \"`\\n\" values for the "<br>
- "archive member header at offset " + Twine(Offset));<br>
+ Twine Msg("terminator characters in archive member \"" + Buf + "\" not "<br>
+ "the correct \"`\\n\" values for the archive member header ");<br>
+ Expected<StringRef> NameOrErr = getName(Size);<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
+ uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
+ *Err = malformedError(Msg + "at offset " + Twine(Offset));<br>
+ } else<br>
+ *Err = malformedError(Msg + "for " + Twine(NameOrErr.get()));<br>
}<br>
return;<br>
}<br>
}<br>
<br>
-StringRef ArchiveMemberHeader::getName() const {<br>
+// This gets the raw name from the ArMemHdr->Name field and checks that it is<br>
+// valid for the kind of archive. If it is not valid it returns an Error.<br>
+Expected<StringRef> ArchiveMemberHeader::getRawName() const {<br>
char EndCond;<br>
- if (ArMemHdr->Name[0] == '/' || ArMemHdr->Name[0] == '#')<br>
+ auto Kind = Parent->kind();<br>
+ if (Kind == Archive::K_BSD || Kind == Archive::K_DARWIN64) {<br>
+ if (ArMemHdr->Name[0] == ' ') {<br>
+ uint64_t Offset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("name contains a leading space for archive member "<br>
+ "header at offset " + Twine(Offset));<br>
+ }<br>
+ EndCond = ' ';<br>
+ }<br>
+ else if (ArMemHdr->Name[0] == '/' || ArMemHdr->Name[0] == '#')<br>
EndCond = ' ';<br>
else<br>
EndCond = '/';<br>
@@ -88,6 +105,105 @@ StringRef ArchiveMemberHeader::getName()<br>
return llvm::StringRef(ArMemHdr->Name, end);<br>
}<br>
<br>
+// This gets the name looking up long names. Size is the size of the archive<br>
+// member including the header, so the size of any name following the header<br>
+// is checked to make sure it does not overflow.<br>
+Expected<StringRef> ArchiveMemberHeader::getName(uint64_t Size) const {<br>
+<br>
+ // This can be called from the ArchiveMemberHeader constructor when the<br>
+ // archive header is truncated to produce an error message with the name.<br>
+ // Make sure the name field is not truncated.<br>
+ if (Size < offsetof(ArMemHdrType, Name) + sizeof(ArMemHdr->Name)) {<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("archive header truncated before the name field "<br>
+ "for archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+<br>
+ // The raw name itself can be invalid.<br>
+ Expected<StringRef> NameOrErr = getRawName();<br>
+ if (!NameOrErr)<br>
+ return NameOrErr.takeError();<br>
+ StringRef Name = NameOrErr.get();<br>
+<br>
+ // Check if it's a special name.<br>
+ if (Name[0] == '/') {<br>
+ if (Name.size() == 1) // Linker member.<br>
+ return Name;<br>
+ if (Name.size() == 2 && Name[1] == '/') // String table.<br>
+ return Name;<br>
+ // It's a long name.<br>
+ // Get the string table offset.<br>
+ std::size_t StringOffset;<br>
+ if (Name.substr(1).rtrim(' ').getAsInteger(10, StringOffset)) {<br>
+ std::string Buf;<br>
+ raw_string_ostream OS(Buf);<br>
+ OS.write_escaped(Name.substr(1).rtrim(' '),<br>
+ sizeof(Name.substr(1)).rtrim(' '));<br>
+ OS.flush();<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name offset characters after the '/' are "<br>
+ "not all decimal numbers: '" + Buf + "' for "<br>
+ "archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+<br>
+ // Verify it.<br>
+ if (StringOffset >= Parent->getStringTable().size()) {<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name offset " + Twine(StringOffset) + " past "<br>
+ "the end of the string table for archive member "<br>
+ "header at offset " + Twine(ArchiveOffset));<br>
+ }<br>
+ const char *addr = Parent->getStringTable().begin() + StringOffset;<br>
+<br>
+ // GNU long file names end with a "/\n".<br>
+ if (Parent->kind() == Archive::K_GNU ||<br>
+ Parent->kind() == Archive::K_MIPS64) {<br>
+ StringRef::size_type End = StringRef(addr).find('\n');<br>
+ return StringRef(addr, End - 1);<br>
+ }<br>
+ return StringRef(addr);<br></blockquote><div><br></div><div>"return addr;" should suffice here?</div></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div>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:</div><div><br></div><div>- return StringRef(addr);</div><div></div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ } else if (Name.startswith("#1/")) {<br></blockquote><div><br></div><div>Drop the else-after-return <a href="http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return" target="_blank">http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return</a></div></div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div>Sure I can make that change. But again the code in Archive::Child::getName() that was deleted (again see below) was exactly as above:</div></div></div><div style="word-wrap:break-word"><div><div><br></div><div>- } else if (name.startswith("#1/")) {</div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ uint64_t NameLength;</blockquote></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+ if (Name.substr(3).rtrim(' ').getAsInteger(10, NameLength)) {<br>
+ std::string Buf;<br>
+ raw_string_ostream OS(Buf);<br>
+ OS.write_escaped(Name.substr(3).rtrim(' '),<br>
+ sizeof(Name.substr(3)).rtrim(' '));<br>
+ OS.flush();<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name length characters after the #1/ are "<br>
+ "not all decimal numbers: '" + Buf + "' for "<br>
+ "archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+ if (getSizeOf() + NameLength > Size) {<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name length: " + Twine(NameLength) +<br>
+ " extends past the end of the member or archive "<br>
+ "for archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+ return StringRef(reinterpret_cast<const char *>(ArMemHdr) + getSizeOf(),<br>
+ NameLength).rtrim('\0');<br>
+ } else {<br></blockquote></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote><div><br>Another else-after-return<br></div></div></div></div></blockquote><div><br></div>Again from the original code (see below), just re-written:</div></div></div><div style="word-wrap:break-word"><div><div><br></div><div>- return Data.substr(Header.getSizeOf(), name_size).rtrim('\0');<br>- } else {</div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // It is not a long name so trim the blanks at the end of the name.</blockquote></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+ if (Name[Name.size() - 1] != '/') {</blockquote></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ return Name.rtrim(' ');<br>
+ }<br></blockquote><div><br>Probably drop the {} on a single line statement.<br></div></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"></div></div></div></blockquote><div><br></div>And another from the original code (see below):</div></div></div><div style="word-wrap:break-word"><div><div><br></div><div>- // It is not a long name so trim the blanks at the end of the name.<br>- if (name[name.size() - 1] != '/') {<br>- return name.rtrim(' ');<br>- }</div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }</blockquote></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+ // It's a simple name.<br>
+ if (Name[Name.size() - 1] == '/')<br>
+ return Name.substr(0, Name.size() - 1);<br>
+ return Name;<br>
+}<br>
+<br>
Expected<uint32_t> ArchiveMemberHeader::getSize() const {<br>
uint32_t Ret;<br>
if (llvm::StringRef(ArMemHdr->Size,<br>
@@ -166,7 +282,14 @@ Archive::Child::Child(const Archive *Par<br>
<br>
uint64_t Size = Header.getSizeOf();<br>
Data = StringRef(Start, Size);<br>
- if (!isThinMember()) {<br>
+ Expected<bool> isThinOrErr = isThinMember();<br>
+ if (!isThinOrErr) {<br>
+ if (Err)<br>
+ *Err = isThinOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ bool isThin = isThinOrErr.get();<br>
+ if (!isThin) {<br>
Expected<uint64_t> MemberSize = getRawSize();<br>
if (!MemberSize) {<br>
if (Err)<br>
@@ -180,11 +303,30 @@ Archive::Child::Child(const Archive *Par<br>
// Setup StartOfFile and PaddingBytes.<br>
StartOfFile = Header.getSizeOf();<br>
// Don't include attached name.<br>
- StringRef Name = getRawName();<br>
+ Expected<StringRef> NameOrErr = getRawName();<br>
+ if (!NameOrErr){<br>
+ if (Err)<br>
+ *Err = NameOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ StringRef Name = NameOrErr.get();<br>
if (Name.startswith("#1/")) {<br>
uint64_t NameSize;<br>
- if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize))<br>
- llvm_unreachable("Long name length is not an integer");<br>
+ if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize)) {<br>
+ if (Err) {<br>
+ std::string Buf;<br>
+ raw_string_ostream OS(Buf);<br>
+ OS.write_escaped(Name.substr(3).rtrim(' '),<br>
+ sizeof(Name.substr(3)).rtrim(' '));<br>
+ OS.flush();<br>
+ uint64_t Offset = Start - Parent->getData().data();<br>
+ *Err = malformedError("long name length characters after the #1/ are "<br>
+ "not all decimal numbers: '" + Buf + "' for "<br>
+ "archive member header at offset " +<br>
+ Twine(Offset));<br>
+ return;<br></blockquote></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div><br></div><div>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)</div></div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"></div></div></div></blockquote><div><br></div>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.</div><div><br></div><div>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?</div></div></div></blockquote></div></div><div><br>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.<br><br>(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.<br><br>- Dave<br> </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ }<br>
StartOfFile += NameSize;<br>
}<br>
}<br>
@@ -203,16 +345,22 @@ Expected<uint64_t> Archive::Child::getRa<br>
return Header.getSize();<br>
}<br>
<br>
-bool Archive::Child::isThinMember() const {<br>
- StringRef Name = Header.getName();<br>
+Expected<bool> Archive::Child::isThinMember() const {<br>
+ Expected<StringRef> NameOrErr = Header.getRawName();<br>
+ if (!NameOrErr)<br>
+ return NameOrErr.takeError();<br>
+ StringRef Name = NameOrErr.get();<br>
return Parent->IsThin && Name != "/" && Name != "//";<br>
}<br>
<br>
ErrorOr<std::string> Archive::Child::getFullName() const {<br>
- assert(isThinMember());<br>
- ErrorOr<StringRef> NameOrErr = getName();<br>
- if (std::error_code EC = NameOrErr.getError())<br>
- return EC;<br>
+ Expected<bool> isThin = isThinMember();<br>
+ if (!isThin)<br>
+ return errorToErrorCode(isThin.takeError());<br>
+ assert(isThin.get());<span style="line-height:1.5"> </span></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Expected<StringRef> NameOrErr = getName();<br>
+ if (!NameOrErr)<br>
+ return errorToErrorCode(NameOrErr.takeError());<br>
StringRef Name = *NameOrErr;<br>
if (sys::path::is_absolute(Name))<br>
return Name;<br>
@@ -224,7 +372,11 @@ ErrorOr<std::string> Archive::Child::get<br>
}<br>
<br>
ErrorOr<StringRef> Archive::Child::getBuffer() const {<br>
- if (!isThinMember()) {<br>
+ Expected<bool> isThinOrErr = isThinMember();<br>
+ if (!isThinOrErr)<br>
+ return errorToErrorCode(isThinOrErr.takeError());<br>
+ bool isThin = isThinOrErr.get();<br>
+ if (!isThin) {<br>
Expected<uint32_t> Size = getSize();<br>
if (!Size)<br>
return errorToErrorCode(Size.takeError());<br>
@@ -257,8 +409,9 @@ Expected<Archive::Child> Archive::Child:<br>
if (NextLoc > Parent->Data.getBufferEnd()) {<br>
Twine Msg("offset to next archive member past the end of the archive after "<br>
"member ");<br>
- ErrorOr<StringRef> NameOrErr = getName();<br>
- if (NameOrErr.getError()) {<br>
+ Expected<StringRef> NameOrErr = getName();<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
uint64_t Offset = Data.data() - Parent->getData().data();<br>
return malformedError(Msg + "at offset " + Twine(Offset));<br>
} else<br>
@@ -279,64 +432,34 @@ uint64_t Archive::Child::getChildOffset(<br>
return offset;<br>
}<br>
<br>
-ErrorOr<StringRef> Archive::Child::getName() const {<br>
- StringRef name = getRawName();<br>
- // Check if it's a special name.<br>
- if (name[0] == '/') {<br>
- if (name.size() == 1) // Linker member.<br>
- return name;<br>
- if (name.size() == 2 && name[1] == '/') // String table.<br>
- return name;<br>
- // It's a long name.<br>
- // Get the offset.<br>
- std::size_t offset;<br>
- if (name.substr(1).rtrim(' ').getAsInteger(10, offset))<br>
- llvm_unreachable("Long name offset is not an integer");<br>
-<br>
- // Verify it.<br>
- if (offset >= Parent->StringTable.size())<br>
- return object_error::parse_failed;<br>
- const char *addr = Parent->StringTable.begin() + offset;<br>
-<br>
- // GNU long file names end with a "/\n".<br>
- if (Parent->kind() == K_GNU || Parent->kind() == K_MIPS64) {<br>
- StringRef::size_type End = StringRef(addr).find('\n');<br>
- return StringRef(addr, End - 1);<br>
- }<br>
- return StringRef(addr);<br>
- } else if (name.startswith("#1/")) {<br>
- uint64_t name_size;<br>
- if (name.substr(3).rtrim(' ').getAsInteger(10, name_size))<br>
- llvm_unreachable("Long name length is not an ingeter");<br>
- return Data.substr(Header.getSizeOf(), name_size).rtrim('\0');<br>
- } else {<br>
- // It is not a long name so trim the blanks at the end of the name.<br>
- if (name[name.size() - 1] != '/') {<br>
- return name.rtrim(' ');<br>
- }<br>
- }<br>
- // It's a simple name.<br>
- if (name[name.size() - 1] == '/')<br>
- return name.substr(0, name.size() - 1);<br>
- return name;<br>
+Expected<StringRef> Archive::Child::getName() const {<br>
+ Expected<uint64_t> RawSizeOrErr = getRawSize();<br>
+ if (!RawSizeOrErr)<br>
+ return RawSizeOrErr.takeError();<br>
+ uint64_t RawSize = RawSizeOrErr.get();<br>
+ Expected<StringRef> NameOrErr = Header.getName(Header.getSizeOf() + RawSize);<br>
+ if (!NameOrErr)<br>
+ return NameOrErr.takeError();<br>
+ StringRef Name = NameOrErr.get();<br>
+ return Name;<br>
}<br>
<br>
-ErrorOr<MemoryBufferRef> Archive::Child::getMemoryBufferRef() const {<br>
- ErrorOr<StringRef> NameOrErr = getName();<br>
- if (std::error_code EC = NameOrErr.getError())<br>
- return EC;<br>
+Expected<MemoryBufferRef> Archive::Child::getMemoryBufferRef() const {<br>
+ Expected<StringRef> NameOrErr = getName();<br>
+ if (!NameOrErr)<br>
+ return NameOrErr.takeError();<br>
StringRef Name = NameOrErr.get();<br>
ErrorOr<StringRef> Buf = getBuffer();<br>
if (std::error_code EC = Buf.getError())<br>
- return EC;<br>
+ return errorCodeToError(EC);<br>
return MemoryBufferRef(*Buf, Name);<br>
}<br>
<br>
Expected<std::unique_ptr<Binary>><br>
Archive::Child::getAsBinary(LLVMContext *Context) const {<br>
- ErrorOr<MemoryBufferRef> BuffOrErr = getMemoryBufferRef();<br>
- if (std::error_code EC = BuffOrErr.getError())<br>
- return errorCodeToError(EC);<br>
+ Expected<MemoryBufferRef> BuffOrErr = getMemoryBufferRef();<br>
+ if (!BuffOrErr)<br>
+ return BuffOrErr.takeError();<br>
<br>
auto BinaryOrErr = createBinary(BuffOrErr.get(), Context);<br>
if (BinaryOrErr)<br>
@@ -372,17 +495,20 @@ Archive::Archive(MemoryBufferRef Source,<br>
return;<br>
}<br>
<br>
+ // Make sure Format is initialized before any call to<br>
+ // ArchiveMemberHeader::getName() is made. This could be a valid empty<br>
+ // archive which is the same in all formats. So claiming it to be gnu to is<br>
+ // fine if not totally correct before we look for a string table or table of<br>
+ // contents.<br>
+ Format = K_GNU;<br>
+<br>
// Get the special members.<br>
child_iterator I = child_begin(Err, false);<br>
if (Err)<br>
return;<br>
child_iterator E = child_end();<br>
<br>
- // This is at least a valid empty archive. Since an empty archive is the<br>
- // same in all formats, just claim it to be gnu to make sure Format is<br>
- // initialized.<br>
- Format = K_GNU;<br>
-<br>
+ // See if this is a valid empty archive and if so return.<br>
if (I == E) {<br>
Err = Error::success();<br>
return;<br>
@@ -397,7 +523,12 @@ Archive::Archive(MemoryBufferRef Source,<br>
return false;<br>
};<br>
<br>
- StringRef Name = C->getRawName();<br>
+ Expected<StringRef> NameOrErr = C->getRawName();<br>
+ if (!NameOrErr) {<br>
+ Err = NameOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ StringRef Name = NameOrErr.get();<br>
<br>
// Below is the pattern that is used to figure out the archive format<br>
// GNU archive format<br>
@@ -437,9 +568,9 @@ Archive::Archive(MemoryBufferRef Source,<br>
if (Name.startswith("#1/")) {<br>
Format = K_BSD;<br>
// We know this is BSD, so getName will work since there is no string table.<br>
- ErrorOr<StringRef> NameOrErr = C->getName();<br>
- if (auto ec = NameOrErr.getError()) {<br>
- Err = errorCodeToError(ec);<br>
+ Expected<StringRef> NameOrErr = C->getName();<br>
+ if (!NameOrErr) {<br>
+ Err = NameOrErr.takeError();<br>
return;<br>
}<br>
Name = NameOrErr.get();<br>
@@ -481,7 +612,12 @@ Archive::Archive(MemoryBufferRef Source,<br>
Err = Error::success();<br>
return;<br>
}<br>
- Name = C->getRawName();<br>
+ Expected<StringRef> NameOrErr = C->getRawName();<br>
+ if (!NameOrErr) {<br>
+ Err = NameOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ Name = NameOrErr.get();<br>
}<br>
<br>
if (Name == "//") {<br>
@@ -522,7 +658,12 @@ Archive::Archive(MemoryBufferRef Source,<br>
return;<br>
}<br>
<br>
- Name = C->getRawName();<br>
+ NameOrErr = C->getRawName();<br>
+ if (!NameOrErr) {<br>
+ Err = NameOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ Name = NameOrErr.get();<br>
<br>
if (Name == "//") {<br>
// The string table is never an external member, so we just assert on the<br>
<br>
Modified: llvm/trunk/lib/Object/ArchiveWriter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/ArchiveWriter.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/ArchiveWriter.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Object/ArchiveWriter.cpp (original)<br>
+++ llvm/trunk/lib/Object/ArchiveWriter.cpp Fri Jul 29 12:44:13 2016<br>
@@ -40,9 +40,9 @@ NewArchiveMember::NewArchiveMember(Memor<br>
Expected<NewArchiveMember><br>
NewArchiveMember::getOldMember(const object::Archive::Child &OldMember,<br>
bool Deterministic) {<br>
- ErrorOr<llvm::MemoryBufferRef> BufOrErr = OldMember.getMemoryBufferRef();<br>
+ Expected<llvm::MemoryBufferRef> BufOrErr = OldMember.getMemoryBufferRef();<br>
if (!BufOrErr)<br>
- return errorCodeToError(BufOrErr.getError());<br>
+ return BufOrErr.takeError();<br>
<br>
NewArchiveMember M;<br>
M.Buf = MemoryBuffer::getMemBuffer(*BufOrErr, false);<br>
<br>
Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a?rev=277177&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a?rev=277177&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a (added)<br>
+++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a Fri Jul 29 12:44:13 2016<br>
@@ -0,0 +1,13 @@<br>
+!<arch><br>
+// 26 `<br>
+1234567890123456hello.c/<br>
+<br>
+/507 0 0 0 644 102 `<br>
+#include <stdio.h><br>
+#include <stdlib.h><br>
+int<br>
+main()<br>
+{<br>
+ printf("Hello World\n");<br>
+ return EXIT_SUCCESS;<br>
+}<br>
<br>
Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a?rev=277177&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a?rev=277177&view=auto</a><br>
==============================================================================<br>
Binary file - no diff available.<br>
<br>
Propchange: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a<br>
------------------------------------------------------------------------------<br>
svn:mime-type = application/octet-stream<br>
<br>
Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a?rev=277177&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a?rev=277177&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a (added)<br>
+++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a Fri Jul 29 12:44:13 2016<br>
@@ -0,0 +1,10 @@<br>
+!<arch><br>
+#1/@123$ 1469564779 124 0 100644 102 `<br>
+#include <stdio.h><br>
+#include <stdlib.h><br>
+int<br>
+main()<br>
+{<br>
+ printf("Hello World\n");<br>
+ return EXIT_SUCCESS;<br>
+}<br>
<br>
Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a?rev=277177&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a?rev=277177&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a (added)<br>
+++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a Fri Jul 29 12:44:13 2016<br>
@@ -0,0 +1,13 @@<br>
+!<arch><br>
+foo.c 1444941645 124 0 100644 17 `<br>
+void foo(void){}<br>
+<br>
+#1/1234 1469564779 124 0 100644 126 `<br>
+1234567890123456Xhello.c#include <stdio.h><br>
+#include <stdlib.h><br>
+int<br>
+main()<br>
+{<br>
+ printf("Hello World\n");<br>
+ return EXIT_SUCCESS;<br>
+}<br>
<br>
Added: llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a?rev=277177&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a?rev=277177&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a (added)<br>
+++ llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a Fri Jul 29 12:44:13 2016<br>
@@ -0,0 +1,13 @@<br>
+!<arch><br>
+// 26 `<br>
+1234567890123456hello.c/<br>
+<br>
+/&a25* 0 0 0 644 102 `<br>
+#include <stdio.h><br>
+#include <stdlib.h><br>
+int<br>
+main()<br>
+{<br>
+ printf("Hello World\n");<br>
+ return EXIT_SUCCESS;<br>
+}<br>
<br>
Modified: llvm/trunk/test/tools/llvm-objdump/malformed-archives.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/malformed-archives.test?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objdump/malformed-archives.test?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/tools/llvm-objdump/malformed-archives.test (original)<br>
+++ llvm/trunk/test/tools/llvm-objdump/malformed-archives.test Fri Jul 29 12:44:13 2016<br>
@@ -23,10 +23,38 @@<br>
# RUN: %p/Inputs/libbogus4.a \<br>
# RUN: 2>&1 | FileCheck -check-prefix=bogus4 %s<br>
<br>
-# bogus4: libbogus4.a': truncated or malformed archive (remaining size of archive too small for next archive member header at offset 170)<br>
+# bogus4: libbogus4.a': truncated or malformed archive (remaining size of archive too small for next archive member header for foo.c)<br>
<br>
# RUN: not llvm-objdump -macho -archive-headers \<br>
# RUN: %p/Inputs/libbogus5.a \<br>
# RUN: 2>&1 | FileCheck -check-prefix=bogus5 %s<br>
<br>
-# 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)<br>
+# 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)<br>
+<br>
+# RUN: not llvm-objdump -macho -archive-headers \<br>
+# RUN: %p/Inputs/libbogus6.a \<br>
+# RUN: 2>&1 | FileCheck -check-prefix=bogus6 %s<br>
+<br>
+# bogus6: libbogus6.a': truncated or malformed archive (name contains a leading space for archive member header at offset 96)<br>
+<br>
+# RUN: not llvm-objdump -macho -archive-headers \<br>
+# RUN: %p/Inputs/libbogus7.a \<br>
+# RUN: 2>&1 | FileCheck -check-prefix=bogus7 %s<br>
+<br>
+# 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)<br>
+<br>
+# RUN: not llvm-objdump -macho -archive-headers \<br>
+# RUN: %p/Inputs/libbogus8.a \<br>
+# RUN: 2>&1 | FileCheck -check-prefix=bogus8 %s<br>
+<br>
+# 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)<br>
+<br>
+# RUN: not llvm-objdump -s %p/Inputs/libbogus9.a \<br>
+# RUN: 2>&1 | FileCheck -check-prefix=bogus9 %s<br>
+<br>
+# 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)<br>
+<br>
+# RUN: not llvm-objdump -s %p/Inputs/libbogus10.a \<br>
+# RUN: 2>&1 | FileCheck -check-prefix=bogus10 %s<br>
+<br>
+# 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)<br>
<br>
Modified: llvm/trunk/tools/dsymutil/BinaryHolder.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/BinaryHolder.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/BinaryHolder.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/dsymutil/BinaryHolder.cpp (original)<br>
+++ llvm/trunk/tools/dsymutil/BinaryHolder.cpp Fri Jul 29 12:44:13 2016<br>
@@ -115,8 +115,8 @@ BinaryHolder::GetArchiveMemberBuffers(St<br>
if (Verbose)<br>
outs() << "\tfound member in current archive.\n";<br>
auto ErrOrMem = Child.getMemoryBufferRef();<br>
- if (auto Err = ErrOrMem.getError())<br>
- return Err;<br>
+ if (!ErrOrMem)<br>
+ return errorToErrorCode(ErrOrMem.takeError());<br>
Buffers.push_back(*ErrOrMem);<br>
}<br>
}<br>
<br>
Modified: llvm/trunk/tools/llvm-ar/llvm-ar.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-ar/llvm-ar.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-ar/llvm-ar.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/llvm-ar/llvm-ar.cpp (original)<br>
+++ llvm/trunk/tools/llvm-ar/llvm-ar.cpp Fri Jul 29 12:44:13 2016<br>
@@ -409,8 +409,8 @@ static void performReadOperation(Archive<br>
{<br>
Error Err;<br>
for (auto &C : OldArchive->children(Err)) {<br>
- ErrorOr<StringRef> NameOrErr = C.getName();<br>
- failIfError(NameOrErr.getError());<br>
+ Expected<StringRef> NameOrErr = C.getName();<br>
+ failIfError(NameOrErr.takeError());<br>
StringRef Name = NameOrErr.get();<br>
<br>
if (Filter) {<br>
@@ -537,8 +537,8 @@ computeNewArchiveMembers(ArchiveOperatio<br>
Error Err;<br>
for (auto &Child : OldArchive->children(Err)) {<br>
int Pos = Ret.size();<br>
- ErrorOr<StringRef> NameOrErr = Child.getName();<br>
- failIfError(NameOrErr.getError());<br>
+ Expected<StringRef> NameOrErr = Child.getName();<br>
+ failIfError(NameOrErr.takeError());<br>
StringRef Name = NameOrErr.get();<br>
if (Name == PosName) {<br>
assert(AddAfter || AddBefore);<br>
<br>
Modified: llvm/trunk/tools/llvm-nm/llvm-nm.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-nm/llvm-nm.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-nm/llvm-nm.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/llvm-nm/llvm-nm.cpp (original)<br>
+++ llvm/trunk/tools/llvm-nm/llvm-nm.cpp Fri Jul 29 12:44:13 2016<br>
@@ -198,13 +198,14 @@ static void error(llvm::Error E, StringR<br>
HadError = true;<br>
errs() << ToolName << ": " << FileName;<br>
<br>
- ErrorOr<StringRef> NameOrErr = C.getName();<br>
+ Expected<StringRef> NameOrErr = C.getName();<br>
// TODO: if we have a error getting the name then it would be nice to print<br>
// the index of which archive member this is and or its offset in the<br>
// archive instead of "???" as the name.<br>
- if (NameOrErr.getError())<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
errs() << "(" << "???" << ")";<br>
- else<br>
+ } else<br>
errs() << "(" << NameOrErr.get() << ")";<br>
<br>
if (!ArchitectureName.empty())<br>
@@ -1099,9 +1100,11 @@ static void dumpSymbolNamesFromFile(std:<br>
ErrorOr<Archive::Child> C = I->getMember();<br>
if (error(C.getError()))<br>
return;<br>
- ErrorOr<StringRef> FileNameOrErr = C->getName();<br>
- if (error(FileNameOrErr.getError()))<br>
+ Expected<StringRef> FileNameOrErr = C->getName();<br>
+ if (!FileNameOrErr) {<br>
+ error(FileNameOrErr.takeError(), Filename);<br>
return;<br>
+ }<br>
StringRef SymName = I->getName();<br>
outs() << SymName << " in " << FileNameOrErr.get() << "\n";<br>
}<br>
<br>
Modified: llvm/trunk/tools/llvm-objdump/MachODump.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/MachODump.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/MachODump.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/llvm-objdump/MachODump.cpp (original)<br>
+++ llvm/trunk/tools/llvm-objdump/MachODump.cpp Fri Jul 29 12:44:13 2016<br>
@@ -1521,16 +1521,23 @@ static void printArchiveChild(StringRef<br>
}<br>
<br>
if (verbose) {<br>
- ErrorOr<StringRef> NameOrErr = C.getName();<br>
- if (NameOrErr.getError()) {<br>
- StringRef RawName = C.getRawName();<br>
+ Expected<StringRef> NameOrErr = C.getName();<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
+ Expected<StringRef> NameOrErr = C.getRawName();<br>
+ if (!NameOrErr)<br>
+ report_error(Filename, C, NameOrErr.takeError(), ArchitectureName);<br>
+ StringRef RawName = NameOrErr.get();<br>
outs() << RawName << "\n";<br>
} else {<br>
StringRef Name = NameOrErr.get();<br>
outs() << Name << "\n";<br>
}<br>
} else {<br>
- StringRef RawName = C.getRawName();<br>
+ Expected<StringRef> NameOrErr = C.getRawName();<br>
+ if (!NameOrErr)<br>
+ report_error(Filename, C, NameOrErr.takeError(), ArchitectureName);<br>
+ StringRef RawName = NameOrErr.get();<br>
outs() << RawName << "\n";<br>
}<br>
}<br>
<br>
Modified: llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp (original)<br>
+++ llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp Fri Jul 29 12:44:13 2016<br>
@@ -312,13 +312,14 @@ LLVM_ATTRIBUTE_NORETURN void llvm::repor<br>
const object::Archive::Child &C,<br>
llvm::Error E,<br>
StringRef ArchitectureName) {<br>
- ErrorOr<StringRef> NameOrErr = C.getName();<br>
+ Expected<StringRef> NameOrErr = C.getName();<br>
// TODO: if we have a error getting the name then it would be nice to print<br>
// the index of which archive member this is and or its offset in the<br>
// archive instead of "???" as the name.<br>
- if (NameOrErr.getError())<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
llvm::report_error(ArchiveName, "???", std::move(E), ArchitectureName);<br>
- else<br>
+ } else<br>
llvm::report_error(ArchiveName, NameOrErr.get(), std::move(E),<br>
ArchitectureName);<br>
}<br>
<br>
Modified: llvm/trunk/tools/llvm-size/llvm-size.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-size/llvm-size.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-size/llvm-size.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/llvm-size/llvm-size.cpp (original)<br>
+++ llvm/trunk/tools/llvm-size/llvm-size.cpp Fri Jul 29 12:44:13 2016<br>
@@ -114,13 +114,14 @@ static void error(llvm::Error E, StringR<br>
HadError = true;<br>
errs() << ToolName << ": " << FileName;<br>
<br>
- ErrorOr<StringRef> NameOrErr = C.getName();<br>
+ Expected<StringRef> NameOrErr = C.getName();<br>
// TODO: if we have a error getting the name then it would be nice to print<br>
// the index of which archive member this is and or its offset in the<br>
// archive instead of "???" as the name.<br>
- if (NameOrErr.getError())<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
errs() << "(" << "???" << ")";<br>
- else<br>
+ } else<br>
errs() << "(" << NameOrErr.get() << ")";<br>
<br>
if (!ArchitectureName.empty())<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</div></blockquote></div><br></div></div></blockquote></div></div></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>