<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 dir="ltr">On Mon, Aug 1, 2016 at 2:45 PM Kevin Enderby <<a href="mailto:enderby@apple.com">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 class="m_2028685715060451787Apple-interchange-newline"></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><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><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>