| Message ID | alpine.DEB.2.22.394.2402091653110.1925432@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] docs/misra/rules.rst: catch up with accepted rules | expand |
On 10.02.2024 02:00, Stefano Stabellini wrote: > Update docs/misra/rules.rst to reflect the MISRA C rules accepted in the > last couple of months. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > --- > > In the notes section I added some info about the deviations, but in any > case the appropriate info will also be added to deviations.rst, > safe.json, etc. > > I also added Rule 14.4, which is older, but when I first tried to add it > to rules.rst, Jan had a question I couldn't reply clearly: > https://marc.info/?l=xen-devel&m=169828285627163 > > I think now with this series, the impact of Rule 14.4 is clearer: > https://marc.info/?l=xen-devel&m=170194257326186 This series is about enums only afaics. Yet the rule is much wider, and iirc we had agreed that for integer and pointer types the normal language conversion to boolean meaning is fine as well. Not only do you not mention this case in the entry, but it also continue to mean that effectively we limit the rule to a very narrow case. Which continue to leave open the question of whether the rule is worthwhile to accept in the first place. Jan
On Mon, 12 Feb 2024, Jan Beulich wrote: > On 10.02.2024 02:00, Stefano Stabellini wrote: > > Update docs/misra/rules.rst to reflect the MISRA C rules accepted in the > > last couple of months. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > --- > > > > In the notes section I added some info about the deviations, but in any > > case the appropriate info will also be added to deviations.rst, > > safe.json, etc. > > > > I also added Rule 14.4, which is older, but when I first tried to add it > > to rules.rst, Jan had a question I couldn't reply clearly: > > https://marc.info/?l=xen-devel&m=169828285627163 > > > > I think now with this series, the impact of Rule 14.4 is clearer: > > https://marc.info/?l=xen-devel&m=170194257326186 > > This series is about enums only afaics. Yet the rule is much wider, and iirc > we had agreed that for integer and pointer types the normal language > conversion to boolean meaning is fine as well. Not only do you not mention > this case in the entry, I can add a note about it. > but it also continue to mean that effectively we > limit the rule to a very narrow case. Which continue to leave open the > question of whether the rule is worthwhile to accept in the first place. When someone does a safety certification, there is a difference between deviating a rule as a whole or accepting the rule and only deviating certain aspects of it (simply ignoring the rule is typically not an option in safety certification context.) So here I think it would help downstreams interested in safety if we added the rule, with specific deviations. Do you have any comments on the other parts of this patch? If not, I would be happy to resent the rest unmodified, and update only 14.4 in its own separate patch where we can discuss further.
On 13.02.2024 00:18, Stefano Stabellini wrote: > On Mon, 12 Feb 2024, Jan Beulich wrote: >> On 10.02.2024 02:00, Stefano Stabellini wrote: >>> Update docs/misra/rules.rst to reflect the MISRA C rules accepted in the >>> last couple of months. >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >>> --- >>> >>> In the notes section I added some info about the deviations, but in any >>> case the appropriate info will also be added to deviations.rst, >>> safe.json, etc. >>> >>> I also added Rule 14.4, which is older, but when I first tried to add it >>> to rules.rst, Jan had a question I couldn't reply clearly: >>> https://marc.info/?l=xen-devel&m=169828285627163 >>> >>> I think now with this series, the impact of Rule 14.4 is clearer: >>> https://marc.info/?l=xen-devel&m=170194257326186 >> >> This series is about enums only afaics. Yet the rule is much wider, and iirc >> we had agreed that for integer and pointer types the normal language >> conversion to boolean meaning is fine as well. Not only do you not mention >> this case in the entry, > > I can add a note about it. > > >> but it also continue to mean that effectively we >> limit the rule to a very narrow case. Which continue to leave open the >> question of whether the rule is worthwhile to accept in the first place. > > When someone does a safety certification, there is a difference between > deviating a rule as a whole or accepting the rule and only deviating > certain aspects of it (simply ignoring the rule is typically not an > option in safety certification context.) So here I think it would help > downstreams interested in safety if we added the rule, with specific > deviations. Yet then in other cases you refer to Bertrand's general statement of it not being helpful when too little of a rule is left by deviating. > Do you have any comments on the other parts of this patch? If not, I > would be happy to resent the rest unmodified, and update only 14.4 in > its own separate patch where we can discuss further. Well. We're in territory now where I'm not really happy anymore with the full scope of what is being added to the "accepted" list. Leaving 14.4 aside, what you have in the patch all looks like what was agreed upon, but then I'm not taking notes during meetings, and hence I can't help the impression that e.g. for 5.5 there was more than just the one "permitted" pattern. Therefore, while I deliberately didn't comment there (for not having a concrete case in mind), I'm afraid I also don't feel anymore like acking such multi-rule patches. If you strictly went one by one, it is certainly possible that I might ack this and that. As attempted to voice several times during the meetings, I pretty strongly disagree with many of the "developer confusion" aspects, when they take away options the language quite obviously and naturally provides. We're talking about hypervisor code here, not some random tool that was thrown together in a haste. At the risk of sounding arrogant, people being easily confused by what I'd call normal code should simply not touch code like this. Whereas the spirit of many of these rules looks to rather go in the direction that basically anyone knowing a little bit of C should be qualified enough to maintain code made subject to all of these rules. I'm sorry, Jan
On Tue, 13 Feb 2024, Jan Beulich wrote: > On 13.02.2024 00:18, Stefano Stabellini wrote: > > On Mon, 12 Feb 2024, Jan Beulich wrote: > >> On 10.02.2024 02:00, Stefano Stabellini wrote: > >>> Update docs/misra/rules.rst to reflect the MISRA C rules accepted in the > >>> last couple of months. > >>> > >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > >>> --- > >>> > >>> In the notes section I added some info about the deviations, but in any > >>> case the appropriate info will also be added to deviations.rst, > >>> safe.json, etc. > >>> > >>> I also added Rule 14.4, which is older, but when I first tried to add it > >>> to rules.rst, Jan had a question I couldn't reply clearly: > >>> https://marc.info/?l=xen-devel&m=169828285627163 > >>> > >>> I think now with this series, the impact of Rule 14.4 is clearer: > >>> https://marc.info/?l=xen-devel&m=170194257326186 > >> > >> This series is about enums only afaics. Yet the rule is much wider, and iirc > >> we had agreed that for integer and pointer types the normal language > >> conversion to boolean meaning is fine as well. Not only do you not mention > >> this case in the entry, > > > > I can add a note about it. > > > > > >> but it also continue to mean that effectively we > >> limit the rule to a very narrow case. Which continue to leave open the > >> question of whether the rule is worthwhile to accept in the first place. > > > > When someone does a safety certification, there is a difference between > > deviating a rule as a whole or accepting the rule and only deviating > > certain aspects of it (simply ignoring the rule is typically not an > > option in safety certification context.) So here I think it would help > > downstreams interested in safety if we added the rule, with specific > > deviations. > > Yet then in other cases you refer to Bertrand's general statement of it > not being helpful when too little of a rule is left by deviating. I think it is a case-by-case judgement call. Also, keep in mind that Bertrand's goal was to encourage us to accept more rules in their entirety without any deviations, not reject more rules. Especially as rejecting rules it is typically not possible for people doing safety, so the alternative is to write a wider deviation -- deviate even more, not less. > > Do you have any comments on the other parts of this patch? If not, I > > would be happy to resent the rest unmodified, and update only 14.4 in > > its own separate patch where we can discuss further. > > Well. We're in territory now where I'm not really happy anymore with the > full scope of what is being added to the "accepted" list. Leaving 14.4 > aside, what you have in the patch all looks like what was agreed upon, > but then I'm not taking notes during meetings, and hence I can't help > the impression that e.g. for 5.5 there was more than just the one > "permitted" pattern. Therefore, while I deliberately didn't comment > there (for not having a concrete case in mind), I'm afraid I also don't > feel anymore like acking such multi-rule patches. If you strictly went > one by one, it is certainly possible that I might ack this and that. That's understandable especially as this time I was slower to send out the patch to update docs/misra/rules.rst (apologies for that.) Specifically for 5.5, I have the following notes from Roberto: """ We tag as deliberate all violations involving clashes between function-like macros and non-callable entities. We also tag as deliberate the violations matching the pattern #define x x. In the future, once string.h will be restricted to the <string.h> functionality of the C standard, string.h will be deviated further. Further deviations are yet to be agreed upon (e.g., on ARM64 the deviation of TYPE_SAFE resolves most of the violation; on x86_64 the deviation of TYPE_SAFE resolves half of the violations but the others depend on only about a dozen of macros). """ I added to the notes section only the part that I thought would make sense to keep there. For the rest, I think it would be best to leave it to future updates of documenting-violations.rst and/or safe.json as appropriate. I'll split out 14.4 and 5.5, and keep the rest in one patch. > As attempted to voice several times during the meetings, I pretty > strongly disagree with many of the "developer confusion" aspects, when > they take away options the language quite obviously and naturally > provides. We're talking about hypervisor code here, not some random > tool that was thrown together in a haste. At the risk of sounding > arrogant, people being easily confused by what I'd call normal code > should simply not touch code like this. Whereas the spirit of many of > these rules looks to rather go in the direction that basically anyone > knowing a little bit of C should be qualified enough to maintain code > made subject to all of these rules. At some point there will be new people working on Xen (among others, AMD has been hiring). New people need to start from somewhere. So the less confusing the code the better it is because it is also less error prone. There is code reviews and tests, but if the code was less prone to misinterpretations, then there would be fewer chances of errors in the first place.
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index 182331089d..0cb87ebc35 100644 --- a/docs/misra/rules.rst +++ b/docs/misra/rules.rst @@ -181,6 +181,12 @@ maintainers if you want to suggest a change. headers (xen/include/public/) are allowed to retain longer identifiers for backward compatibility. + * - `Rule 5.5 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_05.c>`_ + - Required + - Identifiers shall be distinct from macro names + - Clashes between function-like macros and non-callable entities + are allowed. The pattern #define x x is also allowed. + * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ - Required - A typedef name shall be a unique identifier @@ -462,6 +468,13 @@ maintainers if you want to suggest a change. while(0) and while(1) and alike are allowed. + * - `Rule 14.4 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_14_04.c>`_ + - Required + - The controlling expression of an if-statement and the controlling + expression of an iteration-statement shall have essentially + Boolean type + - + * - `Rule 16.3 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_03.c>`_ - Required - An unconditional break statement shall terminate every @@ -478,6 +491,12 @@ maintainers if you want to suggest a change. adhere to Rule 16.2 would result in increased complexity and maintenance difficulty, and could potentially introduce bugs. + * - `Rule 16.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_06.c>`_ + - Required + - Every switch statement shall have at least two switch-clauses + - Single-clause switches are allowed when they do not involve a + default label. + * - `Rule 16.7 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_16_07.c>`_ - Required - A switch-expression shall not have essentially Boolean type @@ -554,6 +573,13 @@ maintainers if you want to suggest a change. evaluation - + * - `Rule 20.12 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_20_12.c>`_ + - A macro parameter used as an operand to the # or ## operators, + which is itself subject to further macro replacement, shall only + be used as an operand to these operators + - Required + - Variadic macros are allowed to violate the rule. + * - `Rule 20.13 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_20_13.c>`_ - Required - A line whose first token is # shall be a valid preprocessing
Update docs/misra/rules.rst to reflect the MISRA C rules accepted in the last couple of months. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- In the notes section I added some info about the deviations, but in any case the appropriate info will also be added to deviations.rst, safe.json, etc. I also added Rule 14.4, which is older, but when I first tried to add it to rules.rst, Jan had a question I couldn't reply clearly: https://marc.info/?l=xen-devel&m=169828285627163 I think now with this series, the impact of Rule 14.4 is clearer: https://marc.info/?l=xen-devel&m=170194257326186