Re: Address MISRA C:2012 Rule 8.4

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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)