[PATCH 0/9] MISRA C 2012 8.1 rule fixes

Michal Orzel posted 9 patches 1 year, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220620070245.77979-1-michal.orzel@arm.com
Test gitlab-ci failed
xen/arch/arm/domain_build.c             |   2 +-
xen/arch/arm/guestcopy.c                |  13 +-
xen/arch/arm/include/asm/arm32/bitops.h |   8 +-
xen/arch/arm/include/asm/fixmap.h       |   4 +-
xen/arch/arm/include/asm/guest_access.h |   8 +-
xen/arch/arm/include/asm/mm.h           |   2 +-
xen/arch/arm/irq.c                      |   2 +-
xen/arch/arm/kernel.c                   |   2 +-
xen/arch/arm/mm.c                       |   4 +-
xen/common/domain.c                     |   2 +-
xen/common/grant_table.c                |   6 +-
xen/common/gunzip.c                     |   8 +-
xen/common/inflate.c                    | 166 ++++++++++++------------
xen/common/libfdt/fdt_ro.c              |   4 +-
xen/common/libfdt/fdt_rw.c              |   2 +-
xen/common/libfdt/fdt_sw.c              |   2 +-
xen/common/libfdt/fdt_wip.c             |   2 +-
xen/common/sched/cpupool.c              |   4 +-
xen/common/trace.c                      |   2 +-
xen/drivers/acpi/tables/tbfadt.c        |   2 +-
xen/drivers/acpi/tables/tbutils.c       |   2 +-
xen/include/public/physdev.h            |   4 +-
xen/include/public/sysctl.h             |  10 +-
xen/include/xen/domain.h                |   2 +-
xen/include/xen/perfc.h                 |   2 +-
xen/include/xen/sched.h                 |   6 +-
xen/xsm/flask/ss/avtab.c                |   2 +-
27 files changed, 137 insertions(+), 136 deletions(-)
[PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Michal Orzel 1 year, 11 months ago
This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
Fixing this rule comes down to replacing implicit 'unsigned' with explicit
'unsigned int' type as there are no other violations being part of that rule
in the Xen codebase.

The last three patches contain fixes for files that originated from other
projects like Linux kernel or libfdt, therefore they are rather not applicable
to be fixed in Xen (they can be dropped). Nevertheless they act as a sign to
what should be added to a deviation list.

Some important notes:
Static analyzers are not perfect. Cppcheck generates internal AST error for some
of the files resulting in skipping all the checks. For these files, one need to
manually check if there are no findings.

Recently there was a release of a new Cppcheck 2.8 version. However it contains
a regression bug with misra addon. Therefore do not use that version for now.

Michal Orzel (9):
  xen/arm: Use explicitly specified types
  xen/domain: Use explicitly specified types
  xen/common: Use explicitly specified types
  include/xen: Use explicitly specified types
  include/public: Use explicitly specified types
  xsm/flask: Use explicitly specified types
  common/libfdt: Use explicitly specified types
  common/inflate: Use explicitly specified types
  drivers/acpi: Use explicitly specified types

 xen/arch/arm/domain_build.c             |   2 +-
 xen/arch/arm/guestcopy.c                |  13 +-
 xen/arch/arm/include/asm/arm32/bitops.h |   8 +-
 xen/arch/arm/include/asm/fixmap.h       |   4 +-
 xen/arch/arm/include/asm/guest_access.h |   8 +-
 xen/arch/arm/include/asm/mm.h           |   2 +-
 xen/arch/arm/irq.c                      |   2 +-
 xen/arch/arm/kernel.c                   |   2 +-
 xen/arch/arm/mm.c                       |   4 +-
 xen/common/domain.c                     |   2 +-
 xen/common/grant_table.c                |   6 +-
 xen/common/gunzip.c                     |   8 +-
 xen/common/inflate.c                    | 166 ++++++++++++------------
 xen/common/libfdt/fdt_ro.c              |   4 +-
 xen/common/libfdt/fdt_rw.c              |   2 +-
 xen/common/libfdt/fdt_sw.c              |   2 +-
 xen/common/libfdt/fdt_wip.c             |   2 +-
 xen/common/sched/cpupool.c              |   4 +-
 xen/common/trace.c                      |   2 +-
 xen/drivers/acpi/tables/tbfadt.c        |   2 +-
 xen/drivers/acpi/tables/tbutils.c       |   2 +-
 xen/include/public/physdev.h            |   4 +-
 xen/include/public/sysctl.h             |  10 +-
 xen/include/xen/domain.h                |   2 +-
 xen/include/xen/perfc.h                 |   2 +-
 xen/include/xen/sched.h                 |   6 +-
 xen/xsm/flask/ss/avtab.c                |   2 +-
 27 files changed, 137 insertions(+), 136 deletions(-)

-- 
2.25.1
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Jan Beulich 1 year, 11 months ago
On 20.06.2022 09:02, Michal Orzel wrote:
> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
> 'unsigned int' type as there are no other violations being part of that rule
> in the Xen codebase.

I'm puzzled, I have to admit. While I agree with all the examples in the
doc, I notice that there's no instance of "signed" or "unsigned" there.
Which matches my understanding that "unsigned" and "signed" on their own
(just like "long") are proper types, and hence the omission of "int"
there is not an "omission of an explicit type".

Nevertheless I think we have had the intention to use "unsigned int"
everywhere, but simply for cosmetic / style reasons (while I didn't ever
see anyone request the use of "long int" in place of "long", despite it
also being possible to combine with "double"), so I'm happy to see this
being changed. Just that (for now) I don't buy the justification.

Jan
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Michal Orzel 1 year, 11 months ago
Hi Jan,

On 22.06.2022 12:25, Jan Beulich wrote:
> On 20.06.2022 09:02, Michal Orzel wrote:
>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>> 'unsigned int' type as there are no other violations being part of that rule
>> in the Xen codebase.
> 
> I'm puzzled, I have to admit. While I agree with all the examples in the
> doc, I notice that there's no instance of "signed" or "unsigned" there.
> Which matches my understanding that "unsigned" and "signed" on their own
> (just like "long") are proper types, and hence the omission of "int"
> there is not an "omission of an explicit type".
> 
Cppcheck was choosed as a tool for MISRA checking and it is considering it as a violation.
It treats unsigned as an implicit type. You can see this flag in cppcheck source code:

"fIsImplicitInt          = (1U << 31),   // Is "int" token implicitly added?"

> Nevertheless I think we have had the intention to use "unsigned int"
> everywhere, but simply for cosmetic / style reasons (while I didn't ever
> see anyone request the use of "long int" in place of "long", despite it
> also being possible to combine with "double"), so I'm happy to see this
> being changed. Just that (for now) I don't buy the justification.
> 
> Jan

Cheers,
Michal
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Jan Beulich 1 year, 11 months ago
On 22.06.2022 14:55, Michal Orzel wrote:
> On 22.06.2022 12:25, Jan Beulich wrote:
>> On 20.06.2022 09:02, Michal Orzel wrote:
>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>>> 'unsigned int' type as there are no other violations being part of that rule
>>> in the Xen codebase.
>>
>> I'm puzzled, I have to admit. While I agree with all the examples in the
>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>> Which matches my understanding that "unsigned" and "signed" on their own
>> (just like "long") are proper types, and hence the omission of "int"
>> there is not an "omission of an explicit type".
>>
> Cppcheck was choosed as a tool for MISRA checking and it is considering it as a violation.

Which by no means indicates that the tool pointing out something as a
violation actually is one.

> It treats unsigned as an implicit type. You can see this flag in cppcheck source code:
> 
> "fIsImplicitInt          = (1U << 31),   // Is "int" token implicitly added?"

Neither the name of the variable nor the comment clarify that this is about
the specific case of "unsigned". As said there's also the fact that they
don't appear to point out the lack of "int" when seeing plain "long" (or
"long long"). I fully agree that "extern x;" or "const y;" lack explicit
"int".

