Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Stefano Stabellini]

On Fri, 27 Oct 2023, Nicola Vetrini wrote:
> As specified in rules.rst, these constants can be used
> in the code.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

Hi Nicola,

On 27/10/2023 16:11, Nicola Vetrini wrote:
> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> index 8511a189253b..8aaaa1473fb4 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>            - __emulate_2op and __emulate_2op_nobyte
>            - read_debugreg and write_debugreg
>   
> +   * - R7.1
> +     - It is safe to use certain octal constants the way they are defined
> +       in specifications, manuals, and algorithm descriptions. Such places
> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a SAF
> +       comment (see safe.json).

Reading this, it is unclear to me why we have two ways to deviate the 
rule r7.1. And more importantely, how would the developper decide which 
one to use?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Stefano Stabellini]

On Mon, 30 Oct 2023, Julien Grall wrote:
> Hi Nicola,
> 
> On 27/10/2023 16:11, Nicola Vetrini wrote:
> > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > index 8511a189253b..8aaaa1473fb4 100644
> > --- a/docs/misra/deviations.rst
> > +++ b/docs/misra/deviations.rst
> > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
> >            - __emulate_2op and __emulate_2op_nobyte
> >            - read_debugreg and write_debugreg
> >   +   * - R7.1
> > +     - It is safe to use certain octal constants the way they are defined
> > +       in specifications, manuals, and algorithm descriptions. Such places
> > +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
> > SAF
> > +       comment (see safe.json).
> 
> Reading this, it is unclear to me why we have two ways to deviate the rule
> r7.1. And more importantely, how would the developper decide which one to use?

I agree with you on this and we were discussing this topic just this
morning in the FUSA community call. I think we need a way to do this
with the SAF framework:

if (some code with violation) /* SAF-xx-safe */

This doesn't work today unfortunately. It can only be done this way:

/* SAF-xx-safe */
if (some code with violation)

Which is not always desirable. octal-ok is just an ad-hoc solution for
one specific violation but we need a generic way to do this. Luca is
investigating possible ways to support the previous format in SAF.

I think we should take this patch for now and harmonize it once SAF is
improved.

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

Hi Stefano,

On 30/10/2023 22:49, Stefano Stabellini wrote:
> On Mon, 30 Oct 2023, Julien Grall wrote:
>> Hi Nicola,
>>
>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>> index 8511a189253b..8aaaa1473fb4 100644
>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>             - __emulate_2op and __emulate_2op_nobyte
>>>             - read_debugreg and write_debugreg
>>>    +   * - R7.1
>>> +     - It is safe to use certain octal constants the way they are defined
>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>> SAF
>>> +       comment (see safe.json).
>>
>> Reading this, it is unclear to me why we have two ways to deviate the rule
>> r7.1. And more importantely, how would the developper decide which one to use?
> 
> I agree with you on this and we were discussing this topic just this
> morning in the FUSA community call. I think we need a way to do this
> with the SAF framework:
> 
> if (some code with violation) /* SAF-xx-safe */
> 
> This doesn't work today unfortunately. It can only be done this way:
> 
> /* SAF-xx-safe */
> if (some code with violation)
> 
> Which is not always desirable. octal-ok is just an ad-hoc solution for
> one specific violation but we need a generic way to do this. Luca is
> investigating possible ways to support the previous format in SAF.

Why can't we use octal-ok everywhere for now? My point here is to make 
simple for the developper to know what to use.

> 
> I think we should take this patch for now and harmonize it once SAF is
> improved.

The description of the deviation needs some improvement. To give an 
example, with the current wording, one could they can use octal-ok 
everywhere. But above, you are implying that SAF-xx-safe should be
preferred.

