<div dir="ltr">On Tue, Oct 7, 2014 at 12:42 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Mon, Oct 6, 2014 at 7:55 PM, Saleem Abdulrasool <span dir="ltr"><<a href="mailto:compnerd@compnerd.org" target="_blank">compnerd@compnerd.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"><span>On Mon, Oct 6, 2014 at 6:42 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br></span><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Mon, Oct 6, 2014 at 6:25 PM, Saleem Abdulrasool <span dir="ltr"><<a href="mailto:compnerd@compnerd.org" target="_blank">compnerd@compnerd.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"><span>On Mon, Oct 6, 2014 at 5:40 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br></span><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ruiu<br>
Date: Mon Oct 6 19:40:54 2014<br>
New Revision: 219176<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=219176&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=219176&view=rev</a><br>
Log:<br>
Do not use llvm_unreachable at reachable code.<br>
<br>
These lines can be reachable if we give a broken or unsupported<br>
input object file.<br></blockquote><div><br></div></span><div>Should we really be relying on this for invalid input detection? We should immediately identify if the object file is incorrect when we snarf the input object and it mismatches the target (I recently added that support). Yes, I agree that it could be a broken input, but, shouldn't we have better detection for that in libObject rather than just happily consuming it?</div></div></div></div></blockquote><div><br></div></span><div>This report_fatal_error is for unknown relocation types. That's different from file magic. I don't think we want libObject to check if a relocation type is within a known range for an architecture. If we make libObject to check for that, it would return a reference only when a reference type value is valid. That's fine. But we'd still want to keep this now-really-unreachable error check anyway to cover all switch cases. That doesn't seem a good deal to me to error out on an broken input file, which is a rare event.</div></div></div></div></blockquote><div><br></div></span><div>Yes, I get that the error reporting here is for a different scenario. I was responding to the commit message: if the object file is unsupported, its either because the format is incorrect (how did we get here in this case? the registry shouldn't have dispatched to us) or because the file magic indicates that the file is for a different target (already diagnosed). That leaves the case of a broken/corrupt object file, in which case, libObject seems like the right place to validate the input.</div><div><br></div><div>I completely agree that libObject should not be aware of the details of the relocation types and their ranges. I think that Nick's approach of the error message + code for a bad relocation (or out of range) is better than the report_fatal_error.</div></div></div></div></blockquote><div><br></div></span><div>So you want libObject to validate the input, but at the same time you are thinking that libObject shouldn't be aware the details of relocation types and their ranges? It seems they are conflicting goal to me. What if an object file being read is almost correct, but only a byte at a relocation type is corrupted? In order to verify if a relocation number is valid, libObject needs to know the valid range, no?</div></div></div></div></blockquote><div><br></div><div>Oh, we are talking about different things! I assumed you meant branch range (the distance a relocation type can address) rather than the range of values representing a relocation. Obviously, libObject knows that a series of bytes indicates a relocation, and it knows the type of relocation it maps to.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Modified:<br>
lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp?rev=219176&r1=219175&r2=219176&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp?rev=219176&r1=219175&r2=219176&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp (original)<br>
+++ lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp Mon Oct 6 19:40:54 2014<br>
@@ -558,7 +558,7 @@ void AtomChunk::applyRelocationsX86_32(u<br>
targetAddr - getSectionStartAddr(targetAddr, sectionRva);<br>
break;<br>
default:<br>
- llvm_unreachable("Unsupported relocation kind");<br>
+ llvm::report_fatal_error("Unsupported relocation kind");<br>
}<br>
}<br>
}<br>
@@ -618,7 +618,7 @@ void AtomChunk::applyRelocationsX86_64(u<br>
break;<br>
default:<br>
llvm::errs() << "Kind: " << (int)ref->kindValue() << "\n";<br>
- llvm_unreachable("Unsupported relocation kind");<br>
+ llvm::report_fatal_error("Unsupported relocation kind");<br>
}<br>
}<br>
}<br>
@@ -934,7 +934,7 @@ StringRef chooseSectionByContent(const D<br>
}<br>
llvm::errs() << "Atom: contentType=" << atom->contentType()<br>
<< " permission=" << atom->permissions() << "\n";<br>
- llvm_unreachable("Failed to choose section based on content");<br>
+ llvm::report_fatal_error("Failed to choose section based on content");<br>
}<br>
<br>
typedef std::map<StringRef, std::vector<const DefinedAtom *> > AtomVectorMap;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div><span><font color="#888888"><br><br clear="all"><div><br></div>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org
</font></span></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><div><div><br><br clear="all"><div><br></div>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org
</div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org
</div></div>