Abuse of unreachable()

[kostja]

unreachable() was added to help compiler determine that a branch can not be reached. Unfortunately, unreachable() in release builds is a noop, and it's being used in all cases when the programmer thinks that a branch is unreachable, not when it really is.

unreachable() should lead to panic() in production, otherwise it's a security time bomb.

[alyapunov]

I don't like the idea.

1. unreachable() as a macro for __builtin_unreachable() has different semantics with exit(..), abort(), panic(..) etc. Apart from them unreachable() tells compiler that the situation is impossible. Having a line if (expr) __builtin_unreachable(); a compiler may assume as a fact that expr is false and use the fact below and even above (!) in the code.

2. BTW unreachable() in release build is not a noop. It is generally undefined but actually it's just never called. switch..case..default: unreachable(); can get SIGSEGV immediately on wrong value, not just leave some values in undefined state.

3. We need unreachable() to tell compiler about impossible situation. There are optimizations that cannot be made without true unreachable(). And if you really make unreachable() implementation to do something it will immediately become 'reachable', and even if you leave it noreturn not all the optimizations will be applied.

4. We can only allow unreachable() do something in debug build - nobody cares about optimizations in it. Actually it already has assert(0);. As for me I don't like it either - the semantics is changes, that must be another method, asssert_unreachable() idk.

5. We have unreachable(), assert(..), panic(..) as different tools as it should be. We should not just replace all 'unreachables' (and 'asserts') with panic - we must use panic in those places where execution can ever reach. We must use corresponding tools for corresponding actions, not one tool for everything.

6. Definitely there are places that must be fixed asap. I've just found a code:

default:
    *key_len = 0;
    unreachable();
    return NULL;

We all must understand that in release neither *key_len = 0; nor return NULL; are not even have compiled code. That's a wrong tool in a wrong place.

Sorry for many words but I wanted to explain why:

1. We should leave unreachable() alone.
2. Our team must use proper tools.
3. We must inspect our code about unreachable() (and even assert()) usage. At least the code in the example above and entire c89adef (another example of that replacing of one tool with another is a bad idea).

[Totktonada]

As fourth point I would propose to hold where each of those instruments should be used in our C Style Guide.

[kyukhin]

As fourth point I would propose to hold where each of those instruments should be used in our C Style Guide.

Fair enough. Feel free to submit a doc issue (or even PR). I totally agree with all points.