Jan
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Bertrand Marquis 1 year, 11 months ago
Hi Jan,

> On 22 Jun 2022, at 14:01, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 22.06.2022 14:55, Michal Orzel wrote:
>> On 22.06.2022 12:25, Jan Beulich wrote:
>>> On 20.06.2022 09:02, Michal Orzel wrote:
>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>>>> 'unsigned int' type as there are no other violations being part of that rule
>>>> in the Xen codebase.
>>> 
>>> I'm puzzled, I have to admit. While I agree with all the examples in the
>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>>> Which matches my understanding that "unsigned" and "signed" on their own
>>> (just like "long") are proper types, and hence the omission of "int"
>>> there is not an "omission of an explicit type".
>>> 
>> Cppcheck was choosed as a tool for MISRA checking and it is considering it as a violation.
> 
> Which by no means indicates that the tool pointing out something as a
> violation actually is one.
> 
>> It treats unsigned as an implicit type. You can see this flag in cppcheck source code:
>> 
>> "fIsImplicitInt          = (1U << 31),   // Is "int" token implicitly added?"
> 
> Neither the name of the variable nor the comment clarify that this is about
> the specific case of "unsigned". As said there's also the fact that they
> don't appear to point out the lack of "int" when seeing plain "long" (or
> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
> "int".