I would still strongly prefer if we use octal-ok everywhere because this 
is simple to remember. But if the other are happy to have both SAF-XX 
and octal-ok, then the description needs to be completely unambiguous 
and the patch should contain some explanation why we have two different 
ways to deviate.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
> 
> Hi Stefano,
> 
> On 30/10/2023 22:49, Stefano Stabellini wrote:
>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>> Hi Nicola,
>>> 
>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>> --- a/docs/misra/deviations.rst
>>>> +++ b/docs/misra/deviations.rst
>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>            - __emulate_2op and __emulate_2op_nobyte
>>>>            - read_debugreg and write_debugreg
>>>>   +   * - R7.1
>>>> +     - It is safe to use certain octal constants the way they are defined
>>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>>> SAF
>>>> +       comment (see safe.json).
>>> 
>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>> r7.1. And more importantely, how would the developper decide which one to use?
>> I agree with you on this and we were discussing this topic just this
>> morning in the FUSA community call. I think we need a way to do this
>> with the SAF framework:
>> if (some code with violation) /* SAF-xx-safe */
>> This doesn't work today unfortunately. It can only be done this way:
>> /* SAF-xx-safe */
>> if (some code with violation)
>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>> one specific violation but we need a generic way to do this. Luca is
>> investigating possible ways to support the previous format in SAF.
> 
> Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use.
> 
>> I think we should take this patch for now and harmonize it once SAF is
>> improved.
> 
> The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be
> preferred.
> 
> I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate.

Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */

So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer
has a way to understand what is the violation here without going to the justification database.

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Nicola Vetrini]

On 2023-10-31 15:13, Luca Fancellu wrote:
>> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
>> 
>> Hi Stefano,
>> 
>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>>> Hi Nicola,
>>>> 
>>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>>> --- a/docs/misra/deviations.rst
>>>>> +++ b/docs/misra/deviations.rst
>>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>>            - __emulate_2op and __emulate_2op_nobyte
>>>>>            - read_debugreg and write_debugreg
>>>>>   +   * - R7.1
>>>>> +     - It is safe to use certain octal constants the way they are 
>>>>> defined
>>>>> +       in specifications, manuals, and algorithm descriptions. 
>>>>> Such places
>>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or 
>>>>> with a
>>>>> SAF
>>>>> +       comment (see safe.json).
>>>> 
>>>> Reading this, it is unclear to me why we have two ways to deviate 
>>>> the rule
>>>> r7.1. And more importantely, how would the developper decide which 
>>>> one to use?
>>> I agree with you on this and we were discussing this topic just this
>>> morning in the FUSA community call. I think we need a way to do this
>>> with the SAF framework:
>>> if (some code with violation) /* SAF-xx-safe */
>>> This doesn't work today unfortunately. It can only be done this way:
>>> /* SAF-xx-safe */
>>> if (some code with violation)
>>> Which is not always desirable. octal-ok is just an ad-hoc solution 
>>> for
>>> one specific violation but we need a generic way to do this. Luca is
>>> investigating possible ways to support the previous format in SAF.
>> 
>> Why can't we use octal-ok everywhere for now? My point here is to make 
>> simple for the developper to know what to use.
>> 
>>> I think we should take this patch for now and harmonize it once SAF 
>>> is
>>> improved.
>> 
>> The description of the deviation needs some improvement. To give an 
>> example, with the current wording, one could they can use octal-ok 
>> everywhere. But above, you are implying that SAF-xx-safe should be
>> preferred.
>> 
>> I would still strongly prefer if we use octal-ok everywhere because 
>> this is simple to remember. But if the other are happy to have both 
>> SAF-XX and octal-ok, then the description needs to be completely 
>> unambiguous and the patch should contain some explanation why we have 
>> two different ways to deviate.
> 
> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
> 
> So that the suppression engine do what it should (currently it doesn’t 
> suppress the same line, but we could do something about it) and the 
> developer
> has a way to understand what is the violation here without going to the 
> justification database.

