| Message ID | 1395766541-23979-14-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, 25 Mar 2014, Julien Grall wrote: > All the functions that send an SGI takes an enum. Therefore checking everytime > if the value is in the range is not correct. > > Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values. > > This is fix the compilation with Clang 3.5: > > gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare] > ASSERT(sgi < 16); /* There are only 16 SGIs */ > ~~~ ^ ~~ > xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT' > do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) > ^ > xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely' > #define unlikely(x) __builtin_expect((x),0) > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com> > Cc: Tim Deegan <tim@xen.org> > --- > xen/arch/arm/gic.c | 7 ++++--- > xen/include/asm-arm/gic.h | 2 ++ > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 0095b97..41142a5 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > unsigned int mask = 0; > cpumask_t online_mask; > > - ASSERT(sgi < 16); /* There are only 16 SGIs */ > + BUILD_BUG_ON(GIC_SGI_MAX >= 16); > + ASSERT(sgi != GIC_SGI_MAX); These new checks wouldn't be able to cover the following case, while the previous check could: enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16; send_SGI_mask(something, sgi);
Hi Stefano, On 25/03/14 18:19, Stefano Stabellini wrote: > On Tue, 25 Mar 2014, Julien Grall wrote: >> All the functions that send an SGI takes an enum. Therefore checking everytime >> if the value is in the range is not correct. >> >> Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values. >> >> This is fix the compilation with Clang 3.5: >> >> gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare] >> ASSERT(sgi < 16); /* There are only 16 SGIs */ >> ~~~ ^ ~~ >> xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT' >> do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) >> ^ >> xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely' >> #define unlikely(x) __builtin_expect((x),0) >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Cc: Ian Campbell <ian.campbell@citrix.com> >> Cc: Stefano Stabellini <stefano.stabellini@citrix.com> >> Cc: Tim Deegan <tim@xen.org> >> --- >> xen/arch/arm/gic.c | 7 ++++--- >> xen/include/asm-arm/gic.h | 2 ++ >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 0095b97..41142a5 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) >> unsigned int mask = 0; >> cpumask_t online_mask; >> >> - ASSERT(sgi < 16); /* There are only 16 SGIs */ >> + BUILD_BUG_ON(GIC_SGI_MAX >= 16); >> + ASSERT(sgi != GIC_SGI_MAX); > > These new checks wouldn't be able to cover the following case, while the > previous check could: > > enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16; > send_SGI_mask(something, sgi); Why people would do that? I think instead of an ASSERT, sgi & 0xff might better. It won't be harmless for the GIC, even debug is turned off. Right now, the developper can put the GIC in wrong state if the value is not in this range. If people wants to give a number instead of an enum, let them go to the hell! They should have used the right type. Regards,
On Tue, 2014-03-25 at 23:23 +0000, Julien Grall wrote: > Hi Stefano, > > On 25/03/14 18:19, Stefano Stabellini wrote: > > On Tue, 25 Mar 2014, Julien Grall wrote: > >> All the functions that send an SGI takes an enum. Therefore checking everytime > >> if the value is in the range is not correct. > >> > >> Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values. > >> > >> This is fix the compilation with Clang 3.5: > >> > >> gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare] > >> ASSERT(sgi < 16); /* There are only 16 SGIs */ > >> ~~~ ^ ~~ > >> xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT' > >> do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) > >> ^ > >> xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely' > >> #define unlikely(x) __builtin_expect((x),0) > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> Cc: Ian Campbell <ian.campbell@citrix.com> > >> Cc: Stefano Stabellini <stefano.stabellini@citrix.com> > >> Cc: Tim Deegan <tim@xen.org> > >> --- > >> xen/arch/arm/gic.c | 7 ++++--- > >> xen/include/asm-arm/gic.h | 2 ++ > >> 2 files changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >> index 0095b97..41142a5 100644 > >> --- a/xen/arch/arm/gic.c > >> +++ b/xen/arch/arm/gic.c > >> @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > >> unsigned int mask = 0; > >> cpumask_t online_mask; > >> > >> - ASSERT(sgi < 16); /* There are only 16 SGIs */ > >> + BUILD_BUG_ON(GIC_SGI_MAX >= 16); > >> + ASSERT(sgi != GIC_SGI_MAX); > > > > These new checks wouldn't be able to cover the following case, while the > > previous check could: > > > > enum gic_sgi sgi = (enum gic_sgi) integer_greater_than_16; > > send_SGI_mask(something, sgi); > > Why people would do that? enums and ints are often, for better or worse, interchangeable. That's why the existing assert is there, to catch people who are inadvertently doing something like this. (I don't think the cast in Stefano's example is strictly needed, so a real case is less likely to leap out into your face during review). > I think instead of an ASSERT, sgi & 0xff might better. It won't be > harmless for the GIC, even debug is turned off. Right now, the > developper can put the GIC in wrong state if the value is not in this range. The whole point of this assert is to help catch programmer mistakes. If the programmers and review process was perfect then the assert would be unnecessary. Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings? > If people wants to give a number instead of an enum, let them go to the > hell! They should have used the right type. As you have discovered in libxl it is sometimes necessary to take an enum through an integer for various tedious API/ABI reasons. Ian.
On 26/03/14 09:03, Ian Campbell wrote: > enums and ints are often, for better or worse, interchangeable. That's > why the existing assert is there, to catch people who are inadvertently > doing something like this. (I don't think the cast in Stefano's example > is strictly needed, so a real case is less likely to leap out into your > face during review). It's a shame that the compiler is not able to warn when an int is implicitly cast into an enum. >> I think instead of an ASSERT, sgi & 0xff might better. It won't be >> harmless for the GIC, even debug is turned off. Right now, the >> developper can put the GIC in wrong state if the value is not in this range. > > The whole point of this assert is to help catch programmer mistakes. If > the programmers and review process was perfect then the assert would be > unnecessary. It's valid for the compiler to do some optimization and think this ASSERT is not neccessary. So we don't catch programmer mistakes. If we want to keep the assert for this reason, we will have also to add sgi & 0xff to protect non-debug build and compiler which remove this assert. > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings? I forgot to try this solution. Surprisingly, the compiler is able to compile correctly this code. So I can replace the ASSERT(sgi < 16) with ASSERT(sgi < GIC_SGI_MAX) + BUILD_BUG_ON. Regards,
On Wed, 2014-03-26 at 09:41 +0000, Julien Grall wrote: > > On 26/03/14 09:03, Ian Campbell wrote: > > > enums and ints are often, for better or worse, interchangeable. That's > > why the existing assert is there, to catch people who are inadvertently > > doing something like this. (I don't think the cast in Stefano's example > > is strictly needed, so a real case is less likely to leap out into your > > face during review). > > It's a shame that the compiler is not able to warn when an int is > implicitly cast into an enum. > > >> I think instead of an ASSERT, sgi & 0xff might better. It won't be > >> harmless for the GIC, even debug is turned off. Right now, the > >> developper can put the GIC in wrong state if the value is not in this range. > > > > The whole point of this assert is to help catch programmer mistakes. If > > the programmers and review process was perfect then the assert would be > > unnecessary. > > It's valid for the compiler to do some optimization and think this > ASSERT is not neccessary. So we don't catch programmer mistakes. If the compiler is able to prove that it can optimise the ASSERT away then the programmer has not made a mistake, I think. > If we want to keep the assert for this reason, we will have also to add > sgi & 0xff to protect non-debug build and compiler which remove this assert. The purpose of the assert in a debug build is so that we can assume it is ok in a non-debug build. > > > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings? > > I forgot to try this solution. Surprisingly, the compiler is able to > compile correctly this code. So I can replace the ASSERT(sgi < 16) with > ASSERT(sgi < GIC_SGI_MAX) + BUILD_BUG_ON. Good, that sounds like the preferable approach then. Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 0095b97..41142a5 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -481,7 +481,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) unsigned int mask = 0; cpumask_t online_mask; - ASSERT(sgi < 16); /* There are only 16 SGIs */ + BUILD_BUG_ON(GIC_SGI_MAX >= 16); + ASSERT(sgi != GIC_SGI_MAX); cpumask_and(&online_mask, cpumask, &cpu_online_map); mask = gic_cpu_mask(&online_mask); @@ -501,7 +502,7 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi) void send_SGI_self(enum gic_sgi sgi) { - ASSERT(sgi < 16); /* There are only 16 SGIs */ + ASSERT(sgi != GIC_SGI_MAX); dsb(sy); @@ -511,7 +512,7 @@ void send_SGI_self(enum gic_sgi sgi) void send_SGI_allbutself(enum gic_sgi sgi) { - ASSERT(sgi < 16); /* There are only 16 SGIs */ + ASSERT(sgi != GIC_SGI_MAX); dsb(sy); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 071280b..968125d 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -205,6 +205,8 @@ enum gic_sgi { GIC_SGI_EVENT_CHECK = 0, GIC_SGI_DUMP_STATE = 1, GIC_SGI_CALL_FUNCTION = 2, + /* GIC_SGI_MAX must be the last type of the enum */ + GIC_SGI_MAX, }; extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi); extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);
All the functions that send an SGI takes an enum. Therefore checking everytime if the value is in the range is not correct. Introduce GIC_SGI_MAX to check the enum will never reach more than 16 values. This is fix the compilation with Clang 3.5: gic.c:515:15: error: comparison of constant 16 with expression of type 'enum gic_sgi' is always true [-Werror,-Wtautological-constant-out-of-range-compare] ASSERT(sgi < 16); /* There are only 16 SGIs */ ~~~ ^ ~~ xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT' do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) ^ xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely' #define unlikely(x) __builtin_expect((x),0) Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Stefano Stabellini <stefano.stabellini@citrix.com> Cc: Tim Deegan <tim@xen.org> --- xen/arch/arm/gic.c | 7 ++++--- xen/include/asm-arm/gic.h | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-)