<div dir="ltr">Hi Benoit,<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 15, 2015 at 5:22 AM, Benoit Belley <span dir="ltr"><<a href="mailto:benoit.belley@autodesk.com" target="_blank">benoit.belley@autodesk.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Everyone,<br>
<br>
My understanding is that the ELF writer is also used by the MCJIT engine when filling an object buffer with jitted code. In that context, I am wondering if the call to llvm::report_fatal_error() is the appropriate way to report the error. It has the effect of killing the entire host application simply because one LLVM module couldn¹t be jitted.<br></blockquote><div>I'm only doing changes for Mach-O, but future similar changes might hit the ELF reader.</div><div><br></div><div>In any case, this impacts the readers, not the writers. And, more importantly, this is covering over assertions and segfaults for bad user input. I'm not calling report_fatal_error in places where our behavior was well-defined (like when we “recovered” from bad user input in some way).</div><div>I think you'll agree that report_fatal_error is preferable to any assert or segfault, which aren't recoverable (in general). :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Of course, fatal errors and assertions should never occur! ;-) And, if they never occur, then there isn¹t any issue. Moreover, it is much better to exit the application after a proper error message has been emitted, then crashing on a segfault! So, this patch is definitely a step in the right direction.<br></blockquote><div>:-)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On the other hand, report_fatal_error() has the unfortunate consequence of killing our application before our users have any chance of saving their work.<br></blockquote><div>We can make libObject recover from this. But that will force us to change a lot os libObject's internals (and API), since most of it isn't really ready for recovering (even if recovering means just signalling an error occurred to the caller) from bad input.</div><div>I have no real proposal to change the API to one that is resilient to bad input, and there are other libObject criticisms that could be addressed in a possible rewrite of the API. We can start doing it, but I'm not an expert (or even very knowledgeable) on most libObject uses, so I can't propose a better API right now. I can participate in the discussion if you want to propose one, though. ;-)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thus, I am wondering if there would be a way to indicate to the MCJIT engine that an internal error occurred while jitting a particular LLVM module. The compilation of that module would abort, but the application would not exit.<br>
<br>
Is that a legitimate concern ? Comments ?<br></blockquote><div>I don't really think this changes anything for the current users of libObject (whatever hits report_fatal_error was either reading bad memory, asserting, or screwing up the file's semantics in a way that was most likely not even compatible with whatever other tools exist (dynamic loaders, otool, etc), which made it useless to try to keep reading the file).</div><div><br></div><div><br></div><div>If you still have issues, or think we didn't account for something, let me know.</div><div><br></div><div>Regards,</div><div><br></div><div>  Filipe</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Benoit<br>
<span class="HOEnZb"><font color="#888888"><br>
Benoit Belley<br>
Sr Principal Developer<br>
M&E-Product Development Group<br>
Autodesk, Inc.<br>
<a href="http://www.autodesk.com" target="_blank">www.autodesk.com</a> <a href="http://www.autodesk.com/" target="_blank">http://www.autodesk.com/</a><br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D6945" target="_blank">http://reviews.llvm.org/D6945</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>