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

[Brian Paul]

On 12/10/2013 03:27 PM, Francisco Jerez wrote:
> Brian Paul <brianp at vmware.com> writes:
>
>> On 12/10/2013 11:06 AM, Francisco Jerez wrote:
>>> Paul Berry <stereotype441 at gmail.com> writes:
>>>
>>>> On 10 December 2013 08:42, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>
>>>>> Brian Paul <brianp at 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