kernel: provide panicky version of __unreachable

[kevans]

Summary

__builtin_unreachable doesn't raise any compile-time warnings/errors on its own, so problems with its usage can't be easily detected. While it would be nice for this situation to change and compilers to at least add a warning for trivial cases where local state means the instruction can't be reached, this isn't the case at the moment.

This commit adds an assert_unreachable, whose intend is incredibly clear: it asserts that this instruction is unreachable. On INVARIANTS builds, it's a panic(), and on non-INVARIANTS it's unreachable().

[pfg]

Ugh.. I think this was discussed when __unreachable() was introduced (D2536 perhaps), and back then it was not considered a good idea.

The idea of __unreachable() is/was exclusively for static checkers and compiler optimizations. There was some rule against adding assertions here unless we are specifically building code in "debug mode".

[kevans]

In D23793#522846, @pfg wrote:

Ugh.. I think this was discussed when __unreachable() was introduced (D2536 perhaps), and back then it was not considered a good idea.

The idea of __unreachable() is/was exclusively for static checkers and compiler optimizations. There was some rule against adding assertions here unless we are specifically building code in "debug mode".

Indeed, said discussion partially drove this. Right now __unreachable has been deployed in some questionable ways that leave it hard to detect actual problems; INVARIANTS is, I think, a good enough analogy for "debug mode" in the kernel.

[kevans]

Provide an __asserts_unreachable instead; this leaves sys/cdefs.h unmolested and provides a name that makes it clear that it functions like other assertions.

[mjg]

sys/sys/systm.h

120 ↗

(On Diff #68690)

this really does not tell you which is one is it and this call will not even print a backtrace. iow this really wants to be panic("executing segment marked as unreachable at %s (%s:%s)", func, FILE, LINE); or so

otherwise lgtm

[kevans]

sys/sys/systm.h

120 ↗

(On Diff #68690)

Whoops, lost that in an iteration where I converted it from a kassert_panic("Unreachable segment reached\n")) to a panic - will fix.

[kevans]

Include file/line/function in panic string in a similar format to the others in the area.

[theraven]

sys/net/mppcc.c

237 ↗

(On Diff #68692)

Why is there unreachable code here?

[kevans]

sys/net/mppcc.c

237 ↗

(On Diff #68692)

Sorry, I overlooked this comment. =-( I have no familiarity with this, but I guess the author decided it's OK to proceed with the caller seeing this as just a failure case. I think this is part of the problem with the status quo; we don't currently have a way to detect when something's reaching bits like this that are clearly assumed to be unreachable, and they'll plow on with looking like some other failure case as a result.

Maybe this is OK on stable branches since the author's clearly designed it to continue working if their assumption is invalidated, but IMO we should be doing better on -CURRENT at least.