MISRA: Compatible declarations for sort and bsearch

MISRA: Compatible declarations for sort and bsearch

[Nicola Vetrini]

Hi all,

in xen/lib.h and xen/sort.h there are definitions of the functions 
bsearch and sort that have no prior declarations, and therefore are 
subject to a violation of MISRA C Rule 8.4.

I'm wondering whether it would be preferred

1. to put a declaration just before the definition, in lib.h and sort.h
2. deviate these functions, as their signatures are well-known and 
somewhat standardized

other resolution strategies are possible, but I think these are the main 
ones.

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

Re: MISRA: Compatible declarations for sort and bsearch

[Nicola Vetrini]

On 2023-11-24 10:40, Nicola Vetrini wrote:
> Hi all,
> 
> in xen/lib.h and xen/sort.h there are definitions of the functions 
> bsearch and sort that have no prior declarations, and therefore are 
> subject to a violation of MISRA C Rule 8.4.
> 
> I'm wondering whether it would be preferred
> 
> 1. to put a declaration just before the definition, in lib.h and sort.h
> 2. deviate these functions, as their signatures are well-known and 
> somewhat standardized
> 
> other resolution strategies are possible, but I think these are the 
> main ones.

Still on the matter of Rule 8.4, though not related to bsearch or sort:

- the definition of do_mca in x86/cpu/mcheck/mca.c has the following 
header:
   #include <xen/hypercall.h> /* for do_mca */
   which in turn leads to x86/include/asm/hypercall.h, which includes the 
following:
   #include <public/arch-x86/xen-mca.h> /* for do_mca */

   where I can't see a declaration for do_mca, as I would have expected. 
I'd like to understand what's going on here, since I may be missing some 
piece of information (perhaps something is generated during the build).

- x86/traps.c do_general_protection may want a declaration in 
x86/include/asm/traps.h, or perhaps it should gain the asmlinkage 
attribute, given that it's used only by asm and the TU that defines it.

- function test and variable data in x86/efi/check.c look like they 
should not be MISRA compliant, so they may be added to the 
exclude-list.json

- given the comment in xen/common/page_alloc.c for first_valid_mfn

/*
  * first_valid_mfn is exported because it is use in ARM specific NUMA
  * helpers. See comment in arch/arm/include/asm/numa.h.
  */
mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;

and the related ARM comment

/*
  * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
  * is required because the dummy helpers are using it.
  */
extern mfn_t first_valid_mfn;

it should probably be deviated.

- compat_set_{px,cx}_pminfo in x86/x86_64/cpufreq.c are perhaps declared 
with an autogenerated header?

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

Re: MISRA: Compatible declarations for sort and bsearch

[Jan Beulich]

On 27.11.2023 15:32, Nicola Vetrini wrote:
> Still on the matter of Rule 8.4, though not related to bsearch or sort:
> 
> - the definition of do_mca in x86/cpu/mcheck/mca.c has the following 
> header:
>    #include <xen/hypercall.h> /* for do_mca */
>    which in turn leads to x86/include/asm/hypercall.h, which includes the 
> following:
>    #include <public/arch-x86/xen-mca.h> /* for do_mca */
> 
>    where I can't see a declaration for do_mca, as I would have expected. 
> I'd like to understand what's going on here, since I may be missing some 
> piece of information (perhaps something is generated during the build).

It can't possibly live in the public header. The comment simply went
stale with the auto-generation of headers; the decl is in hypercall-defs.h
now.

> - x86/traps.c do_general_protection may want a declaration in 
> x86/include/asm/traps.h, or perhaps it should gain the asmlinkage 
> attribute, given that it's used only by asm and the TU that defines it.

Neither is really attractive imo.

> - function test and variable data in x86/efi/check.c look like they 
> should not be MISRA compliant, so they may be added to the 
> exclude-list.json

This file isn't contributing to the final binary.

> - given the comment in xen/common/page_alloc.c for first_valid_mfn
> 
> /*
>   * first_valid_mfn is exported because it is use in ARM specific NUMA
>   * helpers. See comment in arch/arm/include/asm/numa.h.
>   */
> mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
> 
> and the related ARM comment
> 
> /*
>   * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>   * is required because the dummy helpers are using it.
>   */
> extern mfn_t first_valid_mfn;
> 
> it should probably be deviated.

NUMA work is still in progress for Arm, I think, so I'd rather wait with
deviating.

