Page MenuHomeFreeBSD

kernel: provide panicky version of __unreachable
AcceptedPublic

Authored by kevans on Feb 22 2020, 6:34 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 29541

Event Timeline

kevans created this revision.Feb 22 2020, 6:34 PM
kevans added a reviewer: dim.Feb 22 2020, 7:06 PM

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

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 updated this revision to Diff 68690.Feb 22 2020, 10:43 PM
kevans edited the summary of this revision. (Show Details)

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 added inline comments.Feb 22 2020, 10:48 PM
sys/sys/systm.h
120

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 added inline comments.Feb 23 2020, 12:55 AM
sys/sys/systm.h
120

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

kevans updated this revision to Diff 68692.Feb 23 2020, 2:03 AM

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

mjg accepted this revision.Feb 23 2020, 2:07 AM
This revision is now accepted and ready to land.Feb 23 2020, 2:07 AM
theraven added inline comments.Feb 24 2020, 10:35 AM
sys/net/mppcc.c
237

Why is there unreachable code here?