| Message ID | alpine.DEB.2.22.394.2404161227340.2257106@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | docs/misra: add R21.6 R21.14 R21.15 R21.16 | expand |
Hi Stefano, > Other deviations: > ----------------- > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst > index b7b447e152..00db02ad34 100644 > --- a/docs/misra/rules.rst > +++ b/docs/misra/rules.rst > @@ -652,12 +652,38 @@ maintainers if you want to suggest a change. > declared > - See comment for Rule 21.1 > > + * - `Rule 21.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_06.c>`_ > + - Required > + - The Standard Library input/output routines shall not be used > + - See the snprintf() and vsnprintf() deviation in deviations.rst > + > * - `Rule 21.13 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_13.c>`_ > - Mandatory > - Any value passed to a function in <ctype.h> shall be representable as an > unsigned char or be the value EOF > - > > + * - `Rule 21.14 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_14.c>`_ > + - Required > + - The Standard Library function memcmp shall not be used to compare > + null terminated strings > + - > + > + * - `Rule 21.15 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_15.c>`_ > + - Required > + - The pointer arguments to the Standard Library functions memcpy, > + memmove and memcmp shall be pointers to qualified or unqualified > + versions of compatible types There is a trailing space at the end of this line > + - void* arguments are allowed, see deviations.rst > + > + * - `Rule 21.16 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_16.c>`_ > + - Required > + - The pointer arguments to the Standard Library function memcmp > + shall point to either a pointer type, an essentially signed type, > + an essentially unsigned type, an essentially Boolean type or an > + essentially enum type Also here. > + - > + > * - `Rule 21.17 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_17.c>`_ > - Mandatory > - Use of the string handling functions from <string.h> shall not result in > Apart from them, that I guess can be addressed on commit, it looks good to me, I’ve also tested that the changes don’t break convert_misra_doc.py build. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Hi Stefano, On 16/04/2024 20:27, Stefano Stabellini wrote: > Also add two specific project-wide deviations for R21.6 and R21.15. In general, I am fine with add the two rules. However... > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > index 32b02905d1..9123c8edb5 100644 > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: > of the Rule due to uses of this macro. > - Tagged as `deliberate` for ECLAIR. > > + * - R21.6 > + - The use of snprintf() and vsnprintf() is justifiable as, despite > + the fact that such functions have the same names of the > + corresponding standard library functions, each configuration of > + Xen has a unique implementation for them; the code implementing > + such functions is subject to the analysis, so that any undefined > + or unspecified behavior associated to them falls under the > + responsibility of other MISRA guidelines > + - Tagged as `safe` for ECLAIR. ... this implies that ECLAIR should be modified. But this is not happening in this patch. Looking at history, we tend to keep deviations.rst in sync with ECLAIR, so I think it should be updated here too. Cheers,
On Wed, 17 Apr 2024, Julien Grall wrote: > Hi Stefano, > > On 16/04/2024 20:27, Stefano Stabellini wrote: > > Also add two specific project-wide deviations for R21.6 and R21.15. > > In general, I am fine with add the two rules. However... > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > > index 32b02905d1..9123c8edb5 100644 > > --- a/docs/misra/deviations.rst > > +++ b/docs/misra/deviations.rst > > @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: > > of the Rule due to uses of this macro. > > - Tagged as `deliberate` for ECLAIR. > > + * - R21.6 > > + - The use of snprintf() and vsnprintf() is justifiable as, despite > > + the fact that such functions have the same names of the > > + corresponding standard library functions, each configuration of > > + Xen has a unique implementation for them; the code implementing > > + such functions is subject to the analysis, so that any undefined > > + or unspecified behavior associated to them falls under the > > + responsibility of other MISRA guidelines > > + - Tagged as `safe` for ECLAIR. > > ... this implies that ECLAIR should be modified. But this is not happening in > this patch. Looking at history, we tend to keep deviations.rst in sync with > ECLAIR, so I think it should be updated here too. That is true but I am not very familiar with Eclair config language so it is better if that is done by the Bugseng team. I can merge their patch into this one or vice versa or they could be committed separately (keep in mind that the rule is not enabled in the ECLAIR scan so from a Gitlab-CI point of view we don't require the change to the ECLAIR config although it makes sense). I am happy either way.
Hi Stefano, On 17/04/2024 20:10, Stefano Stabellini wrote: > On Wed, 17 Apr 2024, Julien Grall wrote: >> Hi Stefano, >> >> On 16/04/2024 20:27, Stefano Stabellini wrote: >>> Also add two specific project-wide deviations for R21.6 and R21.15. >> >> In general, I am fine with add the two rules. However... >> >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >>> >>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >>> index 32b02905d1..9123c8edb5 100644 >>> --- a/docs/misra/deviations.rst >>> +++ b/docs/misra/deviations.rst >>> @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: >>> of the Rule due to uses of this macro. >>> - Tagged as `deliberate` for ECLAIR. >>> + * - R21.6 >>> + - The use of snprintf() and vsnprintf() is justifiable as, despite >>> + the fact that such functions have the same names of the >>> + corresponding standard library functions, each configuration of >>> + Xen has a unique implementation for them; the code implementing >>> + such functions is subject to the analysis, so that any undefined >>> + or unspecified behavior associated to them falls under the >>> + responsibility of other MISRA guidelines >>> + - Tagged as `safe` for ECLAIR. >> >> ... this implies that ECLAIR should be modified. But this is not happening in >> this patch. Looking at history, we tend to keep deviations.rst in sync with >> ECLAIR, so I think it should be updated here too. > > That is true but I am not very familiar with Eclair config language so > it is better if that is done by the Bugseng team. I can merge their > patch into this one or vice versa or they could be committed separately > (keep in mind that the rule is not enabled in the ECLAIR scan so from a > Gitlab-CI point of view we don't require the change to the ECLAIR config > although it makes sense). I am happy either way. My comment was not about Gitlab CI. It was more about consistency between the file and ECLAIR. I think they should be kept in sync because it explain how the tool doing the scanning behave. My preference is to either split and only commit the rules now or wait for the ECLAIR change to happen. Cheers,
+Bugseng On Wed, 17 Apr 2024, Julien Grall wrote: > Hi Stefano, > > On 17/04/2024 20:10, Stefano Stabellini wrote: > > On Wed, 17 Apr 2024, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 16/04/2024 20:27, Stefano Stabellini wrote: > > > > Also add two specific project-wide deviations for R21.6 and R21.15. > > > > > > In general, I am fine with add the two rules. However... > > > > > > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > > > > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > > > > index 32b02905d1..9123c8edb5 100644 > > > > --- a/docs/misra/deviations.rst > > > > +++ b/docs/misra/deviations.rst > > > > @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: > > > > of the Rule due to uses of this macro. > > > > - Tagged as `deliberate` for ECLAIR. > > > > + * - R21.6 > > > > + - The use of snprintf() and vsnprintf() is justifiable as, despite > > > > + the fact that such functions have the same names of the > > > > + corresponding standard library functions, each configuration of > > > > + Xen has a unique implementation for them; the code implementing > > > > + such functions is subject to the analysis, so that any undefined > > > > + or unspecified behavior associated to them falls under the > > > > + responsibility of other MISRA guidelines > > > > + - Tagged as `safe` for ECLAIR. > > > > > > ... this implies that ECLAIR should be modified. But this is not happening > > > in > > > this patch. Looking at history, we tend to keep deviations.rst in sync > > > with > > > ECLAIR, so I think it should be updated here too. > > > > That is true but I am not very familiar with Eclair config language so > > it is better if that is done by the Bugseng team. I can merge their > > patch into this one or vice versa or they could be committed separately > > (keep in mind that the rule is not enabled in the ECLAIR scan so from a > > Gitlab-CI point of view we don't require the change to the ECLAIR config > > although it makes sense). I am happy either way. > > My comment was not about Gitlab CI. It was more about consistency between the > file and ECLAIR. I think they should be kept in sync because it explain how > the tool doing the scanning behave. > > My preference is to either split and only commit the rules now or wait for the > ECLAIR change to happen. Understood. Maybe the Bugseng team can provide the corresponding ECLAIR/deviations.ecl changes
On 2024-04-17 23:13, Stefano Stabellini wrote: > +Bugseng > > On Wed, 17 Apr 2024, Julien Grall wrote: >> Hi Stefano, >> >> On 17/04/2024 20:10, Stefano Stabellini wrote: >> > On Wed, 17 Apr 2024, Julien Grall wrote: >> > > Hi Stefano, >> > > >> > > On 16/04/2024 20:27, Stefano Stabellini wrote: >> > > > Also add two specific project-wide deviations for R21.6 and R21.15. >> > > >> > > In general, I am fine with add the two rules. However... >> > > >> > > > >> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >> > > > >> > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >> > > > index 32b02905d1..9123c8edb5 100644 >> > > > --- a/docs/misra/deviations.rst >> > > > +++ b/docs/misra/deviations.rst >> > > > @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: >> > > > of the Rule due to uses of this macro. >> > > > - Tagged as `deliberate` for ECLAIR. >> > > > + * - R21.6 >> > > > + - The use of snprintf() and vsnprintf() is justifiable as, despite >> > > > + the fact that such functions have the same names of the >> > > > + corresponding standard library functions, each configuration of >> > > > + Xen has a unique implementation for them; the code implementing >> > > > + such functions is subject to the analysis, so that any undefined >> > > > + or unspecified behavior associated to them falls under the >> > > > + responsibility of other MISRA guidelines >> > > > + - Tagged as `safe` for ECLAIR. >> > > >> > > ... this implies that ECLAIR should be modified. But this is not happening >> > > in >> > > this patch. Looking at history, we tend to keep deviations.rst in sync >> > > with >> > > ECLAIR, so I think it should be updated here too. >> > >> > That is true but I am not very familiar with Eclair config language so >> > it is better if that is done by the Bugseng team. I can merge their >> > patch into this one or vice versa or they could be committed separately >> > (keep in mind that the rule is not enabled in the ECLAIR scan so from a >> > Gitlab-CI point of view we don't require the change to the ECLAIR config >> > although it makes sense). I am happy either way. >> >> My comment was not about Gitlab CI. It was more about consistency >> between the >> file and ECLAIR. I think they should be kept in sync because it >> explain how >> the tool doing the scanning behave. >> >> My preference is to either split and only commit the rules now or wait >> for the >> ECLAIR change to happen. > > Understood. Maybe the Bugseng team can provide the corresponding > ECLAIR/deviations.ecl changes Sure, we can respin the patch with the appropriate deviation in place.
On 16.04.2024 21:27, Stefano Stabellini wrote: > Also add two specific project-wide deviations for R21.6 and R21.15. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > index 32b02905d1..9123c8edb5 100644 > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: > of the Rule due to uses of this macro. > - Tagged as `deliberate` for ECLAIR. > > + * - R21.6 > + - The use of snprintf() and vsnprintf() is justifiable as, despite > + the fact that such functions have the same names of the > + corresponding standard library functions, each configuration of > + Xen has a unique implementation for them; the code implementing > + such functions is subject to the analysis, so that any undefined > + or unspecified behavior associated to them falls under the > + responsibility of other MISRA guidelines Checking the Misra spec, I'm actually surprised a deviation is needed. The rule's rationale talks about streams and file I/O only. Why would the string formatting functions be covered then at all? They also don't have, afaik, any undefined or implementation defined behavior. > + - Tagged as `safe` for ECLAIR. > + > + * - R21.15 > + - The use of void* arguments is justifiable as the rationale for > + the rule is to indicate possible mistakes, and void* is > + frequently used in Xen to represent virtual memory addresses But that doesn't rule out mistakes. Are there actually examples in the code base? Additionally I wonder (a) whether the rule actually needs an exception and thus (b) whether the deviation isn't instead for 21.16. As to (a) I understand the rule is worded slightly differently than what would strictly be needed to permit void*, but the general rule in C is that void* is compatible with all other pointers (suitably qualified as needed, of course) anyway. Jan
On Thu, 18 Apr 2024, Jan Beulich wrote: > On 16.04.2024 21:27, Stefano Stabellini wrote: > > Also add two specific project-wide deviations for R21.6 and R21.15. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > > index 32b02905d1..9123c8edb5 100644 > > --- a/docs/misra/deviations.rst > > +++ b/docs/misra/deviations.rst > > @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: > > of the Rule due to uses of this macro. > > - Tagged as `deliberate` for ECLAIR. > > > > + * - R21.6 > > + - The use of snprintf() and vsnprintf() is justifiable as, despite > > + the fact that such functions have the same names of the > > + corresponding standard library functions, each configuration of > > + Xen has a unique implementation for them; the code implementing > > + such functions is subject to the analysis, so that any undefined > > + or unspecified behavior associated to them falls under the > > + responsibility of other MISRA guidelines > > Checking the Misra spec, I'm actually surprised a deviation is needed. The > rule's rationale talks about streams and file I/O only. Why would the string > formatting functions be covered then at all? They also don't have, afaik, > any undefined or implementation defined behavior. As discussed during the call, I'll add an explanatory note to rules.rst > > + - Tagged as `safe` for ECLAIR. > > + > > + * - R21.15 > > + - The use of void* arguments is justifiable as the rationale for > > + the rule is to indicate possible mistakes, and void* is > > + frequently used in Xen to represent virtual memory addresses > > But that doesn't rule out mistakes. Are there actually examples in the > code base? If you are asking if there are any violations or bugs, I'll defer to the Bugseng team. > Additionally I wonder (a) whether the rule actually needs an exception Yes my understanding is that a deviation is necessary from MISRA point of view, and if nothing else it will serve as extra clarification. > and thus (b) whether the deviation isn't instead for 21.16. As to (a) I > understand the rule is worded slightly differently than what would > strictly be needed to permit void*, but the general rule in C is that > void* is compatible with all other pointers (suitably qualified as > needed, of course) anyway. Roberto and others, can you please confirm whether we need a deviation on 21.16 as well for similar reasons to 21.15? I am asking because I don't have any notes about requiring a deviation for 21.16 but I would like to check with you.
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index 32b02905d1..9123c8edb5 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -387,6 +387,22 @@ Deviations related to MISRA C:2012 Rules: of the Rule due to uses of this macro. - Tagged as `deliberate` for ECLAIR. + * - R21.6 + - The use of snprintf() and vsnprintf() is justifiable as, despite + the fact that such functions have the same names of the + corresponding standard library functions, each configuration of + Xen has a unique implementation for them; the code implementing + such functions is subject to the analysis, so that any undefined + or unspecified behavior associated to them falls under the + responsibility of other MISRA guidelines + - Tagged as `safe` for ECLAIR. + + * - R21.15 + - The use of void* arguments is justifiable as the rationale for + the rule is to indicate possible mistakes, and void* is + frequently used in Xen to represent virtual memory addresses + - Tagged as `deliberate` for ECLAIR. + Other deviations: ----------------- diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index b7b447e152..00db02ad34 100644 --- a/docs/misra/rules.rst +++ b/docs/misra/rules.rst @@ -652,12 +652,38 @@ maintainers if you want to suggest a change. declared - See comment for Rule 21.1 + * - `Rule 21.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_06.c>`_ + - Required + - The Standard Library input/output routines shall not be used + - See the snprintf() and vsnprintf() deviation in deviations.rst + * - `Rule 21.13 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_13.c>`_ - Mandatory - Any value passed to a function in <ctype.h> shall be representable as an unsigned char or be the value EOF - + * - `Rule 21.14 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_14.c>`_ + - Required + - The Standard Library function memcmp shall not be used to compare + null terminated strings + - + + * - `Rule 21.15 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_15.c>`_ + - Required + - The pointer arguments to the Standard Library functions memcpy, + memmove and memcmp shall be pointers to qualified or unqualified + versions of compatible types + - void* arguments are allowed, see deviations.rst + + * - `Rule 21.16 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_16.c>`_ + - Required + - The pointer arguments to the Standard Library function memcmp + shall point to either a pointer type, an essentially signed type, + an essentially unsigned type, an essentially Boolean type or an + essentially enum type + - + * - `Rule 21.17 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_17.c>`_ - Mandatory - Use of the string handling functions from <string.h> shall not result in
Also add two specific project-wide deviations for R21.6 and R21.15. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>