Re: Proposal for deviations in static analyser findings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.10.2022 09:52, Bertrand Marquis wrote:
>> On 19 Oct 2022, at 07:38, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 18.10.2022 18:11, Bertrand Marquis wrote:
>>>> On 18 Oct 2022, at 16:29, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 18.10.2022 17:17, Luca Fancellu wrote:
>>>>>> On 13 Oct 2022, at 12:34, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>> On 13.10.2022 12:11, Luca Fancellu wrote:
>>>>>>> After the analysis, the source code will return as the original (with 
>>>>>>> the SAF-* tag).
>>>>>>
>>>>>> While you mention something similar also as step 3 in the original 
>>>>>> document
>>>>>> near the top, I'm afraid I don't understand what this "return as the 
>>>>>> original"
>>>>>> means. If you want to run the tool on an altered (comments modified) 
>>>>>> source
>>>>>> tree, what I'd expect you to do is clone the sources into a throw-away 
>>>>>> tree,
>>>>>> massage the comments, run the tool, and delete the massaged tree.
>>>>>>>>> if the object doesn't have a key-value, then the corresponding in-code
>>>>>>>>> comment won't be translated.
>>>>>>>>
>>>>>>>> Iirc at least Coverity ignores certain instances of what it might 
>>>>>>>> consider
>>>>>>>> violations (fall-through in switch() statements in particular) in case
>>>>>>>> _any_ comment is present. Therefore may I suggest that such comments be
>>>>>>>> deleted (really: replaced by a blank line, to maintain correct line
>>>>>>>> numbering) if there's no matching key-value pair?
>>>>>>>
>>>>>>> Yes the line won’t be altered if there is no match. This to ensure the 
>>>>>>> correct line
>>>>>>> numbering is not affected.
>>>>>>
>>>>>> "won't be altered" is the opposite of what I've been asking to consider:
>>>>>> Observing that comments _regardless_ of their contents may silence 
>>>>>> findings,
>>>>>> the suggestion is to remove comments (leaving a blank line) when there's 
>>>>>> no
>>>>>> entry for the targeted tool in the table entry.
>>>>>
>>>>> Why? The tag comment won’t do anything, it would act as a blank line from 
>>>>> the analyser
>>>>> perspective.
>>>>
>>>> The _tag_ won't do anything, but as said any _comment_ may have an effect.
>>>
>>> I am not sure I follow this one but in any case we can choose to anyway 
>>> substitute the tag with something like /* Not applicable */.
>>
>> That's still a comment, which hence may still silence a tool:
>>
>>    switch ( x )
>>    {
>>    case a:
>>        ...
>>        /* SAF-<N>-safe */
>>    case b:
>>        ...
>>        break;
>>    }
>>
>> is no different from
>>
>>    switch ( x )
>>    {
>>    case a:
>>        ...
>>        /* fall-through */
>>    case b:
>>        ...
>>        break;
>>    }
>>
>> nor
>>
>>    switch ( x )
>>    {
>>    case a:
>>        ...
>>        /* Not applicable */
>>    case b:
>>        ...
>>        break;
>>    }
>>
>> Only
>>
>>    switch ( x )
>>    {
>>    case a:
>>        ...
>>
>>    case b:
>>        ...
>>        break;
>>    }
>>
>> will make e.g. Coverity actually point out the potentially unintended
>> fall through (based on past observations). Whether that behavior is
>> fall-through-specific I don't know. If it indeed is, then maybe my
>> concern is void - in the long run I think we want to use the pseudo-
>> keyword there in all cases anyway.
> 
> We can choose the replacement comment to be something not
> considered by the tools (or even put an empty /* */).
> What we cannot do is remove the line as it would change line numbers.

Right, and hence I did say we want to zap the comment, leaving an empty
line.

> But apart from fallthrough I do not think any comment is considered by
> any tools so this should not be an issue.

Well, we can hope for that of course.

Jan