Re: Why I dislike MPASS...

On 19 Mar 2024, at 8:46, Bjoern A. Zeeb wrote:
> Hi,
>
> needless to say, this needed a 2nd kernel, a reproducer (when no gdb 
> and
> no crash dump was avail).
>
> XXX-BZ KABOOM 45000 > 0 && 45000 <= 360 && 45000 < 1536
> panic: Assertion (key.to) > 0 && (key.to) <= w_max_used_index && 
> (key.to) < witness_count failed at 
> /usr/src/sys/kern/subr_witness.c:3118
>
> This is why I dislike MPASS; if it hits you are often no wiser as to
> why, especially if people added more than a single condition.
>
> Why do we make our life harder ourselves by not having informative
> messages (I mean that is what the comment above MPASS claims)?
>
> I still have no idea how I get to that assertion but at least key.to
> looks dodgy enough so that I can go look where-as before I really 
> didn't
> know if I had hit a limit or which one...
>
Presumably you’d prefer something that’d log the actual compared 
values?
E.g. something like `ASSERT_GT(key.to, 0); ASSERT_LTE(key.to, 
w_max_used_index); ASSERT_LT(key.to, witness_count);`?
I vaguely recall seeing discussion of similar constructs in the ZFS 
code.

I’d enthusiastically support adding, and encouraging the use of, 
macros like that.

I’d object to removing or discouraging MPASS(), because I fear that 
will reduce the number of assertions developers add to the code, and 
we’d be objectively worse off in a world where the assertion was ` ` 
rather than `MPASS((key.to) > 0 && (key.to) <= w_max_used_index && 
(key.to) < witness_count);`.

We should probably encourage people to avoid complex expressions in 
MPASS(), but I’d still much rather have the complex expression than 
nothing at all.

Best regards,
Kristof