Re: SAF-x-safe rename

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 16 Oct 2023, Jan Beulich wrote:
> On 06.10.2023 00:01, Andrew Cooper wrote:
> > On 05/10/2023 10:38 pm, andrew.cooper3@xxxxxxxxxx wrote:
> >> On 05/10/2023 8:43 am, Luca Fancellu wrote:
> >>>> On 5 Oct 2023, at 00:46, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> >>>> wrote:
> >>>>
> >>>> Hi MISRA C working group (Jan, Roger, Andrew, Julien, Bertrand, George)
> >>>>
> >>>> in a recent thread Andrew pointed out that the SAF-2-safe tag is
> >>>> confusing and requested a rename:
> >>>> https://marc.info/?l=xen-devel&m=169634970821202
> >>>>
> >>>> As documented by docs/misra/documenting-violations.rst:
> >>>>
> >>>> - SAF-X-safe: This tag means that the next line of code contains a 
> >>>> finding, but
> >>>>   the non compliance to the checker is analysed and demonstrated to be 
> >>>> safe.
> >>>> - SAF-X-false-positive-<tool>: This tag means that the next line of code
> >>>>   contains a finding, but the finding is a bug of the tool.
> >>>>
> >>>>
> >>>> Today we have already 28 instances of SAF tags in the Xen codebase.
> >>>>
> >>>>
> >>>> Andrew suggested "ANALYSIS" instead of SAF so I would imagine:
> >>>> - ANALYSIS-X-safe
> >>>> - ANALYSIS-X-false-positive-<tool>
> >>>>
> >>>> If we really want a rename I suggest to rename SAF to SAFE:
> >>>> - SAFE-X-safe
> >>>> - SAFE-X-false-positive-<tool>
> >>>>
> >>>> Or maybe MISRA:
> >>>> - MISRA-X-safe
> >>>> - MISRA-X-false-positive-<tool>
> >>>>
> >>>> But I actually prefer to keep the tag as it is today.
> >>> We chose a generic name instead of MISRA because the tag can potentially 
> >>> suppress findings
> >>> of any checker, including MISRA checker.
> >>>
> >>> If SAF-* is confusing, what about FUSA-* ?
> >>>
> >>> Anyway I’m thinking that every name we could come up will be confusing at 
> >>> first, improving the
> >>> documentation would mitigate it (by improving I mean to improve the 
> >>> fruition of it, for example a
> >>> Read the docs documentation has the search bar, a quick copy paste of 
> >>> SAF- would make the
> >>> documenting-violations page visible.)
> >>
> >> No - this is a problem *because* changing the documentation doesn't
> >> help.   (To be clear, updating the documentation is fine, but irrelevant
> >> here.)
> >>
> >>
> >> These are annotations in code.  They need to be:
> >>
> >> 1) Short (obviously)
> >> 2) Clear to someone who isn't you (the collective us of this group)
> >> reading the code.
> >> 3) Non-intrusive, so as not to get in the way of the code.
> >>
> >> and they must be all three.  This was even a principle given at the
> >> start of the MISRA work that we would not be deteriorating the quality
> >> of the code just to comply.
> >>
> >> Point 3 is other thread about end-of-line, or block regions.  Lets leave
> >> that there because it's really a metadata transformation problem
> >> constrained by where the comments can acceptably go.
> >>
> >>
> >> Point 2 is the issue here, and "SAF-$N-safe" scores very highly on the
> >> WTF-o-meter *even* for people who know that it's something related to 
> >> MISRA.
> >>
> >> Seriously it looks like someone couldn't spell, and everyone else went
> >> with it (reflects poorly on everyone else).
> >>
> >> And yes, I know it's an initialisation for something, but it's not even
> >> an industry standard term - it's a contraction of an intentionally
> >> generic phrase, with substantial irony on an early MISRA call where
> >> there was uncertainly between people as to what it even stood for.
> >>
> >> These are the thoughts running through the minds of people reading the
> >> code when they don't understand what they're looking at.
> >>
> >>
> >> Annotations for other static analysers intentionally use their own name
> >> so they're googleable.
> >>
> >> Guess what SAF googles for?  Sustainable Aviation Fuel, or Specialist
> >> Automotive Finance.
> >>
> >> Fine, lets be more specific.  How about "Xen SAF" ?  Nope...
> >>
> >> "Did you mean to search for:
> >> Xen SAVE Xen SAN Xen VIF Xenstaff"
> >>
> >>
> >> Despite many of the search results referencing patches, or rendered
> >> documents out of docs/, not a single one of them gets
> >> documenting-violations.rst in any form, where the single definition of
> >> this term is hiding in a paragraph which spends 90% of it's volume
> >> describing a monotonically increasing number.
> >>
> >> Seriously, ChatGPT would struggle to make shit this good up.
> >>
> >>
> >> The thing we tag with *must* be trivially recognisable as an analysis
> >> tag in order for others to be able to read the code.  Therefore, it
> >> needs to be an actual full world (hence the ANALYSIS suggestion), or an
> >> industry standard term (where MISRA does qualify).
> 
> ANALYSIS imo gets in conflict with 1) above, considering that permitting
> line length violations was already brought up with the shorter SAF-*.
> 
> >> I don't exactly what it is - something else might turn out to be even
> >> better, but it is very important to be not this, for everyone else to
> >> have an easy time reading the code.
> >>
> >>
> >> And reasoning along that line...  What's wrong with just /* octal-ok */
> >> or /* womble-permitted */ so it's also apparent in context what the
> >> contentious issue might be and why it might be mitigated?
> 
> When the numbering scheme was discussed, I was asking why a shorthand
> for the issue to deal with (kind of a tag) can't be used. I don't
> recall any arguments, but here we are. One issue with something like
> just /* octal-ok */ is that it's not sufficiently reliably machine-
> parseable; that's aiui where the desire of the SAF (or whatever else)
> prefix came from.
> 
> >> The mechanics behind the scenes is just a trivial text replacement, and
> >> the tagging scheme does not have to uniform obfuscated identifier for
> >> that to work.
> > 
> > Or, as has been pointed out to me in private, even
> > 
> > /* RULE-$N-safe */
> > 
> > would be an improvement because it's clearly related to some set of
> > guidelines.
> 
> Both MISRA and RULE are Misra-specific; RULE, if you mean it to be
> followed by the rule number, would even be Misra version specific.

Please everyone fill in your preference here in advance of tomorrow's
MISRA C meeting so we can discuss the results live:
https://cryptpad.fr/form/#/2/form/view/kwflzAkvxhxF5U5Kv9O6QiQ5LEuCmHZlJhnNda7jk2g/