<div dir="ltr">On 11 December 2013 12:37, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</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">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>
>[...]<br>
<div class="im">> I'm glad you're dragging this out, Brian.  IMHO we have to think carefully<br>
> about these sorts of issues to keep the quality of Mesa high.<br>
><br>
> Now that I've spent a day ruminating about this, I've changed my mind<br>
> somewhat.  The question at hand is what Mesa should do in the event that<br>
> something happens that we think ought to be "impossible" (e.g. because a<br>
> data structure got corrupted or some unanticipated case arose).  It seems<br>
> to me that our top priorities should be that:<br>
><br>
> 1. In debug builds, Mesa should abort as quickly as possible when an<br>
> "impossible" case arises, because that gives us the best chance of noticing<br>
> bugs before release, and aborting early makes the bugs easier to track down<br>
> either by looking at the backtrace or snooping around in the debugger.<br>
><br>
> 2. In release builds, Mesa should try to limp along as well as possible<br>
> without crashing, even if this causes incorrect rendering, because that<br>
> makes it more likely that the end user will be able to live with the bug<br>
> while waiting for us to fix it.<br>
><br>
<br>
</div>Doesn't this increase the probability of the bug never getting reported?<br>
I'm starting to think that it might be preferable to do neither of the<br>
previous proposals and simply print a sensible error message and abort,<br>
both in debug and release builds.<br></blockquote><div><br></div><div>I'm sorry, but I disagree.  We have to balance the desire to have bugs reported against the desire for Mesa to be robust when it's in the hands of end users.  I don't believe I'm taking a controversial position here--this is the reason why assert() exists, and the reason why so many successful software products make use of it (or something like it).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> The assert() macro is usually the best way to balance these priorities, and<br>
> I think it's what we should use as our first line of defense.<br>
><br>
> _mesa_problem() has one advantage over assert(), which is that it causes a<br>
> message to be printed even in release builds, and that means that it gives<br>
> alert users the opportunity to help us find bugs.  But since it doesn't<br>
> cause debug builds to abort, it doesn't help non-alert developers very much<br>
> (and frankly, a lot of us fall into the "non-alert developer" category most<br>
> of the time).  That's why I usually prefer assert() over _mesa_problem().<br>
><br>
> Based on Brian's experiments, it sounds like __builtin_unreachable() is<br>
> worse than asserts for 1, because if it leads to a crash, the backtrace may<br>
> not be correct, and it's worse than asserts for 2, because in release<br>
> builds, we really want to avoid crashes if we can.  I think we should only<br>
> use __builtin_unreachable() for those code paths that are so hot that the<br>
> additional compiler optimizations cause a significant performance<br>
> improvement.  And frankly I suspect that significant performance gains due<br>
> to the use of __builtin_unreachable() are going to be extraordinarily rare.<br>
><br>
><br>
> In this particular case (_mesa_tex_target_is_layered() and<br>
> _mesa_get_texture_layers()), I see no evidence that the code path is hot<br>
> enough for the compiler optimizations to be worth the extra crash risk, so<br>
> I'd support Brian's proposal of doing:<br>
><br>
> switch () {<br>
> case:<br>
>    ...<br>
> default:<br>
>    assert(!"Unexpected switch value");<br>
>    return SENSIBLE_DEFAULT;<br>
> }<br>
><br>
</div></div><div class="im">> As for Francisco's proposed patch to make unreachable() produce an abort in<br>
> debug builds, I don't really have any objection per se, but since I believe<br>
> the cases where unreachable() is worth using are so extraordinarily rare, I<br>
> don't really see a big advantage either.<br>
<br>
</div>Your assumption that the cases where returning a "sensible" default<br>
leads to a sensible outcome are any less extraordinarily rare than<br>
__builtin_unreachable() increasing performance seems surprising to me.<br>
I don't think that there is any practical interest in any of both<br>
choices, the probabilities of any of them helping more than the other<br>
may well be negligible.  This whole discussion seems of purely<br>
intellectual and aesthetic interest to me and I think it would be a<br>
waste of time to keep dragging it out.<br></blockquote><div><br></div><div>"Sensible" may be overly strong of a word.  The important thing is that the code should return something that has a decent likelihood of allowing Mesa to continue executing without crashing, even if that means that rendering is incorrect, or Mesa generates spurious GL errors.  Incorrect rendering and spurious GL errors are things that users can frequently live with while waiting for a bug fix.  Crashes are far more likely to make the system unusable.<br>
</div><div><br>As to the question of whether this is of purely academic interest, I don't think it is.  New enumerants get added to GL all the time.  When those new enumerants get added, we have to update all of the switch statements in our code to handle them.  Sometimes we forget to update some of them.  When that happens, the proper use of assert() makes the difference between the user seeing a crash and the user seeing incorrect rendering or spurious GL errors.  And that in turn often makes the difference between the bug being a showstopper that forces us to interrupt our development flow and do an emergency release, versus being a normal priority bug that we can fix using our normal release process.<br>
</div></div></div></div>