| Message ID | 7edf60c0e7bd0680d8b8f8d3aec1264ee5a43878.1696514677.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 10.1 | expand |
On Fri, 6 Oct 2023, Nicola Vetrini wrote: > The essential type of the result of an inequality operator is > essentially boolean, therefore it shouldn't be used as an argument of > the multiplication operator, which expects an integer. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/include/xen/compat.h | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h > index f2ce5bb3580a..5ffee6a9fed1 100644 > --- a/xen/include/xen/compat.h > +++ b/xen/include/xen/compat.h > @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \ > return x == c; \ > } > > +#define SIZE_NEQUAL(a, b) \ > + (sizeof(a) != sizeof(b) ? 1 : 0) > #define CHECK_SIZE(name) \ > - typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \ > - sizeof(compat_ ## name ## _t)) * 2] > + typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## _t, \ > + compat_ ## name ## _t)) * 2] > #define CHECK_SIZE_(k, n) \ > - typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \ > - sizeof(k compat_ ## n)) * 2] > + typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \ > + k compat_ ## n)) * 2] I think this style is easier to read but I'll let the x86 maintainers decide typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \ sizeof(compat_ ## name ## _t)) ? 1 : -1] Also am I reading this correctly that we are using -1 as array index? I must have made a calculation mistake?
On 10/10/2023 9:02 am, Stefano Stabellini wrote: > On Fri, 6 Oct 2023, Nicola Vetrini wrote: >> The essential type of the result of an inequality operator is >> essentially boolean, therefore it shouldn't be used as an argument of >> the multiplication operator, which expects an integer. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/include/xen/compat.h | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h >> index f2ce5bb3580a..5ffee6a9fed1 100644 >> --- a/xen/include/xen/compat.h >> +++ b/xen/include/xen/compat.h >> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \ >> return x == c; \ >> } >> >> +#define SIZE_NEQUAL(a, b) \ >> + (sizeof(a) != sizeof(b) ? 1 : 0) >> #define CHECK_SIZE(name) \ >> - typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \ >> - sizeof(compat_ ## name ## _t)) * 2] >> + typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## _t, \ >> + compat_ ## name ## _t)) * 2] >> #define CHECK_SIZE_(k, n) \ >> - typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \ >> - sizeof(k compat_ ## n)) * 2] >> + typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \ >> + k compat_ ## n)) * 2] > I think this style is easier to read but I'll let the x86 maintainers > decide > > typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \ > sizeof(compat_ ## name ## _t)) ? 1 : -1] > > Also am I reading this correctly that we are using -1 as array index? I > must have made a calculation mistake? This is a NIH BUILD_BUG_ON(). It should be rewritten as BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...)); which will use _Static_assert() in modern compilers. ~Andrew
On 10/10/2023 18:00, Andrew Cooper wrote: > On 10/10/2023 9:02 am, Stefano Stabellini wrote: >> On Fri, 6 Oct 2023, Nicola Vetrini wrote: >>> The essential type of the result of an inequality operator is >>> essentially boolean, therefore it shouldn't be used as an argument of >>> the multiplication operator, which expects an integer. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> xen/include/xen/compat.h | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h >>> index f2ce5bb3580a..5ffee6a9fed1 100644 >>> --- a/xen/include/xen/compat.h >>> +++ b/xen/include/xen/compat.h >>> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \ >>> return x == c; \ >>> } >>> >>> +#define SIZE_NEQUAL(a, b) \ >>> + (sizeof(a) != sizeof(b) ? 1 : 0) >>> #define CHECK_SIZE(name) \ >>> - typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) >>> != \ >>> - sizeof(compat_ ## name ## >>> _t)) * 2] >>> + typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## >>> _t, \ >>> + compat_ ## name >>> ## _t)) * 2] >>> #define CHECK_SIZE_(k, n) \ >>> - typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \ >>> - sizeof(k compat_ ## n)) * >>> 2] >>> + typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \ >>> + k compat_ ## >>> n)) * 2] >> I think this style is easier to read but I'll let the x86 maintainers >> decide >> >> typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \ >> sizeof(compat_ ## name ## _t)) ? >> 1 : -1] >> >> Also am I reading this correctly that we are using -1 as array index? >> I >> must have made a calculation mistake? > > This is a NIH BUILD_BUG_ON(). It should be rewritten as > > BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...)); > > which will use _Static_assert() in modern compilers. > > ~Andrew Ok, thanks.
On 11/10/2023 12:06 am, Nicola Vetrini wrote: > On 10/10/2023 18:00, Andrew Cooper wrote: >> On 10/10/2023 9:02 am, Stefano Stabellini wrote: >>> On Fri, 6 Oct 2023, Nicola Vetrini wrote: >>>> The essential type of the result of an inequality operator is >>>> essentially boolean, therefore it shouldn't be used as an argument of >>>> the multiplication operator, which expects an integer. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> xen/include/xen/compat.h | 10 ++++++---- >>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h >>>> index f2ce5bb3580a..5ffee6a9fed1 100644 >>>> --- a/xen/include/xen/compat.h >>>> +++ b/xen/include/xen/compat.h >>>> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \ >>>> return x == c; \ >>>> } >>>> >>>> +#define SIZE_NEQUAL(a, b) \ >>>> + (sizeof(a) != sizeof(b) ? 1 : 0) >>>> #define CHECK_SIZE(name) \ >>>> - typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## >>>> _t) != \ >>>> - sizeof(compat_ ## name ## >>>> _t)) * 2] >>>> + typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name >>>> ## _t, \ >>>> + compat_ ## >>>> name ## _t)) * 2] >>>> #define CHECK_SIZE_(k, n) \ >>>> - typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \ >>>> - sizeof(k compat_ ## n)) >>>> * 2] >>>> + typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \ >>>> + k compat_ ## >>>> n)) * 2] >>> I think this style is easier to read but I'll let the x86 maintainers >>> decide >>> >>> typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \ >>> sizeof(compat_ ## name ## _t)) >>> ? 1 : -1] >>> >>> Also am I reading this correctly that we are using -1 as array index? I >>> must have made a calculation mistake? >> >> This is a NIH BUILD_BUG_ON(). It should be rewritten as >> >> BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...)); >> >> which will use _Static_assert() in modern compilers. >> >> ~Andrew > > Ok, thanks. > I'm going to go out on a limb and say that every other pattern that looks like this probably wants converting to a BUILD_BUG_ON(). This code quite possibly predates the introduction of BUILD_BUG_ON(), but we want to end up using BUILD_BUG_ON() everywhere because it's the construct that uses _Static_assert() wherever possible. ~Andrew
diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h index f2ce5bb3580a..5ffee6a9fed1 100644 --- a/xen/include/xen/compat.h +++ b/xen/include/xen/compat.h @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \ return x == c; \ } +#define SIZE_NEQUAL(a, b) \ + (sizeof(a) != sizeof(b) ? 1 : 0) #define CHECK_SIZE(name) \ - typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \ - sizeof(compat_ ## name ## _t)) * 2] + typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## _t, \ + compat_ ## name ## _t)) * 2] #define CHECK_SIZE_(k, n) \ - typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \ - sizeof(k compat_ ## n)) * 2] + typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \ + k compat_ ## n)) * 2] #define CHECK_FIELD_COMMON(name, t, f) \ static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
The essential type of the result of an inequality operator is essentially boolean, therefore it shouldn't be used as an argument of the multiplication operator, which expects an integer. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/include/xen/compat.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)