I guess. It could overflow the 80-char limit in 
xen/arch/x86/hvm/svm/svm.h, though.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 15:10, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> On 2023-10-31 15:13, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
>>> Hi Stefano,
>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>>>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>>>> Hi Nicola,
>>>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>>>> --- a/docs/misra/deviations.rst
>>>>>> +++ b/docs/misra/deviations.rst
>>>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>>>           - __emulate_2op and __emulate_2op_nobyte
>>>>>>           - read_debugreg and write_debugreg
>>>>>>  +   * - R7.1
>>>>>> +     - It is safe to use certain octal constants the way they are defined
>>>>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>>>>> SAF
>>>>>> +       comment (see safe.json).
>>>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>>>> r7.1. And more importantely, how would the developper decide which one to use?
>>>> I agree with you on this and we were discussing this topic just this
>>>> morning in the FUSA community call. I think we need a way to do this
>>>> with the SAF framework:
>>>> if (some code with violation) /* SAF-xx-safe */
>>>> This doesn't work today unfortunately. It can only be done this way:
>>>> /* SAF-xx-safe */
>>>> if (some code with violation)
>>>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>>>> one specific violation but we need a generic way to do this. Luca is
>>>> investigating possible ways to support the previous format in SAF.
>>> Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use.
>>>> I think we should take this patch for now and harmonize it once SAF is
>>>> improved.
>>> The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be
>>> preferred.
>>> I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate.
>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>> So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer
>> has a way to understand what is the violation here without going to the justification database.
> 
> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though.

Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars

> 
> -- 
> Nicola Vetrini, BSc
> Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

Hi,

On 31/10/2023 15:12, Luca Fancellu wrote:
>> On 31 Oct 2023, at 15:10, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>
>> On 2023-10-31 15:13, Luca Fancellu wrote:
>>>> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
>>>> Hi Stefano,
>>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>>>>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>>>>> Hi Nicola,
>>>>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>>>>> --- a/docs/misra/deviations.rst
>>>>>>> +++ b/docs/misra/deviations.rst
>>>>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>>>>            - __emulate_2op and __emulate_2op_nobyte
>>>>>>>            - read_debugreg and write_debugreg
>>>>>>>   +   * - R7.1
>>>>>>> +     - It is safe to use certain octal constants the way they are defined
>>>>>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>>>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>>>>>> SAF
>>>>>>> +       comment (see safe.json).
>>>>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>>>>> r7.1. And more importantely, how would the developper decide which one to use?
>>>>> I agree with you on this and we were discussing this topic just this
>>>>> morning in the FUSA community call. I think we need a way to do this
>>>>> with the SAF framework:
>>>>> if (some code with violation) /* SAF-xx-safe */
>>>>> This doesn't work today unfortunately. It can only be done this way:
>>>>> /* SAF-xx-safe */
>>>>> if (some code with violation)
>>>>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>>>>> one specific violation but we need a generic way to do this. Luca is
>>>>> investigating possible ways to support the previous format in SAF.
>>>> Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use.
>>>>> I think we should take this patch for now and harmonize it once SAF is
>>>>> improved.
>>>> The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be
>>>> preferred.
>>>> I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate.
>>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>>> So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer
>>> has a way to understand what is the violation here without going to the justification database.
>>
>> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though.
> 
> Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars

In the past I expressed concerned with this kind of the rule because it 
is not entirely clear how an automatic formatter will be able to check it.

Can you clarify whether clang-format would be able to handle your 
proposed rule?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 15:27, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 31/10/2023 15:12, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 15:10, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>> 
>>> On 2023-10-31 15:13, Luca Fancellu wrote:
>>>>> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
>>>>> Hi Stefano,
>>>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>>>>>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>>>>>> Hi Nicola,
>>>>>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>>>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>>>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>>>>>> --- a/docs/misra/deviations.rst
>>>>>>>> +++ b/docs/misra/deviations.rst
>>>>>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>>>>>           - __emulate_2op and __emulate_2op_nobyte
>>>>>>>>           - read_debugreg and write_debugreg
>>>>>>>>  +   * - R7.1
>>>>>>>> +     - It is safe to use certain octal constants the way they are defined
>>>>>>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>>>>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>>>>>>> SAF
>>>>>>>> +       comment (see safe.json).
>>>>>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>>>>>> r7.1. And more importantely, how would the developper decide which one to use?
>>>>>> I agree with you on this and we were discussing this topic just this
>>>>>> morning in the FUSA community call. I think we need a way to do this
>>>>>> with the SAF framework:
>>>>>> if (some code with violation) /* SAF-xx-safe */
>>>>>> This doesn't work today unfortunately. It can only be done this way:
>>>>>> /* SAF-xx-safe */
>>>>>> if (some code with violation)
>>>>>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>>>>>> one specific violation but we need a generic way to do this. Luca is
>>>>>> investigating possible ways to support the previous format in SAF.
>>>>> Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use.
>>>>>> I think we should take this patch for now and harmonize it once SAF is
>>>>>> improved.
>>>>> The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be
>>>>> preferred.
>>>>> I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate.
>>>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>>>> So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer
>>>> has a way to understand what is the violation here without going to the justification database.
>>> 
>>> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though.
>> Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars
> 
> In the past I expressed concerned with this kind of the rule because it is not entirely clear how an automatic formatter will be able to check it.
> 
> Can you clarify whether clang-format would be able to handle your proposed rule?

