| Message ID | a0341b50ece1ba1b5b346b54db7d2abdc150cb95.1730880832.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address violations of MISRA C Rule 16.3 | expand |
On 06.11.2024 10:04, Federico Serafini wrote: > The pseudo keyword fallthrough shall be used to make explicit the > fallthrough intention at the end of a case statement (doing this > through comments is deprecated). > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > --- > xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) When you had asked my privately on Matrix, I specifically said: "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best, unless problems with that approach turn up." Even if identical re- definitions are deemed fine, I for one consider such bad practice. Yet by playing with this file (and outside of any relevant #ifdef) means there will be such a re-definition when building Xen itself. In fact the patch subject should also already clarify that the auxiliary definition is only needed for the test and fuzzing harnesses. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -23,6 +23,16 @@ > # error Unknown compilation width > #endif > > +/* > + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at > + * the end of a case statement. > + */ > +#if (!defined(__clang__) && (__GNUC__ >= 7)) I realize xen/compiler.h has it like that, but may I ask that you omit the meaningless outer pair of parentheses? Jan
On Wed, 6 Nov 2024, Jan Beulich wrote: > On 06.11.2024 10:04, Federico Serafini wrote: > > The pseudo keyword fallthrough shall be used to make explicit the > > fallthrough intention at the end of a case statement (doing this > > through comments is deprecated). > > > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > > --- > > xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > When you had asked my privately on Matrix, I specifically said: "Adding > the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best, > unless problems with that approach turn up." Even if identical re- > definitions are deemed fine, I for one consider such bad practice. Yet > by playing with this file (and outside of any relevant #ifdef) means > there will be such a re-definition when building Xen itself. > > In fact the patch subject should also already clarify that the auxiliary > definition is only needed for the test and fuzzing harnesses. Hi Jan, I don't understand this comment. You say "playing with this file (and outside of any relevant #ifdef)" but actually the changes are within the #ifndef __X86_EMULATE_H__/#endif. What do you mean? You say "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best". I am not very familiar with x86-isms but the only x86-emulate.h I can find is ./tools/tests/x86_emulator/x86-emulate.h which is not a header that would help define anything for the Xen build? I am not understanding your suggestions. From what I can see this patch looks OK? > > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > > @@ -23,6 +23,16 @@ > > # error Unknown compilation width > > #endif > > > > +/* > > + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at > > + * the end of a case statement. > > + */ > > +#if (!defined(__clang__) && (__GNUC__ >= 7)) > > I realize xen/compiler.h has it like that, but may I ask that you omit > the meaningless outer pair of parentheses? > > Jan >
On 11.11.2024 03:24, Stefano Stabellini wrote: > On Wed, 6 Nov 2024, Jan Beulich wrote: >> On 06.11.2024 10:04, Federico Serafini wrote: >>> The pseudo keyword fallthrough shall be used to make explicit the >>> fallthrough intention at the end of a case statement (doing this >>> through comments is deprecated). >>> >>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >>> --- >>> xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >> >> When you had asked my privately on Matrix, I specifically said: "Adding >> the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best, >> unless problems with that approach turn up." Even if identical re- >> definitions are deemed fine, I for one consider such bad practice. Yet >> by playing with this file (and outside of any relevant #ifdef) means >> there will be such a re-definition when building Xen itself. >> >> In fact the patch subject should also already clarify that the auxiliary >> definition is only needed for the test and fuzzing harnesses. > > Hi Jan, I don't understand this comment. > > You say "playing with this file (and outside of any relevant #ifdef)" > but actually the changes are within the #ifndef > __X86_EMULATE_H__/#endif. What do you mean? "relevant" was specifically to exclude the guard #ifdef. And the remark was to avoid the #define to merely be moved into or framed by an "#ifndef __XEN__". > You say "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h) > is probably best". I am not very familiar with x86-isms but the only > x86-emulate.h I can find is ./tools/tests/x86_emulator/x86-emulate.h > which is not a header that would help define anything for the Xen build? But that's the whole point: We _have_ "fallthrough" as a pseudo-keyword already for the Xen build. For it to be usable in the emulator files, it particularly needs to be made available for the test and fuzzing harnesses. And that without interfering with what the Xen build has. Hence why it wants to go into precisely that file, where all other build compatibility definitions also live. > I am not understanding your suggestions. From what I can see this patch > looks OK? No, it is - first and foremost - the wrong file that is being touched. Jan
On Mon, 11 Nov 2024, Jan Beulich wrote: > On 11.11.2024 03:24, Stefano Stabellini wrote: > > On Wed, 6 Nov 2024, Jan Beulich wrote: > >> On 06.11.2024 10:04, Federico Serafini wrote: > >>> The pseudo keyword fallthrough shall be used to make explicit the > >>> fallthrough intention at the end of a case statement (doing this > >>> through comments is deprecated). > >>> > >>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > >>> --- > >>> xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ > >>> 1 file changed, 10 insertions(+) > >> > >> When you had asked my privately on Matrix, I specifically said: "Adding > >> the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best, > >> unless problems with that approach turn up." Even if identical re- > >> definitions are deemed fine, I for one consider such bad practice. Yet > >> by playing with this file (and outside of any relevant #ifdef) means > >> there will be such a re-definition when building Xen itself. > >> > >> In fact the patch subject should also already clarify that the auxiliary > >> definition is only needed for the test and fuzzing harnesses. > > > > Hi Jan, I don't understand this comment. > > > > You say "playing with this file (and outside of any relevant #ifdef)" > > but actually the changes are within the #ifndef > > __X86_EMULATE_H__/#endif. What do you mean? > > "relevant" was specifically to exclude the guard #ifdef. And the remark > was to avoid the #define to merely be moved into or framed by an > "#ifndef __XEN__". > > > You say "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h) > > is probably best". I am not very familiar with x86-isms but the only > > x86-emulate.h I can find is ./tools/tests/x86_emulator/x86-emulate.h > > which is not a header that would help define anything for the Xen build? > > But that's the whole point: We _have_ "fallthrough" as a pseudo-keyword > already for the Xen build. For it to be usable in the emulator files, it > particularly needs to be made available for the test and fuzzing > harnesses. And that without interfering with what the Xen build has. > Hence why it wants to go into precisely that file, where all other build > compatibility definitions also live. OK. So if I get this right, we need the below instead of patch #1 in this series? diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h index 00abc829b0..380eb8abff 100644 --- a/tools/tests/x86_emulator/x86-emulate.h +++ b/tools/tests/x86_emulator/x86-emulate.h @@ -233,4 +233,14 @@ void emul_test_put_fpu( enum x86_emulate_fpu_type backout, const struct x86_emul_fpu_aux *aux); +/* + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at + * the end of a case statement. + */ +#if (!defined(__clang__) && (__GNUC__ >= 7)) +# define fallthrough __attribute__((__fallthrough__)) +#else +# define fallthrough do {} while (0) /* fallthrough */ +#endif + #endif /* X86_EMULATE_H */
On 12.11.2024 03:16, Stefano Stabellini wrote: > On Mon, 11 Nov 2024, Jan Beulich wrote: >> On 11.11.2024 03:24, Stefano Stabellini wrote: >>> On Wed, 6 Nov 2024, Jan Beulich wrote: >>>> On 06.11.2024 10:04, Federico Serafini wrote: >>>>> The pseudo keyword fallthrough shall be used to make explicit the >>>>> fallthrough intention at the end of a case statement (doing this >>>>> through comments is deprecated). >>>>> >>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >>>>> --- >>>>> xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>> >>>> When you had asked my privately on Matrix, I specifically said: "Adding >>>> the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best, >>>> unless problems with that approach turn up." Even if identical re- >>>> definitions are deemed fine, I for one consider such bad practice. Yet >>>> by playing with this file (and outside of any relevant #ifdef) means >>>> there will be such a re-definition when building Xen itself. >>>> >>>> In fact the patch subject should also already clarify that the auxiliary >>>> definition is only needed for the test and fuzzing harnesses. >>> >>> Hi Jan, I don't understand this comment. >>> >>> You say "playing with this file (and outside of any relevant #ifdef)" >>> but actually the changes are within the #ifndef >>> __X86_EMULATE_H__/#endif. What do you mean? >> >> "relevant" was specifically to exclude the guard #ifdef. And the remark >> was to avoid the #define to merely be moved into or framed by an >> "#ifndef __XEN__". >> >>> You say "Adding the pseudo-keyword to x86-emulate.h (not x86_emulate.h) >>> is probably best". I am not very familiar with x86-isms but the only >>> x86-emulate.h I can find is ./tools/tests/x86_emulator/x86-emulate.h >>> which is not a header that would help define anything for the Xen build? >> >> But that's the whole point: We _have_ "fallthrough" as a pseudo-keyword >> already for the Xen build. For it to be usable in the emulator files, it >> particularly needs to be made available for the test and fuzzing >> harnesses. And that without interfering with what the Xen build has. >> Hence why it wants to go into precisely that file, where all other build >> compatibility definitions also live. > > OK. So if I get this right, we need the below instead of patch #1 in > this series? Yes, just with the addition not at the bottom of the file, but where the other compatibility definitions are. Also (nit) perhaps "statement block", matching terminology in xen/compiler.h. Jan > --- a/tools/tests/x86_emulator/x86-emulate.h > +++ b/tools/tests/x86_emulator/x86-emulate.h > @@ -233,4 +233,14 @@ void emul_test_put_fpu( > enum x86_emulate_fpu_type backout, > const struct x86_emul_fpu_aux *aux); > > +/* > + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at > + * the end of a case statement. > + */ > +#if (!defined(__clang__) && (__GNUC__ >= 7)) > +# define fallthrough __attribute__((__fallthrough__)) > +#else > +# define fallthrough do {} while (0) /* fallthrough */ > +#endif > + > #endif /* X86_EMULATE_H */
On 06/11/24 12:22, Jan Beulich wrote: > On 06.11.2024 10:04, Federico Serafini wrote: >> The pseudo keyword fallthrough shall be used to make explicit the >> fallthrough intention at the end of a case statement (doing this >> through comments is deprecated). >> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >> --- >> xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ >> 1 file changed, 10 insertions(+) > > When you had asked my privately on Matrix, I specifically said: "Adding > the pseudo-keyword to x86-emulate.h (not x86_emulate.h) is probably best, > unless problems with that approach turn up." Even if identical re- > definitions are deemed fine, I for one consider such bad practice. Yet > by playing with this file (and outside of any relevant #ifdef) means > there will be such a re-definition when building Xen itself. Sorry Jan, I misinterpreted your message. I tested the definition in the x86-emulate.h and there are no problems. I will send a V2. > In fact the patch subject should also already clarify that the auxiliary > definition is only needed for the test and fuzzing harnesses. Ok. >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -23,6 +23,16 @@ >> # error Unknown compilation width >> #endif >> >> +/* >> + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at >> + * the end of a case statement. >> + */ >> +#if (!defined(__clang__) && (__GNUC__ >= 7)) > > I realize xen/compiler.h has it like that, but may I ask that you omit > the meaningless outer pair of parentheses? Ok.
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index d75658eba0..f49b1e0dd8 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -23,6 +23,16 @@ # error Unknown compilation width #endif +/* + * Pseudo keyword 'fallthrough' to make explicit the fallthrough intention at + * the end of a case statement. + */ +#if (!defined(__clang__) && (__GNUC__ >= 7)) +# define fallthrough __attribute__((__fallthrough__)) +#else +# define fallthrough do {} while (0) /* fallthrough */ +#endif + struct x86_emulate_ctxt; /*
The pseudo keyword fallthrough shall be used to make explicit the fallthrough intention at the end of a case statement (doing this through comments is deprecated). Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/arch/x86/x86_emulate/x86_emulate.h | 10 ++++++++++ 1 file changed, 10 insertions(+)