| Message ID | 20220620070245.77979-1-michal.orzel@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | MISRA C 2012 8.1 rule fixes | expand |
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
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
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
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
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
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
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
+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".
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
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".
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
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!
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