<p dir="ltr">With comdats parts of the file might never be read.</p>
<p dir="ltr">There are also multiple levels of cache and while all the relocations of a file will fit in ram, it is probably still more efficient to validate them as they are read.</p>
<p dir="ltr">But I think (typing on a phone) that relocations are another case where the best is to have a more specific api: the only way the relocation is invalid from the perspective of the api we provide is if the symbol index is invalid. We can just return symbol_end for that and avoid the error_code and the ErrorOr.</p>
<p dir="ltr">Cheers, <br>
Rafael </p>
<div class="gmail_quote">On Jun 1, 2015 6:19 PM, "Lang Hames" <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Out of interest, what parts of an object file commonly don't get mapped in by the linker? I'd have assumed linkers would usually end up touching just about everything.<div><br></div><div>Even assuming that whole file validation is too coarse for performance reasons, I'd prefer something coarser than what we currently have. For example, right now every access to a relocation has to deal with an error return, but I assume it's rare to parse a single relocation in isolation. Why don't we validate all relocations for each section up front, then have a simpler API for accessing them?</div><div><br></div><div>I don't have especially strong feelings about this, just a nagging sense that libObject's error handling is unnecessarily fine grained.</div><div><br></div><div>FWIW, of the other two proposals I prefer the ErrorOr approach.</div><div><br></div><div>Cheers,</div><div>Lang. </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 29, 2015 at 6:45 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, May 29, 2015 at 4:06 PM, Alexey Samsonov <span dir="ltr"><<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>></span> wrote:<br></span><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi everyone,<div><br></div><div>Having proper error handling in LLVM's Object parsing library is a nice thing by itself, but it would additionally allow us to find bugs by fuzzing (see r238451 that adds llvm-dwarfdump-fuzzer tool), for which the clean input validation is essential.</div><div><br></div><div>This is a generic discussion of state of affairs. I want to do some progress in fuzzing before we finish it (especially if we decide to make a significant intrusive changes), you may scroll down for my plan.</div><div><br></div><div>The code in lib/Object calls report_fatal_error() far too often, both when we're (a) just constructing the specific implementation of ObjectFile, and (b) when we access its contents and find out the file is broken and can't be parsed properly.</div><div><br></div><div>We should just go and fix (a): ObjectFile factory methods return ErrorOr<std::unique_ptr<ObjectFile>>, and we should propagate the error appropriately.</div><div><br></div><div>(b) is harder. The current approach is to use std::error_code as a return type, and store the result in by-reference argument, for instance:</div><div> std::error_code getSymbolAddress(DataRefImpl Symbol, uint64_t &Res);</div><div><br></div><div>I wanted to follow this approach in a proposed large MachO API change</div><div>(<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10111&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=t8fOzjcE-HffAHdlkx8x2RzDYXmOrfwTVRZMBY_I1-k&s=vIEUEH66ZENYUbm5_hrLkGBXJih9kgG91w9SNPrpGGU&e=" target="_blank">http://reviews.llvm.org/D10111</a>), but it raised discussion on whether this approach is right.</div><div>Moving this discussion here. I see the following options:</div><div><br></div><div>1. Use the current approach:</div><div> std::error_code getSymbolAddress(DataRefImpl Symbol, uint64_t &Res);</div><div><br></div><div>Pros:</div><div> * don't need to change a large number of (often virtual) API functions</div><div> * has a nice error handling pattern used in LLVM tools:</div><div> uint64_t Addr;</div><div> if (error(getSymbolAddress(Symbol, Addr)))</div><div> return; // or continue, or do anything else.</div><div><br></div><div>Cons:</div><div> * return value can just be silently ignored. Adding warn_unused_result attribute on per-function basis is ugly</div><div> * adds extra overhead for cases when we're certain the result would be valid.</div><div><br></div><div>2. Switch to ErrorOr wrapper:</div><div> ErrorOr<uint64_t> getSymbolAddress(DataRefImpl Symbol);</div><div><br></div><div>Pros:</div><div> * handling the error is now mandatory and explicit.</div><div> * callers can explicitly skip error handling if they know the result would be valid:</div><div> uint64_t Addr = getSymbolAddress(Symbol).get();</div><div> and it would fail the assert if they are wrong.</div><div><br></div><div>Cons:</div><div> * need to change lots of old code, or live with two versions of functions</div><div> * error handling boilerplate in regular code on call site is ugly:</div><div> auto AddrOrErr = getSymbolAddress(Symbol);</div><div> if (AddrOrErr.hasError())</div><div> return; // or continue, or whatever</div><div> uint64_t Addr = AddrOrErr.get();</div><div> (can probably be improved with a macro)</div><div> * adds extra overhead for cases when we're certain the result would be valid.</div><div><br></div><div>On IRC discussion Lang suggested</div><div>3. Check the whole object file contents in constructor or validate() method, and get rid</div><div>of all error codes in regular accessors.</div></div></blockquote><div><br></div></div></div><div>Unfortunately this option isn't possible with the current libObject. libObject is specifically designed to avoid paging in or allocating any memory beyond simply mmap'ing the file. This makes it annoying to use, but also allows it to be used for demanding use cases like a linker. For many things, I've always wished we had a "libEasyObject" that avoided this annoyance, but it seems like a lot of work compared to the benefit.</div><span><font color="#888888"><div><br></div><div>-- Sean Silva</div><div> </div></font></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><div dir="ltr"><div><br></div><div>Pros:</div><div> * the interface is much cleaner</div><div> * no extra overhead for trusted (e.g. JIT) object files.</div><div><br></div><div>Cons:</div><div> * significant change, fundamentally shifting the way object files are handled</div><div> * verifier function should now absolutely everything about the object file, and anticipate all possible use cases. Especially hard, assuming that ObjectFile interface allows user to pass any garbage as input arguments (e.g. as DataRefImpl in the example above).</div><div> * verifier can be slow, and might be an overkill if we strongly desire to parse some bits of object file lazily.</div><div><br></div><div>================</div><div><br></div><div>Instead of <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10111&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=t8fOzjcE-HffAHdlkx8x2RzDYXmOrfwTVRZMBY_I1-k&s=vIEUEH66ZENYUbm5_hrLkGBXJih9kgG91w9SNPrpGGU&e=" target="_blank">http://reviews.llvm.org/D10111</a>, I'm going to proceed with minimal incremental changes, that would allow fuzzer to move forward. Namely, I want to keep the changes to headers as small as possible, changing functions one by one, and preferring to use ErrorOr<> construct (option 2 listed above). An additional benefit of this is that each small incremental change would be accompanied by the test case generated by fuzzer, that exposed this problem.</div><div><br></div><div>Let me know if you think it's a good or terrible idea.</div><span><font color="#888888"><div><br></div><div>-- <br></div><div><div><div><div><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</div></div></div></font></span></div>
<br></span><span>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></span></blockquote></div><br></div></div>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div>