I am a bit puzzled here trying to understand what you actually want here.

Do you suggest the change is ok but you are not ok with the fact that is flagged
as MISRA fix even though cppcheck is saying otherwise ?

Bertrand
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Jan Beulich 1 year, 11 months ago
On 22.06.2022 15:55, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 22 Jun 2022, at 14:01, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 22.06.2022 14:55, Michal Orzel wrote:
>>> On 22.06.2022 12:25, Jan Beulich wrote:
>>>> On 20.06.2022 09:02, Michal Orzel wrote:
>>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>>>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>>>>> 'unsigned int' type as there are no other violations being part of that rule
>>>>> in the Xen codebase.
>>>>
>>>> I'm puzzled, I have to admit. While I agree with all the examples in the
>>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>>>> Which matches my understanding that "unsigned" and "signed" on their own
>>>> (just like "long") are proper types, and hence the omission of "int"
>>>> there is not an "omission of an explicit type".
>>>>
>>> Cppcheck was choosed as a tool for MISRA checking and it is considering it as a violation.
>>
>> Which by no means indicates that the tool pointing out something as a
>> violation actually is one.
>>
>>> It treats unsigned as an implicit type. You can see this flag in cppcheck source code:
>>>
>>> "fIsImplicitInt          = (1U << 31),   // Is "int" token implicitly added?"
>>
>> Neither the name of the variable nor the comment clarify that this is about
>> the specific case of "unsigned". As said there's also the fact that they
>> don't appear to point out the lack of "int" when seeing plain "long" (or
>> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
>> "int".
> 
> I am a bit puzzled here trying to understand what you actually want here.
> 
> Do you suggest the change is ok but you are not ok with the fact that is flagged
> as MISRA fix even though cppcheck is saying otherwise ?

First of all I'd like to understand whether what we're talking about here
are actually violations (and if so, why that is). Further actions / requests
depend on the answer.

Jan
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Bertrand Marquis 1 year, 11 months ago
Hi,

> On 22 Jun 2022, at 15:10, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 22.06.2022 15:55, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 22 Jun 2022, at 14:01, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 22.06.2022 14:55, Michal Orzel wrote:
>>>> On 22.06.2022 12:25, Jan Beulich wrote:
>>>>> On 20.06.2022 09:02, Michal Orzel wrote:
>>>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>>>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>>>>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>>>>>> 'unsigned int' type as there are no other violations being part of that rule
>>>>>> in the Xen codebase.
>>>>> 
>>>>> I'm puzzled, I have to admit. While I agree with all the examples in the
>>>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>>>>> Which matches my understanding that "unsigned" and "signed" on their own
>>>>> (just like "long") are proper types, and hence the omission of "int"
>>>>> there is not an "omission of an explicit type".
>>>>> 
>>>> Cppcheck was choosed as a tool for MISRA checking and it is considering it as a violation.
>>> 
>>> Which by no means indicates that the tool pointing out something as a
>>> violation actually is one.
>>> 
>>>> It treats unsigned as an implicit type. You can see this flag in cppcheck source code:
>>>> 
>>>> "fIsImplicitInt = (1U << 31), // Is "int" token implicitly added?"
>>> 
>>> Neither the name of the variable nor the comment clarify that this is about
>>> the specific case of "unsigned". As said there's also the fact that they
>>> don't appear to point out the lack of "int" when seeing plain "long" (or
>>> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
>>> "int".
>> 
>> I am a bit puzzled here trying to understand what you actually want here.
>> 
>> Do you suggest the change is ok but you are not ok with the fact that is flagged
>> as MISRA fix even though cppcheck is saying otherwise ?
> 
> First of all I'd like to understand whether what we're talking about here
> are actually violations (and if so, why that is). Further actions / requests
> depend on the answer.