> - compat_set_{px,cx}_pminfo in x86/x86_64/cpufreq.c are perhaps declared 
> with an autogenerated header?

I don't think so. Only top-level hypercall handlers would be. This works by
(perhaps even unintentional) trickery: xen/pmstat.h is included only after
set_{c,p}x_pminfo are re-defined to compat_set_{c,p}x_pminfo, so the same
declarations happen to serve two purposes (but of course don't provide the
intended caller/callee agreement).

Jan

Re: MISRA: Compatible declarations for sort and bsearch

[Nicola Vetrini]

On 2023-11-27 15:59, Jan Beulich wrote:
> On 27.11.2023 15:32, Nicola Vetrini wrote:
>> Still on the matter of Rule 8.4, though not related to bsearch or 
>> sort:
>> 
>> - the definition of do_mca in x86/cpu/mcheck/mca.c has the following
>> header:
>>    #include <xen/hypercall.h> /* for do_mca */
>>    which in turn leads to x86/include/asm/hypercall.h, which includes 
>> the
>> following:
>>    #include <public/arch-x86/xen-mca.h> /* for do_mca */
>> 
>>    where I can't see a declaration for do_mca, as I would have 
>> expected.
>> I'd like to understand what's going on here, since I may be missing 
>> some
>> piece of information (perhaps something is generated during the 
>> build).
> 
> It can't possibly live in the public header. The comment simply went
> stale with the auto-generation of headers; the decl is in 
> hypercall-defs.h
> now.
> 

Ok, thanks.

>> - x86/traps.c do_general_protection may want a declaration in
>> x86/include/asm/traps.h, or perhaps it should gain the asmlinkage
>> attribute, given that it's used only by asm and the TU that defines 
>> it.
> 
> Neither is really attractive imo.
> 
>> - function test and variable data in x86/efi/check.c look like they
>> should not be MISRA compliant, so they may be added to the
>> exclude-list.json
> 
> This file isn't contributing to the final binary.
> 

Then I'll exclude them

>> - given the comment in xen/common/page_alloc.c for first_valid_mfn
>> 
>> /*
>>   * first_valid_mfn is exported because it is use in ARM specific NUMA
>>   * helpers. See comment in arch/arm/include/asm/numa.h.
>>   */
>> mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
>> 
>> and the related ARM comment
>> 
>> /*
>>   * TODO: make first_valid_mfn static when NUMA is supported on Arm, 
>> this
>>   * is required because the dummy helpers are using it.
>>   */
>> extern mfn_t first_valid_mfn;
>> 
>> it should probably be deviated.
> 
> NUMA work is still in progress for Arm, I think, so I'd rather wait 
> with
> deviating.
> 

+Stefano

I can leave it as is, if that's indeed going to become static at some 
point.

>> - compat_set_{px,cx}_pminfo in x86/x86_64/cpufreq.c are perhaps 
>> declared
>> with an autogenerated header?
> 
> I don't think so. Only top-level hypercall handlers would be. This 
> works by
> (perhaps even unintentional) trickery: xen/pmstat.h is included only 
> after
> set_{c,p}x_pminfo are re-defined to compat_set_{c,p}x_pminfo, so the 
> same
> declarations happen to serve two purposes (but of course don't provide 
> the
> intended caller/callee agreement).
> 

I didn't understand your explanation fully; I see xen/pmstat.h in 
cpufreq.c included before
compat/platform.h which, as I understand it, redefines set_{c,p}x_pminfo 
to compat_set_{c,p}x_pminfo, therefore down below no declaration for 
compat_set_{c,p}x_pminfo is visible, triggering the violation.

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

Re: MISRA: Compatible declarations for sort and bsearch

[Jan Beulich]

On 27.11.2023 18:57, Nicola Vetrini wrote:
> On 2023-11-27 15:59, Jan Beulich wrote:
>> On 27.11.2023 15:32, Nicola Vetrini wrote:
>>> - compat_set_{px,cx}_pminfo in x86/x86_64/cpufreq.c are perhaps 
>>> declared
>>> with an autogenerated header?
>>
>> I don't think so. Only top-level hypercall handlers would be. This 
>> works by
>> (perhaps even unintentional) trickery: xen/pmstat.h is included only 
>> after
>> set_{c,p}x_pminfo are re-defined to compat_set_{c,p}x_pminfo, so the 
>> same
>> declarations happen to serve two purposes (but of course don't provide 
>> the
>> intended caller/callee agreement).
>>
> 
> I didn't understand your explanation fully; I see xen/pmstat.h in 
> cpufreq.c included before
> compat/platform.h which, as I understand it, redefines set_{c,p}x_pminfo 
> to compat_set_{c,p}x_pminfo, therefore down below no declaration for 
> compat_set_{c,p}x_pminfo is visible, triggering the violation.

