[Mesa-dev] [PATCH] mesa: Define helper function to get the number of texture layers.

[Francisco Jerez]

Paul Berry <stereotype441 at gmail.com> writes:
>[...]
> 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.
>
> 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:
>
> 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.
>
> 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.
>

Doesn't this increase the probability of the bug never getting reported?
I'm starting to think that it might be preferable to do neither of the
previous proposals and simply print a sensible error message and abort,
both in debug and release builds.

> 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.
>
> _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().
>
> 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.
>
>
> 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:
>
> switch () {
> case:
>    ...
> default:
>    assert(!"Unexpected switch value");
>    return SENSIBLE_DEFAULT;
> }
>
> 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.

Your assumption that the cases where returning a "sensible" default
leads to a sensible outcome are any less extraordinarily rare than
__builtin_unreachable() increasing performance seems surprising to me.
I don't think that there is any practical interest in any of both
choices, the probabilities of any of them helping more than the other
may well be negligible.  This whole discussion seems of purely
intellectual and aesthetic interest to me and I think it would be a
waste of time to keep dragging it out.

From now on I'll try to do without the unreachable() macro if that
avoids falling into this discussion again, but I'm not planning on going
back and replacing past uses with anything else because I don't think
it's worth my [or anyone's] time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131211/ad14e2aa/attachment.pgp>