I would say yes but this would need to be confirmed by Roberto I guess.
In any case if we think this is something we want and the change is right
and cppcheck is saying that it solves a violation, I am wondering what is
the point of discussing that extensively this change.

Cheers
Bertrand
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Jan Beulich 1 year, 11 months ago
On 22.06.2022 16:27, Bertrand Marquis wrote:
> Hi,
> 
>> On 22 Jun 2022, at 15:10, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 22.06.2022 15:55, Bertrand Marquis wrote:
>>> Hi Jan,
>>>
>>>> On 22 Jun 2022, at 14:01, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 22.06.2022 14:55, Michal Orzel wrote:
>>>>> On 22.06.2022 12:25, Jan Beulich wrote:
>>>>>> On 20.06.2022 09:02, Michal Orzel wrote:
>>>>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>>>>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>>>>>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>>>>>>> 'unsigned int' type as there are no other violations being part of that rule
>>>>>>> in the Xen codebase.
>>>>>>
>>>>>> I'm puzzled, I have to admit. While I agree with all the examples in the
>>>>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>>>>>> Which matches my understanding that "unsigned" and "signed" on their own
>>>>>> (just like "long") are proper types, and hence the omission of "int"
>>>>>> there is not an "omission of an explicit type".
>>>>>>
>>>>> Cppcheck was choosed as a tool for MISRA checking and it is considering it as a violation.
>>>>
>>>> Which by no means indicates that the tool pointing out something as a
>>>> violation actually is one.
>>>>
>>>>> It treats unsigned as an implicit type. You can see this flag in cppcheck source code:
>>>>>
>>>>> "fIsImplicitInt = (1U << 31), // Is "int" token implicitly added?"
>>>>
>>>> Neither the name of the variable nor the comment clarify that this is about
>>>> the specific case of "unsigned". As said there's also the fact that they
>>>> don't appear to point out the lack of "int" when seeing plain "long" (or
>>>> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
>>>> "int".
>>>
>>> I am a bit puzzled here trying to understand what you actually want here.
>>>
>>> Do you suggest the change is ok but you are not ok with the fact that is flagged
>>> as MISRA fix even though cppcheck is saying otherwise ?
>>
>> First of all I'd like to understand whether what we're talking about here
>> are actually violations (and if so, why that is). Further actions / requests
>> depend on the answer.
> 
> I would say yes but this would need to be confirmed by Roberto I guess.
> In any case if we think this is something we want and the change is right
> and cppcheck is saying that it solves a violation, I am wondering what is
> the point of discussing that extensively this change.

Because imo a patch shouldn't be committed with a misleading description,
if at all possible. Even less so several such patches in one go.

Jan
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Stefano Stabellini 1 year, 11 months ago
+Roberto


Hi Roberto,

A quick question about Rule 8.1.


