Re: [PATCH] docs/misra: add R13.2 and R18.2 to rules.rst

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.08.2024 19:59, Stefano Stabellini wrote:
> On Thu, 1 Aug 2024, Jan Beulich wrote:
>> On 01.08.2024 01:50, Stefano Stabellini wrote:
>>> On Wed, 31 Jul 2024, Jan Beulich wrote:
>>>> On 31.07.2024 01:30, Stefano Stabellini wrote:
>>>>> --- a/docs/misra/rules.rst
>>>>> +++ b/docs/misra/rules.rst
>>>>> @@ -462,6 +462,15 @@ maintainers if you want to suggest a change.
>>>>>       - Initializer lists shall not contain persistent side effects
>>>>>       -
>>>>>  
>>>>> +   * - `Rule 13.2 
>>>>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_13_02.c>`_
>>>>> +     - Required
>>>>> +     - The value of an expression and its persistent side-effects shall
>>>>> +       be the same under all permitted evaluation orders
>>>>> +     - Be aware that the static analysis tool Eclair might report
>>>>> +       several findings for Rule 13.2 of type "caution". These are
>>>>> +       instances where Eclair is unable to verify that the code is valid
>>>>> +       in regard to Rule 13.2. Caution reports are not violations.
>>>>
>>>> Which doesn't make clear what our take is towards new code people may
>>>> submit.
>>>
>>> Good point, see my comment below
>>>
>>>
>>>>> @@ -583,6 +592,15 @@ maintainers if you want to suggest a change.
>>>>>         submitting new patches please try to decrease the number of
>>>>>         violations when possible.
>>>>>  
>>>>> +   * - `Rule 18.2 
>>>>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_18_02.c>`_
>>>>> +     - Required
>>>>> +     - Subtraction between pointers shall only be applied to pointers
>>>>> +       that address elements of the same array
>>>>> +     - Be aware that the static analysis tool Eclair might report
>>>>> +       several findings for Rule 18.2 of type "caution". These are
>>>>> +       instances where Eclair is unable to verify that the code is valid
>>>>> +       in regard to Rule 18.2. Caution reports are not violations.
>>>>
>>>> And while the same wording is used here, I think it is pretty clear for
>>>> this that we'd reject changes where bad subtractions are used. IOW even
>>>> more so important to clarify the (possibly different) positions on what
>>>> is going to be added into the code base.
>>>
>>> In both of these cases, we would reject code that doesn't follow R13.2
>>> and R18.2.
>>
>> But we shouldn't (unconditionally) do so for for 13.2, should we?
>>
>>> I'll change it to the following:
>>>
>>>
>>> Be aware that the static analysis tool Eclair might report several
>>> findings for Rule 18.2 of type "caution". These are instances where
>>> Eclair is unable to verify that the code is valid in regard to Rule
>>> 18.2. Caution reports are not violations. Regardless, new code is
>>> expected to follow this rule.
>>
>> I'm fine with this for 18.2, but not so much for 13.2.
> 
> Let me clarify something about R13.2. I expect we are aligned on this.
> 
> Rule 13.2 only expects that "the value of an expression and its persistent
> side-effects shall be the same under all permitted evaluation orders"
> and nothing more.
> 
> It is an outstanding limitation of static analyzers such as ECLAIR
> that they cannot be certain that "the value of an expression and its
> persistent side-effects shall be the same under all permitted evaluation
> orders". So one way to make ECLAIR happy is to change this code:
> 
> 1)
> func1(param1, func2(a), func3(b);
> 
> into this code:
> 
> 2)
> param2 = func2(a);
> param3 = func3(b);
> func1(param1, param2, param3);
> 
> Rule 13.2 is not asking us to change 1) into 2). 1) is acceptable. It is
> just that ECLAIR cannot help us ensure that 1) is compliant with Rule
> 13.2. It is totally fine to accept new code written in the form 1), of
> course only if "the value of an expression and its persistent
> side-effects shall be the same under all permitted evaluation orders".
> It would likely increase the number of ECLAIR cautions, but it is not
> necessarily a problem, and the ECLAIR Gitlab job will not fail.
> 
> If one of the reviewers discovers that 1) doesn't comply with Rule 13.2
> due to manual review, then they should ask the contributor to change the
> code. That is a good idea because we wouldn't want the value of an
> expression to be dependant on the evaluation order which GCC cannot
> guarantee.

Okay, if that is our interpretation of the rule for practical purposes,
then I'm no longer concerned.

Jan