<div dir="ltr">On 11 December 2013 08:56, Brian Paul <span dir="ltr"><<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><div class="h5">On 12/10/2013 03:27 PM, Francisco Jerez wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> writes:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On 12/10/2013 11:06 AM, Francisco Jerez wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> writes:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On 10 December 2013 08:42, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> writes:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On 12/07/2013 06:17 PM, Francisco Jerez wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
+   default:<br>
+      unreachable();<br>
</blockquote>
<br>
I think I'd like to see an assertion or _mesa_problem() call to catch<br>
unhandled cases if new texture targets are added in the future.<br>
<br>
<br>
</blockquote>
How about having the unreachable() macro print out an error and abort if<br>
it's ever reached?  See the attached patch.<br>
<br>
</blockquote>
<br>
I would prefer for us to simply do this:<br>
<br>
assert(!"Unexpected texture target");<br>
unreachable();<br>
<br>
That would have the exact same semantics as your proposed patch (assert in<br>
debug builds, allow compiler optimizations in release builds), and it would<br>
have the added advantage that it's obvious what's going on to someone<br>
reading the code.  With your proposed patch it's not obvious--the reader<br>
has to be aware that Mesa has a non-standard meaning for unreachable().<br>
</blockquote>
<br>
The message adds no additional useful information IMHO, and it's already<br>
obvious for anyone reading the code that something marked with<br>
unreachable() shouldn't ever be reached -- What else do we have to say?<br>
I have the impression that using the assert(!"") idiom before every<br>
occurrence of unreachable() just increases the amount of noise in the<br>
code and the likelihood of making mistakes.<br>
<br>
Regardless of what we do in this specific case, I believe that making<br>
the unreachable() macro abort is a good thing because it will catch<br>
cases where we have forgotten to pair it with an assert call [I have<br>
already, a few times], and mistakes like writing 'assert("!...' [see<br>
61143b87c16231a2df0d69324d5315<u></u>03027f9aca].<br>
<br>
If you would like to include some additional explanation in an<br>
unreachable block you can just write a comment, or make unreachable() a<br>
variadic macro that prints its (optionally passed) argument to stderr<br>
when the unreachable block is executed.<br>
</blockquote>
<br>
<br>
I just did a test.  I put unreachable() in a conspicuous place in Mesa<br>
and ran glxinfo.  I got a segfault.  And in gdb the stack trace wasn't<br>
even correct.<br>
<br>
</blockquote>
That's precisely what I'm attempting to fix with the change I proposed.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
An assert would be much more useful since it tells us the location and<br>
we can get a good stack trace in gdb.<br>
<br>
</blockquote>
The unreachable() macro could too.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The gcc docs for __builtin_unreachable say it's about expressing your<br>
intent when doing unconventional flow-control, and possibly suppressing<br>
compiler warnings.  It doesn't sounds like a debug check to report when<br>
there's unexpected control flow.<br>
<br>
</blockquote>
Yes, it's mainly useful as a way to suppress warnings and allow the<br>
optimizer make assumptions it wouldn't otherwise make because the<br>
program has invariants beyond the inference capabilities of the compiler<br>
[e.g. that the texObj->Target GLenum is one of the texture target<br>
enums].  Say that we want to express one of these invariants using<br>
unreachable(), an invariant which turns out to be wrong, wouldn't it be<br>
a useful debugging aid to have the program print out an error and abort<br>
rather than having undefined behavior?<br>
</blockquote>
<br></div></div>
Definitely.  An unreachable() macro that acts like assert sounds good to me.<div class="im"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm more convinced now that we should simply have an assert there rather<br>
than unreachable().<br>
<br>
Finally, in release builds we generally don't want to crash in these<br>
situations.  So IMHO, it's preferable to do something like:<br>
<br>
</blockquote>
I have the feeling that this is an orthogonal question to unreachable()<br>
aborting or not.  I'm not convinced that there's a "sensible" default in<br>
this case, and setting one seems like trying to delay the inevitable to<br>
me.  An invariant of the program has been broken and it's likely that<br>
something will go very wrong sooner or later.  The question boils down<br>
to: do we want undefined behavior now, or later?  I'd rather have it<br>
now, but it seems like a moot question to me...<br>
</blockquote>
<br></div>
In the cases at hand, _mesa_tex_target_is_layered() and _mesa_get_texture_layers(), returning false/0 seem like fairly reasonable defaults to return and hopefully wouldn't lead to a later crash.<br>
<br>
Over the years I've seen several bug reports where someone runs their app and sees a bunch of _mesa_problem() error messages but the app keeps running and doesn't crash.  I think that's preferable to just crashing (esp. when Mesa may be running in the X server).  That's the reason I'm hesitant to see __builtin_unreachable() getting used in release builds in this particular scenario.  Having X crash just because we hit a default switch case seems unnecessarily fragile.<br>

<br>
My second concern is this: if someday someone really does want to use __builtin_unreachable() in the context of unconventional flow control (per gcc docs), will our new-and-improved unreachable() macro still be the right tool for them?  I worry about changing the behavior of what seems to be an established convention.<br>

<br>
Sorry for dragging this out.  Please go ahead and post a patch showing what you have in mind.<span class=""><font color="#888888"><br>
<br>
-Brian<br>
<br>
</font></span></blockquote></div><br></div><div class="gmail_extra">I'm glad you're dragging this out, Brian.  IMHO we have to think carefully about these sorts of issues to keep the quality of Mesa high. <br></div>
<div class="gmail_extra"><br>Now that I've spent a day ruminating about this, I've changed my mind somewhat.  The question at hand is what Mesa should do in the event that something happens that we think ought to be "impossible" (e.g. because a data structure got corrupted or some unanticipated case arose).  It seems to me that our top priorities should be that:<br>
<br>1. In debug builds, Mesa should abort as quickly as possible when an "impossible" case arises, because that gives us the best chance of noticing bugs before release, and aborting early makes the bugs easier to track down 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 without crashing, even if this causes incorrect rendering, because that makes it more likely that the end user will be able to live with the bug while waiting for us to fix it.<br>
<br></div><div class="gmail_extra">The assert() macro is usually the best way to balance these priorities, and I think it's what we should use as our first line of defense.<br><br></div><div class="gmail_extra">_mesa_problem() has one advantage over assert(), which is that it causes a message to be printed even in release builds, and that means that it gives alert users the opportunity to help us find bugs.  But since it doesn't cause debug builds to abort, it doesn't help non-alert developers very much (and frankly, a lot of us fall into the "non-alert developer" category most 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 worse than asserts for 1, because if it leads to a crash, the backtrace may not be correct, and it's worse than asserts for 2, because in release builds, we really want to avoid crashes if we can.  I think we should only use __builtin_unreachable() for those code paths that are so hot that the additional compiler optimizations cause a significant performance improvement.  And frankly I suspect that significant performance gains due to the use of __builtin_unreachable() are going to be extraordinarily rare.<br>
</div><div class="gmail_extra"><br><br></div><div class="gmail_extra">In this particular case (_mesa_tex_target_is_layered() and _mesa_get_texture_layers()), I see no evidence that the code path is hot enough for the compiler optimizations to be worth the extra crash risk, so 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>
}<div class=""><div id=":21n" class="" tabindex="0"><img class="" src="https://mail.google.com/mail/u/0/images/cleardot.gif"><br></div><div id=":21n" class="" tabindex="0">As for Francisco's proposed patch to make unreachable() produce an abort in debug builds, I don't really have any objection per se, but since I believe the cases where unreachable() is worth using are so extraordinarily rare, I don't really see a big advantage either.<br>
</div></div></div></div>