tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
marking kern_assert(9) as __dead, and recursive panics
I would like to declare kern_assert() as __dead, so that static
analysers can understand that code after a failed KASSERT is never
executed.
However, kern_assert returns without panicing if panicstr != NULL
(that is, if a panic has already occurred), so gcc will not allow
it to be declared __dead.
kern_assert's predecessor, __assert(), used to be declared
with __attribute__((__noreturn__)), but that was removed in
revision 1.70 of sys/lib/libkern/libkern.h and revision 1.9
of sys/lib/libkern/__assert.c. A similar change was made to
assert_sleepable() in kern_lock.c at the same time. The relevant
commits were these:
2007-07-29 11:45 ad
* src/sys/kern/kern_lock.c (1.117):
Be more forgiving if panicstr != NULL.
2007-07-29 11:46 ad
* src/sys/lib/libkern/: __assert.c (1.9), libkern.h (1.70):
Disable kernel assertions if panicstr != NULL.
2007-08-03 13:06 ad
* src/sys/lib/libkern/__assert.c (1.10):
Do the panicstr check only if _KERNEL.
I propose to to do the following:
* Keep the panicstr test in assert_sleepable() in sys/kern/kern_lock.c,
so it continues to degenerates to a no-op after a panic. I am not
sure whether this behaviour is necessary, but I see no reason to
change the status quo here.
* Remove the panicstr test from kern_assert() in
sys/lib/libkern/kern_assert.c, so that KASSERT, KASSERTMSG and
friends do not degenerate to no-ops after a panic.
I don't know a reason for making all kernel asserts degenerate
to no-ops, but I imagine that it might have been a workaround
for problems with recursive panics, and I propose to address
recursive panics directly (see below).
I can also imagine that there are particular kernel asserts
that need to degenerate to no-ops after a panic, and I suggest
explicitly rewriting them in terms of (panicstr != NULL ||
<other tests>). I have not attempted to identify such asserts.
* Change the declaration of kern_assert() in sys/lib/libkern/libkern.h to
void kern_assert(const char *, ...) __dead __printflike(1, 2);
so that static analysers will know that it never returns.
* Change db_panic() in ddb/db_panic.c so that a panic while the system
is already in DDB will always re-enter DDB, regardless of the
value of the db_onpanic kernel variable (which corresponds to the
ddb.onpanic sysctl variable).
* Change db_command_loop() in ddb/db_command.c so that a panic while
executing the commands stored in the db_cmd_on_enter kernel variable
(which corresponds to the ddb/commandonenter sysctl variable) will
not cause an infinite loop.
The end result should be that, after a panic has occurred, an
assertion failure will be treated like any other recursive panic,
and either drop into the debugger, or halt or reboot immediately,
without syncing the disks.
I append a patch to do all that (except for asserts that might
need to be changed to explicitly test panicstr).
--apb (Alan Barrett)
Index: sys/ddb/db_command.c
===================================================================
--- sys/ddb/db_command.c 6 Jan 2013 04:17:27 -0000 1.142
+++ sys/ddb/db_command.c 10 Feb 2013 15:37:12 -0000
@@ -535,6 +535,7 @@ db_command_loop(void)
{
label_t db_jmpbuf;
label_t *savejmp;
+ static bool db_processing_cmd_on_enter = false;
/*
* Initialize 'prev' and 'next' to dot.
@@ -554,7 +555,13 @@ db_command_loop(void)
(void) setjmp(&db_jmpbuf);
/* Execute default ddb start commands */
- db_execute_commandlist(db_cmd_on_enter);
+ if (db_processing_cmd_on_enter) {
+ db_printf("Fault during db_cmd_on_enter; aborting...\n");
+ } else {
+ db_processing_cmd_on_enter = true;
+ db_execute_commandlist(db_cmd_on_enter);
+ }
+ db_processing_cmd_on_enter = false;
(void) setjmp(&db_jmpbuf);
while (!db_cmd_loop_done) {
Index: sys/ddb/db_panic.c
===================================================================
--- sys/ddb/db_panic.c 10 Feb 2013 11:04:20 -0000 1.1
+++ sys/ddb/db_panic.c 10 Feb 2013 15:37:12 -0000
@@ -40,9 +40,22 @@ __KERNEL_RCSID(0, "$NetBSD: db_panic.c,v
void db_panic(void)
{
- if (db_onpanic == 1)
- Debugger();
- else if (db_onpanic >= 0) {
+ /*
+ * Do what db_onpanic requires.
+ * Valid settings are documented in ddb(4):
+ * -1: No traceback, do not enter debugger.
+ * 0: Print traceback, do not enter debugger.
+ * 1: No traceback, enter debugger.
+ * 2: Print traceback, enter debugger.
+ * other positive values: documented to enter debugger;
+ * in this implementation, they are treated the same as 2.
+ * other negative values: documented not to enter debugger;
+ * in this implementation, they are treated the same as -1.
+ *
+ * Also, if DDB was already active (db_recover != 0), then
+ * re-enter the debugger regardless of the value of db_onpanic.
+ */
+ if (db_onpanic >= 0 && db_onpanic != 1) {
static int intrace = 0;
if (intrace == 0) {
@@ -57,8 +70,8 @@ void db_panic(void)
intrace = 0;
} else
printf("Faulted in mid-traceback; aborting...");
- if (db_onpanic == 2)
- Debugger();
}
+ if (db_onpanic >= 1 || db_recover != 0)
+ Debugger();
return;
}
Index: sys/lib/libkern/kern_assert.c
===================================================================
--- sys/lib/libkern/kern_assert.c 29 Sep 2011 20:50:09 -0000 1.2
+++ sys/lib/libkern/kern_assert.c 10 Feb 2013 15:37:12 -0000
@@ -42,10 +42,6 @@ void
kern_assert(const char *fmt, ...)
{
va_list ap;
-#ifdef _KERNEL
- if (panicstr != NULL)
- return;
-#endif
va_start(ap, fmt);
vpanic(fmt, ap);
va_end(ap);
Index: sys/lib/libkern/libkern.h
===================================================================
--- sys/lib/libkern/libkern.h 30 Aug 2012 12:16:49 -0000 1.106
+++ sys/lib/libkern/libkern.h 10 Feb 2013 15:37:12 -0000
@@ -300,8 +300,7 @@ int ffs(int);
#define ffs(x) __builtin_ffs(x)
#endif
-void kern_assert(const char *, ...)
- __attribute__((__format__(__printf__, 1, 2)));
+void kern_assert(const char *, ...) __dead __printflike(1,2);
unsigned int
bcdtobin(unsigned int);
unsigned int
Home |
Main Index |
Thread Index |
Old Index