May I suggest that you do what I also did in order to answer your original
question (and which imo you could have done up front): Generate the
preprocessed file(s) and check where the declaration(s) are? After all I
assume you understand that the compiler would choke if indeed there was no
declaration before use. The issue here is that there is a declaration
before use, but there is no (suitable) declaration before the definition.
Hence figuring out where the declaration is and why it won't be possible
to also use it for the definition (simply because there's nothing you can
include to achieve that effect) is the first step.

The (as said imo unintended) trickery can actually also be broken, by
simply adding an explicit inclusion of xen/pmstat.h to
x86_64/platform_hypercall.c near the top of the file. And it looks to me
as if adding this #include there is going to want to be part of the
eventual change (to deliberately make said trickery not work anymore).

Jan

Re: MISRA: Compatible declarations for sort and bsearch

[Jan Beulich]

On 24.11.2023 10:40, Nicola Vetrini wrote:
> in xen/lib.h and xen/sort.h there are definitions of the functions 
> bsearch and sort that have no prior declarations, and therefore are 
> subject to a violation of MISRA C Rule 8.4.
> 
> I'm wondering whether it would be preferred
> 
> 1. to put a declaration just before the definition, in lib.h and sort.h
> 2. deviate these functions, as their signatures are well-known and 
> somewhat standardized

Seeing that so far no-one else has replied to this: My take is "neither".
It is the very nature of extern gnu_inline functions to work like this.

Jan

Re: MISRA: Compatible declarations for sort and bsearch

[Nicola Vetrini]

On 2023-11-28 09:56, Jan Beulich wrote:
> On 24.11.2023 10:40, Nicola Vetrini wrote:
>> in xen/lib.h and xen/sort.h there are definitions of the functions
>> bsearch and sort that have no prior declarations, and therefore are
>> subject to a violation of MISRA C Rule 8.4.
>> 
>> I'm wondering whether it would be preferred
>> 
>> 1. to put a declaration just before the definition, in lib.h and 
>> sort.h
>> 2. deviate these functions, as their signatures are well-known and
>> somewhat standardized
> 
> Seeing that so far no-one else has replied to this: My take is 
> "neither".
> It is the very nature of extern gnu_inline functions to work like this.
> 

This could well be a justification text: violations of required rules 
need either to be fixed or justified.

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

Re: MISRA: Compatible declarations for sort and bsearch

[Stefano Stabellini]

On Mon, 27 Nov 2023, Nicola Vetrini wrote:
> > > /*
> > >   * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> > >   * is required because the dummy helpers are using it.
> > >   */
> > > extern mfn_t first_valid_mfn;
> > > 
> > > it should probably be deviated.
> > 
> > NUMA work is still in progress for Arm, I think, so I'd rather wait with
> > deviating.
> > 
> 
> +Stefano
> 
> I can leave it as is, if that's indeed going to become static at some point.

I see the point in waiting given the TODO comment, but I wouldn't want
this issue to be the only thing standing between us and zero violation
of Rule 8.4 on ARM. So I think we should add SAF to the comment and
remove it when not necessary any longer. 

Re: MISRA: Compatible declarations for sort and bsearch

[Nicola Vetrini]

On 2023-11-29 04:26, Stefano Stabellini wrote:
> On Mon, 27 Nov 2023, Nicola Vetrini wrote:
>> > > /*
>> > >   * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>> > >   * is required because the dummy helpers are using it.
>> > >   */
>> > > extern mfn_t first_valid_mfn;
>> > >
>> > > it should probably be deviated.
>> >
>> > NUMA work is still in progress for Arm, I think, so I'd rather wait with
>> > deviating.
>> >
>> 
>> +Stefano
>> 
>> I can leave it as is, if that's indeed going to become static at some 
>> point.
> 
> I see the point in waiting given the TODO comment, but I wouldn't want
> this issue to be the only thing standing between us and zero violation
> of Rule 8.4 on ARM. So I think we should add SAF to the comment and
> remove it when not necessary any longer.

Ok, thanks.

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