So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if we use ReflowComments: false we should
be able to let the line as it is (not tested).

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments



> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Julien Grall]

On 31/10/2023 15:32, Luca Fancellu wrote:
> 
> 
>> On 31 Oct 2023, at 15:27, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 31/10/2023 15:12, Luca Fancellu wrote:
>>>> On 31 Oct 2023, at 15:10, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>>>
>>>> On 2023-10-31 15:13, Luca Fancellu wrote:
>>>>>> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
>>>>>> Hi Stefano,
>>>>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>>>>>>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>>>>>>> Hi Nicola,
>>>>>>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>>>>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>>>>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>>>>>>> --- a/docs/misra/deviations.rst
>>>>>>>>> +++ b/docs/misra/deviations.rst
>>>>>>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>>>>>>            - __emulate_2op and __emulate_2op_nobyte
>>>>>>>>>            - read_debugreg and write_debugreg
>>>>>>>>>   +   * - R7.1
>>>>>>>>> +     - It is safe to use certain octal constants the way they are defined
>>>>>>>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>>>>>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>>>>>>>> SAF
>>>>>>>>> +       comment (see safe.json).
>>>>>>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>>>>>>> r7.1. And more importantely, how would the developper decide which one to use?
>>>>>>> I agree with you on this and we were discussing this topic just this
>>>>>>> morning in the FUSA community call. I think we need a way to do this
>>>>>>> with the SAF framework:
>>>>>>> if (some code with violation) /* SAF-xx-safe */
>>>>>>> This doesn't work today unfortunately. It can only be done this way:
>>>>>>> /* SAF-xx-safe */
>>>>>>> if (some code with violation)
>>>>>>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>>>>>>> one specific violation but we need a generic way to do this. Luca is
>>>>>>> investigating possible ways to support the previous format in SAF.
>>>>>> Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use.
>>>>>>> I think we should take this patch for now and harmonize it once SAF is
>>>>>>> improved.
>>>>>> The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be
>>>>>> preferred.
>>>>>> I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate.
>>>>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>>>>> So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer
>>>>> has a way to understand what is the violation here without going to the justification database.
>>>>
>>>> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though.
>>> Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars
>>
>> In the past I expressed concerned with this kind of the rule because it is not entirely clear how an automatic formatter will be able to check it.
>>
>> Can you clarify whether clang-format would be able to handle your proposed rule?
> 
> So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if we use ReflowComments: false we should
> be able to let the line as it is (not tested).

Wouldn't that prevent reflow for all the comments? If so, I don't think 
this is we want. Instead, we want to allow reflow for any comments but 
the one done at the end of the line.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Luca Fancellu]

> On 31 Oct 2023, at 15:36, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 31/10/2023 15:32, Luca Fancellu wrote:
>>> On 31 Oct 2023, at 15:27, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi,
>>> 
>>> On 31/10/2023 15:12, Luca Fancellu wrote:
>>>>> On 31 Oct 2023, at 15:10, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>>>> 
>>>>> On 2023-10-31 15:13, Luca Fancellu wrote:
>>>>>>> On 31 Oct 2023, at 13:27, Julien Grall <julien@xen.org> wrote:
>>>>>>> Hi Stefano,
>>>>>>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>>>>>>>> On Mon, 30 Oct 2023, Julien Grall wrote:
>>>>>>>>> Hi Nicola,
>>>>>>>>> On 27/10/2023 16:11, Nicola Vetrini wrote:
>>>>>>>>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>>>>>>>>> index 8511a189253b..8aaaa1473fb4 100644
>>>>>>>>>> --- a/docs/misra/deviations.rst
>>>>>>>>>> +++ b/docs/misra/deviations.rst
>>>>>>>>>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>>>>>>>           - __emulate_2op and __emulate_2op_nobyte
>>>>>>>>>>           - read_debugreg and write_debugreg
>>>>>>>>>>  +   * - R7.1
>>>>>>>>>> +     - It is safe to use certain octal constants the way they are defined
>>>>>>>>>> +       in specifications, manuals, and algorithm descriptions. Such places
>>>>>>>>>> +       are marked safe with a /\* octal-ok \*/ in-code comment, or with a
>>>>>>>>>> SAF
>>>>>>>>>> +       comment (see safe.json).
>>>>>>>>> Reading this, it is unclear to me why we have two ways to deviate the rule
>>>>>>>>> r7.1. And more importantely, how would the developper decide which one to use?
>>>>>>>> I agree with you on this and we were discussing this topic just this
>>>>>>>> morning in the FUSA community call. I think we need a way to do this
>>>>>>>> with the SAF framework:
>>>>>>>> if (some code with violation) /* SAF-xx-safe */
>>>>>>>> This doesn't work today unfortunately. It can only be done this way:
>>>>>>>> /* SAF-xx-safe */
>>>>>>>> if (some code with violation)
>>>>>>>> Which is not always desirable. octal-ok is just an ad-hoc solution for
>>>>>>>> one specific violation but we need a generic way to do this. Luca is
>>>>>>>> investigating possible ways to support the previous format in SAF.
>>>>>>> Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use.
>>>>>>>> I think we should take this patch for now and harmonize it once SAF is
>>>>>>>> improved.
>>>>>>> The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be
>>>>>>> preferred.
>>>>>>> I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate.
>>>>>> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */
>>>>>> So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer
>>>>>> has a way to understand what is the violation here without going to the justification database.
>>>>> 
>>>>> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though.
>>>> Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars
>>> 
>>> In the past I expressed concerned with this kind of the rule because it is not entirely clear how an automatic formatter will be able to check it.
>>> 
>>> Can you clarify whether clang-format would be able to handle your proposed rule?
>> So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if we use ReflowComments: false we should
>> be able to let the line as it is (not tested).
> 
> Wouldn't that prevent reflow for all the comments? If so, I don't think this is we want. Instead, we want to allow reflow for any comments but the one done at the end of the line.

Ok well, I was optimistic, in reality with the option as false, it would anyway reflow the line leaving the comment untouched.

E.g. from this:

        if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe octal-ok */
             (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe octal-ok */
             (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe octal-ok */
            return emul_len;

To this:

        if ( modrm_mod ==
                 MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe octal-ok */
             (modrm_reg & 7) ==
                 MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe octal-ok */
             (modrm_rm & 7) ==
                 MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe octal-ok */
            return emul_len;

... sigh...

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Stefano Stabellini]

On Tue, 30 Oct 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/10/2023 22:49, Stefano Stabellini wrote:
> > On Mon, 30 Oct 2023, Julien Grall wrote:
> > > Hi Nicola,
> > > 
> > > On 27/10/2023 16:11, Nicola Vetrini wrote:
> > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > > > index 8511a189253b..8aaaa1473fb4 100644
> > > > --- a/docs/misra/deviations.rst
> > > > +++ b/docs/misra/deviations.rst
> > > > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
> > > >             - __emulate_2op and __emulate_2op_nobyte
> > > >             - read_debugreg and write_debugreg
> > > >    +   * - R7.1
> > > > +     - It is safe to use certain octal constants the way they are
> > > > defined
> > > > +       in specifications, manuals, and algorithm descriptions. Such
> > > > places
> > > > +       are marked safe with a /\* octal-ok \*/ in-code comment, or with
> > > > a
> > > > SAF
> > > > +       comment (see safe.json).
> > > 
> > > Reading this, it is unclear to me why we have two ways to deviate the rule
> > > r7.1. And more importantely, how would the developper decide which one to
> > > use?
> > 
> > I agree with you on this and we were discussing this topic just this
> > morning in the FUSA community call. I think we need a way to do this
> > with the SAF framework:
> > 
> > if (some code with violation) /* SAF-xx-safe */
> > 
> > This doesn't work today unfortunately. It can only be done this way:
> > 
> > /* SAF-xx-safe */
> > if (some code with violation)
> > 
> > Which is not always desirable. octal-ok is just an ad-hoc solution for
> > one specific violation but we need a generic way to do this. Luca is
> > investigating possible ways to support the previous format in SAF.
> 
> Why can't we use octal-ok everywhere for now?

I think this is a good option for now, yes


> My point here is to make simple for the developper to know what to use.
>
> > 
> > I think we should take this patch for now and harmonize it once SAF is
> > improved.
> 
> The description of the deviation needs some improvement.

+1


> To give an example,
> with the current wording, one could they can use octal-ok everywhere. But
> above, you are implying that SAF-xx-safe should be
> preferred.
> 
> I would still strongly prefer if we use octal-ok everywhere because this is
> simple to remember. But if the other are happy to have both SAF-XX and
> octal-ok, then the description needs to be completely unambiguous and the
> patch should contain some explanation why we have two different ways to
> deviate.

I think we could say "octal-ok" only and not mention SAF. As you can see
from the other messages we still have work to do on SAF to be able to
use it the way we would like to use it.

Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1

[Nicola Vetrini]

Hi Julien, Stefano

On 2023-10-31 22:41, Stefano Stabellini wrote:
> On Tue, 30 Oct 2023, Julien Grall wrote:
>> Hi Stefano,
>> 
>> On 30/10/2023 22:49, Stefano Stabellini wrote:
>> > On Mon, 30 Oct 2023, Julien Grall wrote:
>> > > Hi Nicola,
>> > >
>> > > On 27/10/2023 16:11, Nicola Vetrini wrote:
>> > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>> > > > index 8511a189253b..8aaaa1473fb4 100644
>> > > > --- a/docs/misra/deviations.rst
>> > > > +++ b/docs/misra/deviations.rst
>> > > > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
>> > > >             - __emulate_2op and __emulate_2op_nobyte
>> > > >             - read_debugreg and write_debugreg
>> > > >    +   * - R7.1
>> > > > +     - It is safe to use certain octal constants the way they are
>> > > > defined
>> > > > +       in specifications, manuals, and algorithm descriptions. Such
>> > > > places
>> > > > +       are marked safe with a /\* octal-ok \*/ in-code comment, or with
>> > > > a
>> > > > SAF
>> > > > +       comment (see safe.json).
>> > >
>> > > Reading this, it is unclear to me why we have two ways to deviate the rule
>> > > r7.1. And more importantely, how would the developper decide which one to
>> > > use?
>> >
>> > I agree with you on this and we were discussing this topic just this
>> > morning in the FUSA community call. I think we need a way to do this
>> > with the SAF framework:
>> >
>> > if (some code with violation) /* SAF-xx-safe */
>> >
>> > This doesn't work today unfortunately. It can only be done this way:
>> >
>> > /* SAF-xx-safe */
>> > if (some code with violation)
>> >
>> > Which is not always desirable. octal-ok is just an ad-hoc solution for
>> > one specific violation but we need a generic way to do this. Luca is
>> > investigating possible ways to support the previous format in SAF.
>> 
>> Why can't we use octal-ok everywhere for now?
> 
> I think this is a good option for now, yes
> 
> 
>> My point here is to make simple for the developper to know what to 
>> use.
>> 
>> >
>> > I think we should take this patch for now and harmonize it once SAF is
>> > improved.
>> 
>> The description of the deviation needs some improvement.
> 
> +1
> 
> 
>> To give an example,
>> with the current wording, one could they can use octal-ok everywhere. 
>> But
>> above, you are implying that SAF-xx-safe should be
>> preferred.
>> 
>> I would still strongly prefer if we use octal-ok everywhere because 
>> this is
>> simple to remember. But if the other are happy to have both SAF-XX and
>> octal-ok, then the description needs to be completely unambiguous and 
>> the
>> patch should contain some explanation why we have two different ways 
>> to
>> deviate.
> 
> I think we could say "octal-ok" only and not mention SAF. As you can 
> see
> from the other messages we still have work to do on SAF to be able to
> use it the way we would like to use it.

Thanks for the feedback; I'll revise the patch to use and mention only 
octal-ok.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)