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

[Brian Paul]

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.

An assert would be much more useful since it tells us the location and 
we can get a good stack trace in gdb.

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.

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:

switch () {
case:
    ...
default:
    assert(!"Unexpected switch value");
    return SENSIBLE_DEFAULT;
}

-Brian