Message ID | 30fdada8b08b2060c6f1ccc2ecb06d418efd97b0.1690966632.git.nicola.vetrini@bugseng.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [XEN,v2] x86/cpu-policy: justify a violation of MISRA C:2012 Rule 1.3 | expand |
On 02.08.2023 10:57, Nicola Vetrini wrote: > The empty feature set 'str_7c1' in 'tools/misc/xen-cpuid.c' causes the > struct declaration to have no named members, hence violating > Rule 1.3: > "There shall be no occurrence of undefined or critical unspecified behaviour" > because it is forbidden by ISO/IEC 9899:1999(E), Section 6.7.2.1.7: > "If the struct-declaration-list contains no named > members, the behavior is undefined." > > Given that Xen is using an undocumented GCC extension that specifies the > behaviour upon defining a struct with no named member, this construct is > well-defined and thus it is marked as safe. Especially after realizing that I was wrong here (I was under the wrong impression that we'd generate a struct without members, when it's one without named members, yet [to me at least] only the former is a known gcc extension we use), I've sent an alternative proposal. Let's see whether in particular Andrew considers this acceptable. Jan
On 02/08/2023 11:47, Jan Beulich wrote: > On 02.08.2023 10:57, Nicola Vetrini wrote: >> The empty feature set 'str_7c1' in 'tools/misc/xen-cpuid.c' causes the >> struct declaration to have no named members, hence violating >> Rule 1.3: >> "There shall be no occurrence of undefined or critical unspecified >> behaviour" >> because it is forbidden by ISO/IEC 9899:1999(E), Section 6.7.2.1.7: >> "If the struct-declaration-list contains no named >> members, the behavior is undefined." >> >> Given that Xen is using an undocumented GCC extension that specifies >> the >> behaviour upon defining a struct with no named member, this construct >> is >> well-defined and thus it is marked as safe. > > Especially after realizing that I was wrong here (I was under the wrong > impression that we'd generate a struct without members, when it's one > without named members, yet [to me at least] only the former is a known > gcc extension we use), I've sent an alternative proposal. Let's see > whether in particular Andrew considers this acceptable. > Well, I don't know the exact discussion on this in that MISRA meeting (25/07 iirc), but the outcome I'm aware of was to deviate that construct because there are possibly others like that in other configurations/future patches. Anyway, this usage does not fall under gcc's documented extensions (either the one you mentioned or the one about unnamed fields, 6.63), but it does allow it (has a warning only on -pedantic afaict), hence why I put undocumented here. In the meantime, I can test your patch to see if it has no unintended impact on other code w.r.t. Rule 1.3. Regards,
On 02.08.2023 12:01, Nicola Vetrini wrote: > On 02/08/2023 11:47, Jan Beulich wrote: >> On 02.08.2023 10:57, Nicola Vetrini wrote: >>> The empty feature set 'str_7c1' in 'tools/misc/xen-cpuid.c' causes the >>> struct declaration to have no named members, hence violating >>> Rule 1.3: >>> "There shall be no occurrence of undefined or critical unspecified >>> behaviour" >>> because it is forbidden by ISO/IEC 9899:1999(E), Section 6.7.2.1.7: >>> "If the struct-declaration-list contains no named >>> members, the behavior is undefined." >>> >>> Given that Xen is using an undocumented GCC extension that specifies >>> the >>> behaviour upon defining a struct with no named member, this construct >>> is >>> well-defined and thus it is marked as safe. >> >> Especially after realizing that I was wrong here (I was under the wrong >> impression that we'd generate a struct without members, when it's one >> without named members, yet [to me at least] only the former is a known >> gcc extension we use), I've sent an alternative proposal. Let's see >> whether in particular Andrew considers this acceptable. >> > > Well, I don't know the exact discussion on this in that MISRA meeting > (25/07 iirc), > but the outcome I'm aware of was to deviate that construct because there > are possibly > others like that in other configurations/future patches. Correct. The goal wants to be to allow for such future changes in as seamless a manner as possible. By tweaking the script generating these struct fields, we can eliminate the present instance and avoid any future instances showing up, and we also won't need to remember to add SAF-* comments at any point in time. Jan
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index e3c8a1d8eb..ec2bd58777 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -12,6 +12,15 @@ }, { "id": "SAF-1-safe", + "analyser": { + "eclair": "MC3R1.R1.3", + "text": "The following declaration of a struct with no named members is deliberate and the use of an undocumented GCC extension ensures that the behaviour is fully defined for that compiler." + }, + "name": "Sentinel", + "text": "Next ID to be used" + }, + { + "id": "SAF-2-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h index bab3eecda6..6b52f080c9 100644 --- a/xen/include/xen/lib/x86/cpu-policy.h +++ b/xen/include/xen/lib/x86/cpu-policy.h @@ -203,6 +203,7 @@ struct cpu_policy }; union { uint32_t _7c1; + /* SAF-1-safe */ struct { DECL_BITFIELD(7c1); }; }; union {
The empty feature set 'str_7c1' in 'tools/misc/xen-cpuid.c' causes the struct declaration to have no named members, hence violating Rule 1.3: "There shall be no occurrence of undefined or critical unspecified behaviour" because it is forbidden by ISO/IEC 9899:1999(E), Section 6.7.2.1.7: "If the struct-declaration-list contains no named members, the behavior is undefined." Given that Xen is using an undocumented GCC extension that specifies the behaviour upon defining a struct with no named member, this construct is well-defined and thus it is marked as safe. No functional changes. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - Added a comment mentioning the use of a GCC extension. Note for v1: As agreed during the MISRA C group meetings, this violation is dealt with by means of a comment deviation, as future changes may eliminate the root cause, which is the empty feature set. My justification for the claim and the commit message may need some adjusting. Note for v2: Note that GCC does not document the particular usage of non-empty structs with no named members, but it works as expected nonetheless. --- docs/misra/safe.json | 9 +++++++++ xen/include/xen/lib/x86/cpu-policy.h | 1 + 2 files changed, 10 insertions(+)