Re: KASSERTMSG fix

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

On 07.09.2011 18:14, Joerg Sonnenberger wrote:
>> Hmm. This is generally done via the fmt string of panic(9). Someone
>> could also ask to have explicit timestamping and function name, the
>> "verbose panic" prototype will become rapidly ugly...
> 
> Timestamping can be done in panic itself. For the rest, it is more a
> question of consistency and ease of use. I haven't tried building a
> kernel to see what the size impact of always including them would be.

I'll do when we agree on a fix for KASSERTMSG.

>> I did not propose this for this reason: I can fix all the KASSERTMSG()
>> uses in src, but not those outside the tree. And I expect that most
>> compile time warnings here won't make much sense once the macro gets
>> changed.
> 
> I don't think the compiler warnings will degrade much, even with GCC.
> I don't think there is enough external code to justify worrying about
> it.
> 
>> This can work around the limitations elegantly though (provided all call
>> sites are fixed):
>>
>> #define KASSERTMSG(e, fmt, ...) do {            \
>>   panic(                                        \
>>     "kernel diag assert \"%s\" failed: file \"%s\", line %d; " fmt, \
>>     #e, __FILE__, __LINE__, ## __VA_ARGS__);    \
>> } while(0)
> 
> That assumes that fmt is a string literal.

Yes; I did not check all KASSERTMSG() consumers, but I'd expect this to
be the case almost everywhere.

This is the least "invasive" (for code churn) I could come up with (will
be done a bit differently for debugging/debug/"" message stuff, but the
spirit remains).

> I don't think it is easier
> than:
> 
> panic_verbose(__FILE__, __LINE__, #e, fmt, ##__VA_ARGS__);

I am confused -- you are asking for a modification (or reimplementation)
of panic(9) here, am I right? Because you won't be able to pass these
args without adapting panic first. It's close to what kern_assert() can
do, except for the variadic stuff.

> and it would allow panic to be turned into the verbose form easily.

Somewhat; there are two args specific to KASSERTs:
- its type ("diagnostic", "debug", ...) (missing here)
- the expression checked via an assertion ("e")

I find unpleasant to make one of them (the expression) appear explicitly
in a panic_verbose(...) prototype. panic() can be called for a whole lot
of reasons, where expression "e" does not necessarily carry a meaning.
Passing NULL when turning panic to its verbose form is proof of a lack
of design IMHO. Whereas FILE and LINE will always exist.

>> But do all compilers used with NetBSD support variadic macros?
> 
> The kernel is C99, any compiler not supporting that is irrelevant.

That settles the use of variadic macros.

-- 
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost