Address MISRA C:2012 Rule 8.4

Address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

The headline of Rule 8.4 is as follows:
"A compatible declaration shall be visible when an object or
function with external linkage is defined".

Some functions reported in [1][2] are lacking a declaration in the 
respective header files;
as remarked on xen-devel's IRC channel, this is ok since they are only 
called from asm code (e.g., start_xen). A similar discussion
had taken place in the past (see [3]) and the general consensus was to 
deviate these cases.
If that is still the case, a suitable project-wide deviation can be 
added to address these violations.

[1] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/origin/staging/ARM64-Set1/210/PROJECT.ecd;/by_service/MC3R1.R8.4.html
[2] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/origin/staging/X86_64-Set1/210/PROJECT.ecd;/by_service/MC3R1.R8.4.html
[3] 
https://lore.kernel.org/all/20220705210218.483854-2-burzalodowa@gmail.com/

Regards,

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

Re: Address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

I think that's OK for me. My only concern is that we should track the
project-wide deviations properly somewhere besides the ECLAIR
configuration under xen.git which is ECLAIR specific. So far we used the
notes in docs/misra/rules.rst. I don't know if that sufficient, but we
could add a note for 8.4:

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index 8f0e4d3f25..5977bc9d5e 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -245,7 +245,8 @@ maintainers if you want to suggest a change.
      - Required
      - A compatible declaration shall be visible when an object or
        function with external linkage is defined
-     -
+     - No need for declarations when functions are only called from
+       assembly
 
    * - `Rule 8.5 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_05_2.c>`_
      - Required


On Thu, 3 Aug 2023, Nicola Vetrini wrote:
> The headline of Rule 8.4 is as follows:
> "A compatible declaration shall be visible when an object or
> function with external linkage is defined".
> 
> Some functions reported in [1][2] are lacking a declaration in the respective
> header files;
> as remarked on xen-devel's IRC channel, this is ok since they are only called
> from asm code (e.g., start_xen). A similar discussion
> had taken place in the past (see [3]) and the general consensus was to deviate
> these cases.
> If that is still the case, a suitable project-wide deviation can be added to
> address these violations.
> 
> [1]
> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/origin/staging/ARM64-Set1/210/PROJECT.ecd;/by_service/MC3R1.R8.4.html
> [2]
> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/origin/staging/X86_64-Set1/210/PROJECT.ecd;/by_service/MC3R1.R8.4.html
> [3] https://lore.kernel.org/all/20220705210218.483854-2-burzalodowa@gmail.com/
> 
> Regards,
> 
> -- 
> Nicola Vetrini, BSc
> Software Engineer, BUGSENG srl (https://bugseng.com)
> 

Re: Address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

On 04/08/2023 02:35, Stefano Stabellini wrote:
> I think that's OK for me. My only concern is that we should track the
> project-wide deviations properly somewhere besides the ECLAIR
> configuration under xen.git which is ECLAIR specific. So far we used 
> the
> notes in docs/misra/rules.rst. I don't know if that sufficient, but we
> could add a note for 8.4:
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 8f0e4d3f25..5977bc9d5e 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -245,7 +245,8 @@ maintainers if you want to suggest a change.
>       - Required
>       - A compatible declaration shall be visible when an object or
>         function with external linkage is defined
> -     -
> +     - No need for declarations when functions are only called from
> +       assembly
> 
>     * - `Rule 8.5
> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_05_2.c>`_
>       - Required
> 
> 
> On Thu, 3 Aug 2023, Nicola Vetrini wrote:
>> The headline of Rule 8.4 is as follows:
>> "A compatible declaration shall be visible when an object or
>> function with external linkage is defined".
>> 
>> Some functions reported in [1][2] are lacking a declaration in the 
>> respective
>> header files;
>> as remarked on xen-devel's IRC channel, this is ok since they are only 
>> called
>> from asm code (e.g., start_xen). A similar discussion
>> had taken place in the past (see [3]) and the general consensus was to 
>> deviate
>> these cases.
>> If that is still the case, a suitable project-wide deviation can be 
>> added to
>> address these violations.
>> 
>> [1]
>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/origin/staging/ARM64-Set1/210/PROJECT.ecd;/by_service/MC3R1.R8.4.html
>> [2]
>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/origin/staging/X86_64-Set1/210/PROJECT.ecd;/by_service/MC3R1.R8.4.html
>> [3] 
>> https://lore.kernel.org/all/20220705210218.483854-2-burzalodowa@gmail.com/
>> 

Upon further examination, I identified the following patterns:

1. Functions defined in .c called only from asm code (e.g., the already 
mentioned __start_xen)
2. Functions/variables declared in a .h, defined in a .c that does not 
include the .h with the declaration
(e.g., 'fill_console_start_info' is defined in 'xen/drivers/vga.c', 
declared in 'xen/include/xen/console.h' which is not visible when 
compiling the .c).
3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in 
'xen/include/acpi/acglobal.h', depending on
    DEFINE_ACPI_GLOBALS

Below are the proposed resolution strategies:

1. I would advise to add the declaration in the relative .h, to support 
automatic consistency checks with the
    implementation and a quick reference when touching the asm.
2. To comply with the rule, the header with the declaration should be 
included. Also note that there are some
    corner cases, such as 'get_sec', which is used in 'cper.h' without 
including 'time.h' (which should gain a
    declaration for it).
3. One possible resolution pattern is including 'acglobal.h' twice 
(either directly or indirectly trough acpi.h, if
    the latter does not cause other issues) like so:

    (assuming DEFINE_ACPI_GLOBALS is undefined here)
    #include "acglobal.h"
    #define DEFINE_ACPI_GLOBALS
    #include  "acglobal.h"

   this way, the rule is followed properly, though it's not the prettiest 
pattern and also clashes with the objectives
   of D4.10 ("Precautions shall be taken in order to prevent the contents 
of a header file being included
   more than once"), but then a motivated exception is allowed there.

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

Re: Address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 04.08.2023 11:47, Nicola Vetrini wrote:
> Upon further examination, I identified the following patterns:
> 
> 1. Functions defined in .c called only from asm code (e.g., the already 
> mentioned __start_xen)
> 2. Functions/variables declared in a .h, defined in a .c that does not 
> include the .h with the declaration
> (e.g., 'fill_console_start_info' is defined in 'xen/drivers/vga.c', 
> declared in 'xen/include/xen/console.h' which is not visible when 
> compiling the .c).
> 3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in 
> 'xen/include/acpi/acglobal.h', depending on
>     DEFINE_ACPI_GLOBALS
> 
> Below are the proposed resolution strategies:
> 
> 1. I would advise to add the declaration in the relative .h, to support 
> automatic consistency checks with the
>     implementation and a quick reference when touching the asm.

That'll need discussing first.

> 2. To comply with the rule, the header with the declaration should be 
> included. Also note that there are some
>     corner cases, such as 'get_sec', which is used in 'cper.h' without 
> including 'time.h' (which should gain a
>     declaration for it).

This one of course wants fixing wherever found.

> 3. One possible resolution pattern is including 'acglobal.h' twice 
> (either directly or indirectly trough acpi.h, if
>     the latter does not cause other issues) like so:
> 
>     (assuming DEFINE_ACPI_GLOBALS is undefined here)
>     #include "acglobal.h"
>     #define DEFINE_ACPI_GLOBALS
>     #include  "acglobal.h"
> 
>    this way, the rule is followed properly, though it's not the prettiest 
> pattern and also clashes with the objectives
>    of D4.10 ("Precautions shall be taken in order to prevent the contents 
> of a header file being included
>    more than once"), but then a motivated exception is allowed there.

Not really sure about this one.

Jan

Re: Address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

> 
> Upon further examination, I identified the following patterns:
> 
> 1. Functions defined in .c called only from asm code (e.g., the
> already mentioned __start_xen)
> 2. Functions/variables declared in a .h, defined in a .c that does not
> include the .h with the declaration
> (e.g., 'fill_console_start_info' is defined in 'xen/drivers/vga.c',
> declared in 'xen/include/xen/console.h' which is not visible when
> compiling the .c).
> 3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in
> 'xen/include/acpi/acglobal.h', depending on
>    DEFINE_ACPI_GLOBALS
> 
> Below are the proposed resolution strategies:
> 
> 1. I would advise to add the declaration in the relative .h, to
> support automatic consistency checks with the
>    implementation and a quick reference when touching the asm.
> 2. To comply with the rule, the header with the declaration should be
> included. Also note that there are some
>    corner cases, such as 'get_sec', which is used in 'cper.h' without
> including 'time.h' (which should gain a
>    declaration for it).
> 3. One possible resolution pattern is including 'acglobal.h' twice
> (either directly or indirectly trough acpi.h, if
>    the latter does not cause other issues) like so:
> 
>    (assuming DEFINE_ACPI_GLOBALS is undefined here)
>    #include "acglobal.h"
>    #define DEFINE_ACPI_GLOBALS
>    #include  "acglobal.h"
> 
>   this way, the rule is followed properly, though it's not the
> prettiest pattern and also clashes with the objectives
>   of D4.10 ("Precautions shall be taken in order to prevent the
> contents of a header file being included
>   more than once"), but then a motivated exception is allowed there.

One further question is whether functions under 
'xen/common/coverage/gcov_base.c' should gain
a declaration in 'gcov.h' or not, as they exist just for the purpose of 
being referenced
by autogenerated profiling code. I see no reason why they shouldn't, but 
they can also be safely deviated,
since they are not called by Xen code.

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

Re: Address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

>> 3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in
>> 'xen/include/acpi/acglobal.h', depending on
>>     DEFINE_ACPI_GLOBALS
>> 
>> Below are the proposed resolution strategies:

> 
>> 3. One possible resolution pattern is including 'acglobal.h' twice
>> (either directly or indirectly trough acpi.h, if
>>     the latter does not cause other issues) like so:
>> 
>>     (assuming DEFINE_ACPI_GLOBALS is undefined here)
>>     #include "acglobal.h"
>>     #define DEFINE_ACPI_GLOBALS
>>     #include  "acglobal.h"
>> 
>>    this way, the rule is followed properly, though it's not the 
>> prettiest
>> pattern and also clashes with the objectives
>>    of D4.10 ("Precautions shall be taken in order to prevent the 
>> contents
>> of a header file being included
>>    more than once"), but then a motivated exception is allowed there.
> 
> Not really sure about this one.
> 
> Jan

If you can tell me more about why that header is defined the way it is 
(i.e. why it's used twice with
DEFINE_ACPI_GLOBALS #defined and the other times without), maybe we can 
come up
with better alternatives.

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

Re: Address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

On Fri, 4 Aug 2023, Jan Beulich wrote:
> On 04.08.2023 11:47, Nicola Vetrini wrote:
> > Upon further examination, I identified the following patterns:
> > 
> > 1. Functions defined in .c called only from asm code (e.g., the already 
> > mentioned __start_xen)
> > 2. Functions/variables declared in a .h, defined in a .c that does not 
> > include the .h with the declaration
> > (e.g., 'fill_console_start_info' is defined in 'xen/drivers/vga.c', 
> > declared in 'xen/include/xen/console.h' which is not visible when 
> > compiling the .c).
> > 3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in 
> > 'xen/include/acpi/acglobal.h', depending on
> >     DEFINE_ACPI_GLOBALS
> > 
> > Below are the proposed resolution strategies:
> > 
> > 1. I would advise to add the declaration in the relative .h, to support 
> > automatic consistency checks with the
> >     implementation and a quick reference when touching the asm.
> 
> That'll need discussing first.

My take is that it would be nicer if they were declared in .h files. It
would also be useful for ourselves as documentation/reference. So I
would be OK accepting patches like that. But at the same time I don't
see it as a must-have requirement for safety (the .h declaration would
not be actually used).


> > 2. To comply with the rule, the header with the declaration should be 
> > included. Also note that there are some
> >     corner cases, such as 'get_sec', which is used in 'cper.h' without 
> > including 'time.h' (which should gain a
> >     declaration for it).
> 
> This one of course wants fixing wherever found.

+1


> > 3. One possible resolution pattern is including 'acglobal.h' twice 
> > (either directly or indirectly trough acpi.h, if
> >     the latter does not cause other issues) like so:
> > 
> >     (assuming DEFINE_ACPI_GLOBALS is undefined here)
> >     #include "acglobal.h"
> >     #define DEFINE_ACPI_GLOBALS
> >     #include  "acglobal.h"
> > 
> >    this way, the rule is followed properly, though it's not the prettiest 
> > pattern and also clashes with the objectives
> >    of D4.10 ("Precautions shall be taken in order to prevent the contents 
> > of a header file being included
> >    more than once"), but then a motivated exception is allowed there.
> 
> Not really sure about this one.
 
I would leave these alone because it was clearly done on purpose "to
simplify maintenance of the code" as the in-code comment claims. Also
this is very old code, probably inherited from somewhere else and never
changed.

Re: Address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 04.08.2023 16:09, Nicola Vetrini wrote:
>>> 3. One possible resolution pattern is including 'acglobal.h' twice
>>> (either directly or indirectly trough acpi.h, if
>>>     the latter does not cause other issues) like so:
>>>
>>>     (assuming DEFINE_ACPI_GLOBALS is undefined here)
>>>     #include "acglobal.h"
>>>     #define DEFINE_ACPI_GLOBALS
>>>     #include  "acglobal.h"
>>>
>>>    this way, the rule is followed properly, though it's not the 
>>> prettiest
>>> pattern and also clashes with the objectives
>>>    of D4.10 ("Precautions shall be taken in order to prevent the 
>>> contents
>>> of a header file being included
>>>    more than once"), but then a motivated exception is allowed there.
>>
>> Not really sure about this one.
> 
> If you can tell me more about why that header is defined the way it is 
> (i.e. why it's used twice with
> DEFINE_ACPI_GLOBALS #defined and the other times without), maybe we can 
> come up
> with better alternatives.

The "Why?" question can only be answered by the ACPICA folks who originally
made it like this. Linux inherited the file from there, and we inherited it
from Linux. See also Stefano's reply on the matter. My guess is that the
goal was to have just a single place that needs changing (besides consumer
sites, where changes may or may not be necessary) if the type of a variable
needs to be adjusted.

Jan

Re: Address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 04.08.2023 16:00, Nicola Vetrini wrote:
>>
>> Upon further examination, I identified the following patterns:
>>
>> 1. Functions defined in .c called only from asm code (e.g., the
>> already mentioned __start_xen)
>> 2. Functions/variables declared in a .h, defined in a .c that does not
>> include the .h with the declaration
>> (e.g., 'fill_console_start_info' is defined in 'xen/drivers/vga.c',
>> declared in 'xen/include/xen/console.h' which is not visible when
>> compiling the .c).
>> 3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in
>> 'xen/include/acpi/acglobal.h', depending on
>>    DEFINE_ACPI_GLOBALS
>>
>> Below are the proposed resolution strategies:
>>
>> 1. I would advise to add the declaration in the relative .h, to
>> support automatic consistency checks with the
>>    implementation and a quick reference when touching the asm.
>> 2. To comply with the rule, the header with the declaration should be
>> included. Also note that there are some
>>    corner cases, such as 'get_sec', which is used in 'cper.h' without
>> including 'time.h' (which should gain a
>>    declaration for it).
>> 3. One possible resolution pattern is including 'acglobal.h' twice
>> (either directly or indirectly trough acpi.h, if
>>    the latter does not cause other issues) like so:
>>
>>    (assuming DEFINE_ACPI_GLOBALS is undefined here)
>>    #include "acglobal.h"
>>    #define DEFINE_ACPI_GLOBALS
>>    #include  "acglobal.h"
>>
>>   this way, the rule is followed properly, though it's not the
>> prettiest pattern and also clashes with the objectives
>>   of D4.10 ("Precautions shall be taken in order to prevent the
>> contents of a header file being included
>>   more than once"), but then a motivated exception is allowed there.
> 
> One further question is whether functions under 
> 'xen/common/coverage/gcov_base.c' should gain
> a declaration in 'gcov.h' or not, as they exist just for the purpose of 
> being referenced
> by autogenerated profiling code. I see no reason why they shouldn't, but 
> they can also be safely deviated,
> since they are not called by Xen code.

Imo it should be the compiler to provide a prototype for these (much
like it does for builtins), thus ensuring that an implementation
actually matches the compiler's expectations. Yet afaics it doesn't.

Jan

Re: Address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

>> One further question is whether functions under
>> 'xen/common/coverage/gcov_base.c' should gain
>> a declaration in 'gcov.h' or not, as they exist just for the purpose 
>> of
>> being referenced
>> by autogenerated profiling code. I see no reason why they shouldn't, 
>> but
>> they can also be safely deviated,
>> since they are not called by Xen code.
> 
> Imo it should be the compiler to provide a prototype for these (much
> like it does for builtins), thus ensuring that an implementation
> actually matches the compiler's expectations. Yet afaics it doesn't.
> 
> Jan

Then it's perhaps best to deviate them.

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