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

[Paul Berry]

On 11 December 2013 12:37, Francisco Jerez <currojerez at riseup.net> wrote:

> 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.
>

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).


>
> > 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.
>

"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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131211/f4101f00/attachment.html>