Michal sent a patch series to fix Xen against Rule 8.1 (here is a link
if you are interested: https://marc.info/?l=xen-devel&m=165570851227125)

Although we all generally agree that the changes are a good thing, there
was a question about the rule itself. Specifically, is the following
actually a violation?

  unsigned x;


Looking through the examples in the MISRA document I can see various
instances of more confusing and obvious violations such as:

  const x;
  extern x;

but no examples of using "unsigned" without "int". Do you know if it is
considered a violation?


Thanks!

Cheers,

Stefano



On Wed, 22 Jun 2022, Jan Beulich wrote:
> >>>>> On 22.06.2022 12:25, Jan Beulich wrote:
> >>>>>> On 20.06.2022 09:02, Michal Orzel wrote:
> >>>>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
> >>>>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
> >>>>>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
> >>>>>>> 'unsigned int' type as there are no other violations being part of that rule
> >>>>>>> in the Xen codebase.
> >>>>>>
> >>>>>> I'm puzzled, I have to admit. While I agree with all the examples in the
> >>>>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
> >>>>>> Which matches my understanding that "unsigned" and "signed" on their own
> >>>>>> (just like "long") are proper types, and hence the omission of "int"
> >>>>>> there is not an "omission of an explicit type".

[...]

> >>>> Neither the name of the variable nor the comment clarify that this is about
> >>>> the specific case of "unsigned". As said there's also the fact that they
> >>>> don't appear to point out the lack of "int" when seeing plain "long" (or
> >>>> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
> >>>> "int".
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Roberto Bagnara 1 year, 10 months ago
Hi there.

Rule 8.1 only applies to C90 code, as all the violating instances are
syntax errors in C99 and later versions of the language.  So,
the following line does not contain a violation of Rule 8.1:

     unsigned x;

It does contain a violation of Directive 4.6, though, whose correct
handling depends on the intention (uint32_t, uin64_t, size_t, ...).

As a side note (still on the theme of the many ways of referring
to a concrete type) Rule 6.1 says not to use plain int for a bitfield
and using an explicitly signed or unsigned type instead.
(Note that Directive 4.6 does not apply to bitfield types.)
So

     int field1:2;

is not compliant; the following are compliant:

     signed int   field1:2;
     unsigned int field2:3;

But also the following are compliant, and we much favor
this variant as the specification of "int" buys nothing
and can even mislead someone into thinking that more bits
are reserved:

     signed   field1:2;
     unsigned field2:3;

I mention this to encourage, as a matter of style, not to add
"int" on bitfield types currently specified as "signed" or "unsigned".
Kind regards,

    Roberto

On 22/06/22 21:23, Stefano Stabellini wrote:
> +Roberto
> 
> 
> Hi Roberto,
> 
> A quick question about Rule 8.1.
> 
> 
> Michal sent a patch series to fix Xen against Rule 8.1 (here is a link
> if you are interested: https://marc.info/?l=xen-devel&m=165570851227125)
> 
> Although we all generally agree that the changes are a good thing, there
> was a question about the rule itself. Specifically, is the following
> actually a violation?
> 
>    unsigned x;
> 
> 
> Looking through the examples in the MISRA document I can see various
> instances of more confusing and obvious violations such as:
> 
>    const x;
>    extern x;
> 
> but no examples of using "unsigned" without "int". Do you know if it is
> considered a violation?
> 
> 
> Thanks!
> 
> Cheers,
> 
> Stefano
> 
> 
> 
> On Wed, 22 Jun 2022, Jan Beulich wrote:
>>>>>>> On 22.06.2022 12:25, Jan Beulich wrote:
>>>>>>>> On 20.06.2022 09:02, Michal Orzel wrote:
>>>>>>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported by
>>>>>>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target allyesconfig).
>>>>>>>>> Fixing this rule comes down to replacing implicit 'unsigned' with explicit
>>>>>>>>> 'unsigned int' type as there are no other violations being part of that rule
>>>>>>>>> in the Xen codebase.
>>>>>>>>
>>>>>>>> I'm puzzled, I have to admit. While I agree with all the examples in the
>>>>>>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>>>>>>>> Which matches my understanding that "unsigned" and "signed" on their own
>>>>>>>> (just like "long") are proper types, and hence the omission of "int"
>>>>>>>> there is not an "omission of an explicit type".
> 
> [...]
> 
>>>>>> Neither the name of the variable nor the comment clarify that this is about
>>>>>> the specific case of "unsigned". As said there's also the fact that they
>>>>>> don't appear to point out the lack of "int" when seeing plain "long" (or
>>>>>> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
>>>>>> "int".
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Jan Beulich 1 year, 10 months ago
On 23.06.2022 09:37, Roberto Bagnara wrote:
> Rule 8.1 only applies to C90 code, as all the violating instances are
> syntax errors in C99 and later versions of the language.  So,
> the following line does not contain a violation of Rule 8.1:
> 
>      unsigned x;
> 
> It does contain a violation of Directive 4.6, though, whose correct
> handling depends on the intention (uint32_t, uin64_t, size_t, ...).

Interesting - this goes straight against a rule we have set in
./CODING_STYLE. I'm also puzzled by you including size_t in your list
of examples, when the spec doesn't. The sole "goal" of the directive
(which is advisory only anyway) is to be able to determine allocation
size. size_t size, however, varies as much as short, int, long, etc
do.

Jan
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Roberto Bagnara 1 year, 10 months ago
Hi Jan.

I know I will sound pedantic ;-)  but an important fact about
the MISRA standards is that reading the headline alone is almost
never enough.  In the specific of (advisory) Directive 4.6,
the Rationale says, among other things:

     It might be desirable not to apply this guideline when
     interfacing with The Standard Library or code outside
     the project’s control.

