On 08.09.2011 19:38, Joerg Sonnenberger wrote:
> On Thu, Sep 08, 2011 at 06:41:54PM +0200, Jean-Yves Migeon wrote:
>> On 08.09.2011 14:27, Joerg Sonnenberger wrote:
>>> 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.
>
> I would change one minor aspect: set panicstr to fmt before calling
> va_start etc, to give at least some basic information in case that fails
> for any reason. Otherwise I think that should go in, it already improves
> the status quo. We can discuss the rest after that.
Bringing back the thing again; quoting a previous mail from me:
> 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. This would need a specific format specifier for va_list
> also.
>
> 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... [by about 20k]
I am attaching the patch I wrote for the proposal above (note attaching
the patch for call sites, it's worthless for now)
Comments are welcomed, I'd like to improve the KASSERTMSG(...) macro a
bit. Currently, it's just a mere alias for an if + panic(9).
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Index: sys/lib/libkern/libkern.h
===================================================================
RCS file: /cvsroot/src/sys/lib/libkern/libkern.h,v
retrieving revision 1.99
diff -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 16 Sep 2011 22:44:22 -0000
@@ -37,6 +37,7 @@
#include <sys/types.h>
#include <sys/inttypes.h>
#include <sys/null.h>
+#include <sys/systm.h>
#ifndef LIBKERN_INLINE
#define LIBKERN_INLINE static __inline
@@ -174,15 +175,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 +200,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
/*
