Re: Why I dislike MPASS...

On Tue, 19 Mar 2024, Kristof Provost wrote:

> 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?

Yes, because without them you still don't know what's wrong.


> 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.

Yes, MPASS should probably only be used for a single simple expression
the most.

And I am all for assertions as they are great documentation of assumption
in code as well.  It's the first stage of writing test code as well ;-)

But using KASSERT with a proper message which isn't a teapot is too hard and
too time consuming?

Always think of when you get a PR from a user (not a machine you sit in
front of) what can you do with this information and what's the first
things you'd love to know.

I often find that the latter changes as code changes or one has a
better understanding of a problem (depending on the complexity of the
problem).  But I also always find that if I didn't have the initial
extra information I would have to ask for it often by guessing the best
three things...  or if ddb is avail at least can have something looked
up  "type show ifnet <values from message> and send that output along".

/bz


-- 
Bjoern A. Zeeb                                                     r15:7