For this reason, size_t is typically set as an exception in the
tool configuration.  To properly deal with the many Standard Library
functions returning int, one can use a typedef named something
like "lib_int_t" to write, e.g.,

   const lib_int_t r = strncmp(...);

The lib_int_t typedef can be used with a suitable tool configuration,
just as I mentioned one would do with size_t.
Kind regards,

    Roberto

On 23/06/22 09:51, Jan Beulich wrote:
> On 23.06.2022 09:37, Roberto Bagnara wrote:
>> Rule 8.1 only applies to C90 code, as all the violating instances are
>> syntax errors in C99 and later versions of the language.  So,
>> the following line does not contain a violation of Rule 8.1:
>>
>>       unsigned x;
>>
>> It does contain a violation of Directive 4.6, though, whose correct
>> handling depends on the intention (uint32_t, uin64_t, size_t, ...).
> 
> Interesting - this goes straight against a rule we have set in
> ./CODING_STYLE. I'm also puzzled by you including size_t in your list
> of examples, when the spec doesn't. The sole "goal" of the directive
> (which is advisory only anyway) is to be able to determine allocation
> size. size_t size, however, varies as much as short, int, long, etc
> do.
> 
> Jan

Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Stefano Stabellini 1 year, 10 months ago
On Thu, 23 Jun 2022, Jan Beulich wrote:
> On 23.06.2022 09:37, Roberto Bagnara wrote:
> > Rule 8.1 only applies to C90 code, as all the violating instances are
> > syntax errors in C99 and later versions of the language.  So,
> > the following line does not contain a violation of Rule 8.1:
> > 
> >      unsigned x;
> > 
> > It does contain a violation of Directive 4.6, though, whose correct
> > handling depends on the intention (uint32_t, uin64_t, size_t, ...).

Hi Roberto,

Thank you very much for the quick reply and very clear answer!


> Interesting - this goes straight against a rule we have set in
> ./CODING_STYLE. I'm also puzzled by you including size_t in your list
> of examples, when the spec doesn't. The sole "goal" of the directive
> (which is advisory only anyway) is to be able to determine allocation
> size. size_t size, however, varies as much as short, int, long, etc
> do.

I wouldn't worry about Directive 4.6 for now. We'll talk about it when
we get to it. (Also we already require uint32_t, uint64_t, etc. in all
external interfaces and ABIs which I think is what Dir 4.6 cares about
the most.)

For this series, I suggest to keep the patches because "unsigned int" is
better than "unsigned" from a style perspective, but we need to rephrase
the commit messages because we cannot claim they are fixing Rule 8.1.
Also, thank Jan for spotting the misunderstanding!
Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes
Posted by Jan Beulich 1 year, 10 months ago
On 22.06.2022 21:23, Stefano Stabellini wrote:
> A quick question about Rule 8.1.
> 
> 
> Michal sent a patch series to fix Xen against Rule 8.1 (here is a link
> if you are interested: https://marc.info/?l=xen-devel&m=165570851227125)
> 
> Although we all generally agree that the changes are a good thing, there
> was a question about the rule itself. Specifically, is the following
> actually a violation?
> 
>   unsigned x;
> 
> 
> Looking through the examples in the MISRA document I can see various
> instances of more confusing and obvious violations such as:
> 
>   const x;
>   extern x;
> 
> but no examples of using "unsigned" without "int". Do you know if it is
> considered a violation?

And if it is, by implication would plain "long" also be a violation?

Jan