Date: Tue, 19 Mar 2024 08:57:28 +0800 From: Kristof Provost <kp@FreeBSD.org> To: "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net> Cc: hackers@freebsd.org Subject: Re: Why I dislike MPASS... Message-ID: <E92DF0F3-54E4-4F08-ADE7-551C94F3C4FA@FreeBSD.org> In-Reply-To: <57o4rnnq-013s-3nsn-59n5-4ssn1pq81s94@yvfgf.mnoonqbm.arg> References: <57o4rnnq-013s-3nsn-59n5-4ssn1pq81s94@yvfgf.mnoonqbm.arg>
next in thread | previous in thread | raw e-mail | index | archive | help
--=_MailMate_FA3B4B96-0ECB-4EA5-937A-ECEE45C6FE33_= Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 --=_MailMate_FA3B4B96-0ECB-4EA5-937A-ECEE45C6FE33_= Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html> <head> <meta http-equiv=3D"Content-Type" content=3D"text/xhtml; charset=3Dutf-8"= > </head> <body><div style=3D"font-family: sans-serif;"><div class=3D"markdown" sty= le=3D"white-space: normal;"> <p dir=3D"auto">On 19 Mar 2024, at 8:46, Bjoern A. Zeeb wrote:</p> </div><div class=3D"plaintext" style=3D"white-space: normal;"><blockquote= style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136= BCE; color: #136BCE;"><p dir=3D"auto">Hi,</p> <p dir=3D"auto">needless to say, this needed a 2nd kernel, a reproducer (= when no gdb and <br> no crash dump was avail).</p> <p dir=3D"auto">XXX-BZ KABOOM 45000 > 0 && 45000 <=3D 360 &= amp;& 45000 < 1536 <br> panic: Assertion (key.to) > 0 && (key.to) <=3D w_max_used_i= ndex && (key.to) < witness_count failed at /usr/src/sys/kern/s= ubr_witness.c:3118</p> <p dir=3D"auto">This is why I dislike MPASS; if it hits you are often no = wiser as to <br> why, especially if people added more than a single condition.</p> <p dir=3D"auto">Why do we make our life harder ourselves by not having in= formative <br> messages (I mean that is what the comment above MPASS claims)?</p> <p dir=3D"auto">I still have no idea how I get to that assertion but at l= east key.to <br> looks dodgy enough so that I can go look where-as before I really didn't <br> know if I had hit a limit or which one...</p> <br></blockquote></div> <div class=3D"markdown" style=3D"white-space: normal;"> <p dir=3D"auto">Presumably you=E2=80=99d prefer something that=E2=80=99d = log the actual compared values?<br> E.g. something like <code style=3D"padding: 0 0.25em; background-color: #= E4E4E4;">ASSERT_GT(key.to, 0); ASSERT_LTE(key.to, w_max_used_index); ASSE= RT_LT(key.to, witness_count);</code>?<br> I vaguely recall seeing discussion of similar constructs in the ZFS code.= </p> <p dir=3D"auto">I=E2=80=99d enthusiastically support adding, and encourag= ing the use of, macros like that.</p> <p dir=3D"auto">I=E2=80=99d object to removing or discouraging MPASS(), b= ecause I fear that will reduce the number of assertions developers add to= the code, and we=E2=80=99d be objectively worse off in a world where the= assertion was <code style=3D"padding: 0 0.25em; background-color: #E4E4E= 4;"> </code> rather than <code style=3D"padding: 0 0.25em; background-col= or: #E4E4E4;">MPASS((key.to) > 0 && (key.to) <=3D w_max_use= d_index && (key.to) < witness_count);</code>.</p> <p dir=3D"auto">We should probably encourage people to avoid complex expr= essions in MPASS(), but I=E2=80=99d still much rather have the complex ex= pression than nothing at all.</p> <p dir=3D"auto">Best regards,<br> Kristof</p> </div> </div> </body> </html> --=_MailMate_FA3B4B96-0ECB-4EA5-937A-ECEE45C6FE33_=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E92DF0F3-54E4-4F08-ADE7-551C94F3C4FA>