On 12/10/2013 03:27 PM, Francisco Jerez wrote:
Brian Paul <bri...@vmware.com> writes:
On 12/10/2013 11:06 AM, Francisco Jerez wrote:
Paul Berry <stereotype...@gmail.com> writes:
On 10 December 2013 08:42, Francisco Jerez <curroje...@riseup.net> wrote:
Brian Paul <bri...@vmware.com> writes:
On 12/07/2013 06:17 PM, Francisco Jerez wrote:
[...]
+ default:
+ unreachable();
I think I'd like to see an assertion or _mesa_problem() call to catch
unhandled cases if new texture targets are added in the future.
How about having the unreachable() macro print out an error and abort if
it's ever reached? See the attached patch.
I would prefer for us to simply do this:
assert(!"Unexpected texture target");
unreachable();
That would have the exact same semantics as your proposed patch (assert in
debug builds, allow compiler optimizations in release builds), and it would
have the added advantage that it's obvious what's going on to someone
reading the code. With your proposed patch it's not obvious--the reader
has to be aware that Mesa has a non-standard meaning for unreachable().
The message adds no additional useful information IMHO, and it's already
obvious for anyone reading the code that something marked with
unreachable() shouldn't ever be reached -- What else do we have to say?
I have the impression that using the assert(!"") idiom before every
occurrence of unreachable() just increases the amount of noise in the
code and the likelihood of making mistakes.
Regardless of what we do in this specific case, I believe that making
the unreachable() macro abort is a good thing because it will catch
cases where we have forgotten to pair it with an assert call [I have
already, a few times], and mistakes like writing 'assert("!...' [see
61143b87c16231a2df0d69324d531503027f9aca].
If you would like to include some additional explanation in an
unreachable block you can just write a comment, or make unreachable() a
variadic macro that prints its (optionally passed) argument to stderr
when the unreachable block is executed.
I just did a test. I put unreachable() in a conspicuous place in Mesa
and ran glxinfo. I got a segfault. And in gdb the stack trace wasn't
even correct.
That's precisely what I'm attempting to fix with the change I proposed.
An assert would be much more useful since it tells us the location and
we can get a good stack trace in gdb.
The unreachable() macro could too.
The gcc docs for __builtin_unreachable say it's about expressing your
intent when doing unconventional flow-control, and possibly suppressing
compiler warnings. It doesn't sounds like a debug check to report when
there's unexpected control flow.
Yes, it's mainly useful as a way to suppress warnings and allow the
optimizer make assumptions it wouldn't otherwise make because the
program has invariants beyond the inference capabilities of the compiler
[e.g. that the texObj->Target GLenum is one of the texture target
enums]. Say that we want to express one of these invariants using
unreachable(), an invariant which turns out to be wrong, wouldn't it be
a useful debugging aid to have the program print out an error and abort
rather than having undefined behavior?
Definitely. An unreachable() macro that acts like assert sounds good to me.
I'm more convinced now that we should simply have an assert there rather
than unreachable().
Finally, in release builds we generally don't want to crash in these
situations. So IMHO, it's preferable to do something like:
I have the feeling that this is an orthogonal question to unreachable()
aborting or not. I'm not convinced that there's a "sensible" default in
this case, and setting one seems like trying to delay the inevitable to
me. An invariant of the program has been broken and it's likely that
something will go very wrong sooner or later. The question boils down
to: do we want undefined behavior now, or later? I'd rather have it
now, but it seems like a moot question to me...
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.
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.
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.
Sorry for dragging this out. Please go ahead and post a patch showing
what you have in mind.
-Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
