Page MenuHomeFreeBSD

Add KPASS macros
Needs ReviewPublic

Authored by mjg on Feb 20 2020, 2:54 PM.

Details

Reviewers
markj
kib
jhb
Summary

With an intent of being used by default instead of KASSERT.

3 variants are provided, depending on the args passed by the caller.

Sample panic:

panic: condition 1 == 16 false at fildesc_drvinit (kern_descrip.c:4391): test

Generated with:

KPASS2(1 == 16, "test");

And this:

KASSERT(_BLOCKCOUNT_WAITERS(old),
         ("%s: no waiters on %p", __func__, bc));

could be rewritten as:

KPASS3(_BLOCKCOUNT_WAITERS(old), "no waiters on %p", bc);

or so

I think this saves a lot of retyping and in general is more pleasant to use.

Note the short name. This is a clang extension which can be worked around for gcc, but I'm not doing it (gcc instead prints the full path).

Note this still does not provide anything which would automatically print passed values (in the lines of KPASS_VAL(foo, <, bar) performing foo < bar and printing both on failure along with the stringified expression.). I think adding functionality like this is a separate effort.

MPASS macros can be redefined on top of this to avoid a tree-wide sweep. Note the M thing apparently came from mutexes because the code for some reason was put there despite being really generic.

Diff Detail

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

Event Timeline

mjg created this revision.Feb 20 2020, 2:54 PM
mjg edited the summary of this revision. (Show Details)Feb 20 2020, 2:57 PM
mjg edited the summary of this revision. (Show Details)Feb 20 2020, 3:06 PM
kib added a comment.Feb 21 2020, 12:17 PM

I very much dislike this. It repeats the mistake of KASSERT() while not eliminating KASSERT.

IMO the big and perhaps the only problem of KASSERT is that it was written before C99 so it cannot use variadic args. If the () around the printing part are dropped, then we can do any additions to the printed line without adding yet another set of macros.

So if you want to spend time on this, doing the convertion of KASSERT to variadic, and dropping the braces, would be a first step IMO. Personally I am fine with the churn.

markj added a comment.Feb 21 2020, 2:59 PM
In D23774#522504, @kib wrote:

I very much dislike this. It repeats the mistake of KASSERT() while not eliminating KASSERT.

IMO the big and perhaps the only problem of KASSERT is that it was written before C99 so it cannot use variadic args. If the () around the printing part are dropped, then we can do any additions to the printed line without adding yet another set of macros.

So if you want to spend time on this, doing the convertion of KASSERT to variadic, and dropping the braces, would be a first step IMO. Personally I am fine with the churn.

I don't really care about the parens either way, but I don't want yet another assertion macro. I'd prefer to see kassert_panic() take func or LINE and FILE arguments. Existing consumers can be fixed up to avoid printing redundant info, it could be combined with the paren removal.

imp added a comment.Feb 21 2020, 3:44 PM
In D23774#522504, @kib wrote:

I very much dislike this. It repeats the mistake of KASSERT() while not eliminating KASSERT.

IMO the big and perhaps the only problem of KASSERT is that it was written before C99 so it cannot use variadic args. If the () around the printing part are dropped, then we can do any additions to the printed line without adding yet another set of macros.

So if you want to spend time on this, doing the convertion of KASSERT to variadic, and dropping the braces, would be a first step IMO. Personally I am fine with the churn.

I don't really care about the parens either way, but I don't want yet another assertion macro. I'd prefer to see kassert_panic() take func or LINE and FILE arguments. Existing consumers can be fixed up to avoid printing redundant info, it could be combined with the paren removal.

First off, I 100% agree on this point. If we fixed it, it would be little different than KASSERT.
Second, this seems identical to what you get with MPASS once you expand the args. Why do we need a new one when MPASS has been pushed to far afield in the kernel?
Third, we could do the FILE vs FILE_NAME trick with KASSERT....

So I'm not feeling the love for this..

kib added a comment.Feb 21 2020, 4:02 PM
In D23774#522504, @kib wrote:

I very much dislike this. It repeats the mistake of KASSERT() while not eliminating KASSERT.

IMO the big and perhaps the only problem of KASSERT is that it was written before C99 so it cannot use variadic args. If the () around the printing part are dropped, then we can do any additions to the printed line without adding yet another set of macros.

So if you want to spend time on this, doing the convertion of KASSERT to variadic, and dropping the braces, would be a first step IMO. Personally I am fine with the churn.

I don't really care about the parens either way, but I don't want yet another assertion macro. I'd prefer to see kassert_panic() take func or LINE and FILE arguments. Existing consumers can be fixed up to avoid printing redundant info, it could be combined with the paren removal.

I mean that if we pass VA_ARGS instead of bracketed args list, we can prepend arguments. This allows to add args that can be referenced from appends to the format string.
In fact, () removal is optional, we can stash whatever is needed and print it after panic, or extend args to kassert_panic, as you suggested.