<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 9, 2016 at 4:23 PM, Lang Hames via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@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"><span class=""><span style="font-size:13px">> > I don't think these are really independent. Whether or not you need to emit<br>> > a diagnostic depends on whether a caller can handle the corresponding error,<br>> > which isn't something you know at the point where the error is raised.<br><br></span><span style="font-size:13px">> But you do in the diag handler. For example, if you are trying to open</span><br style="font-size:13px"><span style="font-size:13px">> multiple files, some of which are bitcode, you know to ignore</span><br style="font-size:13px"><span style="font-size:13px">> InvalidBitcodeSignature.</span><br style="font-size:13px"><div><span style="font-size:13px"><br></span></div></span><div>That means anticipating the kinds of errors you do/don't want to recover from at the point at which you insatiate your diagnostic handler. At that point you may not have the information that you need to know whether you want to recover. E.g. What if one path through library code can recover, and another can't? </div></div></blockquote><div><br></div><div>+1 to this (I had the same thought - stateful handlers would be pretty tricky, doubly so with \/ this point)</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>Worse still, what if the divergence point between these two paths is in library code itself? Now you need to thread two diagnostic handlers up to that point. This scales very poorly.</div><div><br></div><div>Simply put: diagnostic handlers aren't library friendly. It's tough enough to get all libraries to agree on an error type, without having them all agree on a diagnostic handlers too, and thread them everywhere.</div></div></blockquote><div><br></div><div>I think it depends a bit on the library & how generic it is - likely lower level libraries (like libObject) will want a granular error-based result, not a diagnostic engine. (the clients will be more widely varied (than, say, Clang) & have different diagnostic/behavioral needs, etc)<br><br>But I think it makes total sense for Clang to use a diagnostic handling approach for the most part (all the possible ways that C++ can be written incorrectly essentially amount to the same thing for a consumer of Clang - code was bad & I need some text (and fixits, notes, etc) to tell the user why), the IR verifier similarly - and possibly LLD, for example, depending on how broad the use cases become.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class=""><div><br></div><div>> <span style="font-size:13px">This highlights why I think it is important to keep diagnostics and</span></div><span style="font-size:13px">> errors separate. In the solution you have there is a need to allocate</span><br style="font-size:13px"><span style="font-size:13px">> a std::string, even if that is never used.</span><div><span style="font-size:13px"><br></span></div></span><div><div>Errors are only constructed on the error path. There is no construction on the success path.</div></div><span class=""><div><br></div><div><span style="font-size:13px">> If it was always a</span><br style="font-size:13px"><span style="font-size:13px">> std::string it would not be that bad, but the framework seems designed</span><br style="font-size:13px"><span style="font-size:13px">> to allow even more context to be stored, opening the way for fun cases</span><br style="font-size:13px"><span style="font-size:13px">> of error handling interfering with lifetime management.</span><br style="font-size:13px"><div><br></div></div></span><div>This is a real problem, but I think the solution is to have a style guideline: You can't use reference types (in the general sense - references, pointers, etc.) in errors. Errors that would otherwise require references should have their error-message stringified at the point of creation.</div><span class=""><div><br></div><div>> <span style="color:rgb(80,0,80);font-size:13px">> Consider someone who wants to use libObject to write an object-file repair</span></div><span style="font-size:13px">> > tool: A "bad_symbol" error code tells you what went wrong, but not where or<br>> > why, so it's not much use in attempting a fix. On the other hand "bad_symbol<br>> > { idx = 10 }" gives you enough information to pop up a dialog, ask the user<br>> > what the symbol at index 10 should be, then re-write that symbol table<br>> > entry.<br><br></span><span style="font-size:13px">> Using error results to find deep information you want to patch seems</span><br style="font-size:13px"><span style="font-size:13px">> like a bad idea. A tool that wants to patch symbol indexes should be</span><br style="font-size:13px"><span style="font-size:13px">> reading them directly.</span><div><br></div></span><div>I'm not sure I agree, but I don't have strong feelings on this particular example - it was intended as a straw man. My point is that error recover is useful in general: there is a reason things like exceptions exist. Diagnostic handlers are very poorly suited to general error recovery.</div><span class=""><div><br></div><div><span style="color:rgb(80,0,80);font-size:13px">> > void check(TypedError Err) {</span><br style="color:rgb(80,0,80);font-size:13px"><span style="color:rgb(80,0,80);font-size:13px">> > if (!Err)</span><br style="color:rgb(80,0,80);font-size:13px"><span style="color:rgb(80,0,80);font-size:13px">> > return;</span><br style="color:rgb(80,0,80);font-size:13px"><span style="color:rgb(80,0,80);font-size:13px">> ></span><br style="color:rgb(80,0,80);font-size:13px"><span style="color:rgb(80,0,80);font-size:13px">> > logAllUnhandledErrors(std::</span><span style="color:rgb(80,0,80);font-size:13px">move(Err), errs(), "<tool name>");</span><br style="color:rgb(80,0,80);font-size:13px"><span style="color:rgb(80,0,80);font-size:13px">> > exit(1);</span><br style="color:rgb(80,0,80);font-size:13px"><span style="color:rgb(80,0,80);font-size:13px">> > }</span><br></div><div><br></div><div>> <span style="font-size:13px">That is not the same that you get with a diagnostic handler. What you</span></div><span style="font-size:13px">> get is an exit after the error was propagated over the library layer,</span><br style="font-size:13px"><span style="font-size:13px">> instead of an exit at the point where the issue is found.</span><br style="font-size:13px"><div><br></div></span><div>As long as you log the error I'm not sure I see a meaningful difference? If you're not debugging you shouldn't be crashing in your library. If you are debugging you want to set a breakpoint on the error constructor instead.</div><span class=""><div><br></div><div>> <span style="font-size:13px">I would prefer the diagnostic handler for exactly the same reason :-)</span></div></span><span style="font-size:13px">> Generality has a cost...</span><div><span style="font-size:13px"><br></span></div><div>Could you explain what you see as the cost in this instance?</div><div><br></div><div>The scheme has a higher cost that std::error_code alone when you're returning an error, but I don't think optimizing the error case is a sensible thing to do. It has a potentially *lower* cost on the success path, as you don't need to take up an argument for the diagnostic handler.</div><div><br></div><div>The most serious objection that you've raised, to my mind, is the potential lifetime issues if people mistakenly use references in their error types. I'm planning to write additions to llvm's coding guidelines to cover that, which I think should be sufficient.</div><div><br></div><div>On the flip side, compared to a diagnostic handler, I think the scheme I've proposed has the following benefits:</div><div><br></div><div>- No need to thread a diagnostic handler type through the world (and consequently no need to agree on its type). Retrofitting rich error logging to an interface that already returns errors requires less work than adding a diagnostic handler.</div><div>- The ability to describe error hierarchies (e.g. the class of all errors that represent malformed objects, which may be further subclassed to describe specific errors).</div><div>- The ability for client code to meaningfully distinguish between error instances, rather than just error types.</div><span class=""><div><br></div><div><span style="font-size:13px">> , and I don't think we should invest on it until</span><br></div><div><span style="font-size:13px">> we have a concrete case where that is needed.</span></div><div><br></div></span><div>The error infrastructure investment is largely done, and I'm volunteering to maintain it. Myself and Kevin Enderby are signing up to weave this through libObject and the JIT. Other libraries could be converted as people see fit, with ECError providing an interface between the std::error_code world and TypedError.<br></div><div><br></div><div><span class=""><span style="font-size:13px">> Given that I would</span><br style="font-size:13px"><span style="font-size:13px">> suggest going the more incremental way. Add a CheckedError that wraps</span><br style="font-size:13px"><span style="font-size:13px">> error_code and use to make sure the errors are checked. When a better</span><br style="font-size:13px"><span style="font-size:13px">> diagnostic is needed, pass in a diagnostic handler.</span><br style="font-size:13px"><br></span>Our first use-case for this system is libObject, where we want to use this to enable richer error messages. An incremental approach would involve threading a diagnostic handler through everything (and returning CheckedError), then un-threading it again would involve a lot of churn. I'd prefer to move directly to the scheme I'm proposing. <br><br>Cheers,</div><div>Lang.<div><br></div></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 9, 2016 at 3:27 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>> I don't think these are really independent. Whether or not you need to emit<br>
> a diagnostic depends on whether a caller can handle the corresponding error,<br>
> which isn't something you know at the point where the error is raised.<br>
<br>
</span>But you do in the diag handler. For example, if you are trying to open<br>
multiple files, some of which are bitcode, you know to ignore<br>
InvalidBitcodeSignature.<br>
<span><br>
<br>
> That's the idea behind the 'log' method on TypedErrorInfo: It lets you<br>
> transform meaningful information about the problem into a log message<br>
> *after* you know whether it can be handled or not. Recoverable errors can be<br>
> logged (if the client wants to do so), but they don't have to be.<br>
<br>
</span>...<br>
<span><br>
> By contrast, in my system this would be:<br>
><br>
> class InvalidBitcodeSignature : TypedErrorInfo<InvalidBitcodeSignature> {};<br>
> class CorruptedBitcode : TypedErrorInfo<CorruptedBitcode> {<br>
> public:<br>
> CorruptedBitcode(std::string Msg) : Msg(Msg) {}<br>
> void log(raw_ostream &OS) const override { OS << Msg; }<br>
> private:<br>
> std::string Msg;<br>
> };<br>
<br>
</span>This highlights why I think it is important to keep diagnostics and<br>
errors separate. In the solution you have there is a need to allocate<br>
a std::string, even if that is never used. If it was always a<br>
std::string it would not be that bad, but the framework seems designed<br>
to allow even more context to be stored, opening the way for fun cases<br>
of error handling interfering with lifetime management.<br>
<span><br>
> Consider someone who wants to use libObject to write an object-file repair<br>
> tool: A "bad_symbol" error code tells you what went wrong, but not where or<br>
> why, so it's not much use in attempting a fix. On the other hand "bad_symbol<br>
> { idx = 10 }" gives you enough information to pop up a dialog, ask the user<br>
> what the symbol at index 10 should be, then re-write that symbol table<br>
> entry.<br>
<br>
</span>Using error results to find deep information you want to patch seems<br>
like a bad idea. A tool that wants to patch symbol indexes should be<br>
reading them directly.<br>
<span><br>
><br>
>> Other advantages are:<br>
>><br>
>> * It is easy to use fatal error in simple programs by just using a<br>
>> diganostic handler that exits.<br>
>> * Very debugger friendly. It is easy to put a breakpoint at the diag<br>
>> handler.<br>
><br>
> This proposal provides the same advantages. I noted the second point in the<br>
> original RFC. The first is easily implemented using an idiom I've seen in<br>
> llvm-objdump and elsewhere:<br>
><br>
> void check(TypedError Err) {<br>
> if (!Err)<br>
> return;<br>
><br>
> logAllUnhandledErrors(std::move(Err), errs(), "<tool name>");<br>
> exit(1);<br>
> }<br>
<br>
</span>That is not the same that you get with a diagnostic handler. What you<br>
get is an exit after the error was propagated over the library layer,<br>
instead of an exit at the point where the issue is found.<br>
<br>
We use the above code now because lib/Object has no diagnostic handler.<br>
<span><br>
> Side-note: you can actually go one better and replace this idiom with:<br>
><br>
> template <typename T><br>
> T check(TypedErrorOr<T> ValOrErr) {<br>
> if (!ValOrErr) {<br>
> logAllUnhandledErrors(ValOrErr.takeError(), errs(), "<tool name>");<br>
> exit(1);<br>
> }<br>
> return std::move(*ValOrErr);<br>
> }<br>
<br>
</span>Yes, we do pretty much that in ELF/COFF lld.<br>
<span><br>
<br>
> Using TypedError as a wrapper for std::error_code would prevent errors from<br>
> be silently dropped, which is a huge step forward. (Though TypedError would<br>
> be a misnomer - we'd need a new name. :)<br>
<br>
</span>CheckedError?<br>
<span><br>
> Using a diagnostic handler would allow richer diagnostics from libObject,<br>
> but it supports a narrower class of error-recovery than my proposal. I'm<br>
> inclined to favor my proposal as a strictly more general solution.<br>
<br>
</span>I would prefer the diagnostic handler for exactly the same reason :-)<br>
Generality has a cost, and I don't think we should invest on it until<br>
we have a concrete case where that is needed. Given that I would<br>
suggest going the more incremental way. Add a CheckedError that wraps<br>
error_code and use to make sure the errors are checked. When a better<br>
diagnostic is needed, pass in a diagnostic handler.<br>
<br>
If we ever get to the point were we really want to *return* more than<br>
an error_code, it will be a much smaller refactoring to<br>
s/CheckedError/TypedError/.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br></blockquote></div><br></div></div>