On 08.09.2011 14:27, Joerg Sonnenberger wrote:
> On Thu, Sep 08, 2011 at 02:06:29PM +0200, Jean-Yves Migeon wrote:
>> I can't expect the kernel to be in a good shape when it calls panic(9)
>> so allocating/copying strings is not possible. So it has to be done at
>> compile time, or the use of panicstr has to be changed.
>
> There is no reason to allocate a string, just reserve e.g. 160 Bytes or
> so and use snprintf to that. But yes, the current usage is suboptimal.
So let's make it a little bit more helpful: see panicstr.diff.
Summary about my discussion regarding KASSERTMSG(): in its current form
it cannot append the msg passed as argument to it to the panic format
string, which makes it rather impractical: we fail to log the file,
line, expression that triggered the assertion and worse, it does not
conform to its description in KASSERT(9).
It would be very practical to append "msg" to the panic string while
keeping its usefulness: "msg" can specify a format string with optional
arguments (helpful when you are interested in more information that the
assertion alone).
Joerg proposed to introduce a panic_verbose(9) function which is a
panic(9) function enhanced with additional arguments:
panic_verbose(__FILE__, __LINE__, #e, fmt, ##__VA_ARGS__)
This needs to (re)write the string parsing parts of panic(9) to cope
with the additional arguments. I can't simply add a wrapper around
panic(9) with a scratch string for that, concurrent callers could
completely thrash it (due to preemptions etc).
I am not really convinced that we should touch panic(9) to provide
KASSERTMSG() functionality. IMHO we cannot predict future uses and
adapting the prototype each time is cumbersome, especially as
KASSERTMSG(...) isn't widely used throughout the kernel.
Having two format strings in the prototype is a possibility (pointed by
Mouse). va_* plays is something I'd like to keep to the kprintf(9)
functions though.
So I am back with the __VA_ARGS__ tricks; with the attached patch and
all the KASSERT call sites fixed, the ALL kernel size is actually
smaller... ?
# vanilla
16862719 Sep 8 16:26
/home/jym/cvs/src/../obj/sys/arch/i386/compile/ALL/netbsd
# the one with KASSERTMSG patch and fixed call sites
16838041 Sep 8 17:51
/home/jym/cvs/src/../obj/sys/arch/i386/compile/ALL/netbsd
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Index: sys/kern/subr_prf.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_prf.c,v
retrieving revision 1.141
diff -u -p -r1.141 subr_prf.c
--- sys/kern/subr_prf.c 17 Jul 2011 20:54:52 -0000 1.141
+++ sys/kern/subr_prf.c 8 Sep 2011 13:31:50 -0000
@@ -203,6 +203,7 @@ panic(const char *fmt, ...)
struct cpu_info *ci, *oci;
int bootopt;
va_list ap;
+ static char scratchstr[160]; /* stores panic message */
spldebug_stop();
@@ -242,18 +243,23 @@ panic(const char *fmt, ...)
} else
printf("Skipping crash dump on recursive panic\n");
- if (!panicstr)
- panicstr = fmt;
doing_shutdown = 1;
if (msgbufenabled && msgbufp->msg_magic == MSG_MAGIC)
panicstart = msgbufp->msg_bufx;
- va_start(ap, fmt);
printf("panic: ");
- vprintf(fmt, ap);
- printf("\n");
+ va_start(ap, fmt);
+ if (panicstr == NULL) {
+ /* first time in panic */
+ panicstr = scratchstr;
+ vsnprintf(scratchstr, sizeof(scratchstr), fmt, ap);
+ printf("%s", scratchstr);
+ } else {
+ vprintf(fmt, ap);
+ }
va_end(ap);
+ printf("\n");
if (msgbufenabled && msgbufp->msg_magic == MSG_MAGIC)
panicend = msgbufp->msg_bufx;
Index: sys/lib/libkern/libkern.h
===================================================================
RCS file: /cvsroot/src/sys/lib/libkern/libkern.h,v
retrieving revision 1.99
diff -u -p -u -p -r1.99 libkern.h
--- sys/lib/libkern/libkern.h 1 Sep 2011 22:35:17 -0000 1.99
+++ sys/lib/libkern/libkern.h 8 Sep 2011 15:58:19 -0000
@@ -174,15 +174,17 @@ tolower(int ch)
#define __NULL_STMT do { } while (/* CONSTCOND */ 0)
+#define ASSERTSTR "kernel %sassertion \"%s\" failed: file \"%s\", line %d "
+
#ifdef NDEBUG /* tradition! */
#define assert(e) ((void)0)
#else
#ifdef __STDC__
#define assert(e) (__predict_true((e)) ? (void)0 :
\
- kern_assert("", __FILE__, __LINE__, #e))
+ panic(ASSERTSTR, "", #e, __FILE__, __LINE__))
#else
#define assert(e) (__predict_true((e)) ? (void)0 :
\
- kern_assert("", __FILE__, __LINE__, "e"))
+ panic(ASSERTSTR, "", "e", __FILE__, __LINE__))
#endif
#endif
@@ -197,46 +199,50 @@ tolower(int ch)
#ifndef DIAGNOSTIC
#define _DIAGASSERT(a) (void)0
#ifdef lint
-#define KASSERTMSG(e, msg) /* NOTHING */
+#define KASSERTMSG(e, msg, ...) /* NOTHING */
#define KASSERT(e) /* NOTHING */
#else /* !lint */
-#define KASSERTMSG(e, msg) ((void)0)
+#define KASSERTMSG(e, msg, ...) ((void)0)
#define KASSERT(e) ((void)0)
#endif /* !lint */
#else /* DIAGNOSTIC */
#define _DIAGASSERT(a) assert(a)
-#define KASSERTMSG(e, msg) do { \
- if (__predict_false(!(e))) \
- panic msg; \
- } while (/*CONSTCOND*/ 0)
+#define KASSERTMSG(e, msg, ...) \
+ (__predict_true((e)) ? (void)0 : \
+ panic(ASSERTSTR msg, "diagnostic ", #e, \
+ __FILE__, __LINE__, ## __VA_ARGS__))
#ifdef __STDC__
#define KASSERT(e) (__predict_true((e)) ? (void)0 :
\
- kern_assert("diagnostic ", __FILE__, __LINE__, #e))
+ panic(ASSERTSTR, "diagnostic ", #e, \
+ __FILE__, __LINE__))
#else
#define KASSERT(e) (__predict_true((e)) ? (void)0 :
\
- kern_assert("diagnostic ", __FILE__, __LINE__,"e"))
+ panic(ASSERTSTR, "diagnostic ", "e", \
+ __FILE__, __LINE__))
#endif
#endif
#ifndef DEBUG
#ifdef lint
-#define KDASSERTMSG(e,msg) /* NOTHING */
+#define KDASSERTMSG(e,msg, ...) /* NOTHING */
#define KDASSERT(e) /* NOTHING */
#else /* lint */
-#define KDASSERTMSG(e,msg) ((void)0)
+#define KDASSERTMSG(e,msg, ...) ((void)0)
#define KDASSERT(e) ((void)0)
#endif /* lint */
#else
-#define KDASSERTMSG(e, msg) do { \
- if (__predict_false(!(e))) \
- panic msg; \
- } while (/*CONSTCOND*/ 0)
+#define KDASSERTMSG(e, msg, ...) \
+ (__predict_true((e)) ? (void)0 : \
+ panic(ASSERTSTR msg, "debugging ", #e, \
+ __FILE__, __LINE__, ## __VA_ARGS__))
#ifdef __STDC__
#define KDASSERT(e) (__predict_true((e)) ? (void)0 :
\
- kern_assert("debugging ", __FILE__, __LINE__, #e))
+ panic(ASSERTSTR, "debugging ", #e, \
+ __FILE__, __LINE__))
#else
#define KDASSERT(e) (__predict_true((e)) ? (void)0 :
\
- kern_assert("debugging ", __FILE__, __LINE__, "e"))
+ panic(ASSERTSTR, "debugging ", "e", \
+ __FILE__, __LINE__))
#endif
#endif
/*
