Re: Why I dislike MPASS...

On 3/19/24 10:43, Bjoern A. Zeeb wrote:
> 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
> 
> 

Hi,

I am in total agreement about the limitations of certain assertion 
messages. In order to begin to understand _why_ an assertion failed, 
there can be no ambiguity in _how_ it failed.

I won't fight for deprecating MPASS, but the small action we can take 
today is to improve the documentation of these macros, for those who 
might read it.

Here's an attempt at some guidelines and examples:

https://reviews.freebsd.org/D44434

Suggested improvements are welcome, fully formed text preferred.

Cheers,
Mitchell