| Message ID | 1447141453-19876-7-git-send-email-stewart@linux.vnet.ibm.com |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, 2015-11-10 at 18:44 +1100, Stewart Smith wrote: > hw/fsp/fsp.c:205:13: warning: The result of the '<<' expression is > undefined > return 1ul << (class - FSP_MCLASS_FIRST); > ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ That's not quite right... > Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com> > --- > hw/fsp/fsp.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/fsp/fsp.c b/hw/fsp/fsp.c > index ab625a55d16d..583476841ad1 100644 > --- a/hw/fsp/fsp.c > +++ b/hw/fsp/fsp.c > @@ -198,11 +198,15 @@ static struct fsp *fsp_get_active(void) > > static u64 fsp_get_class_bit(u8 class) > { > + u8 shift; > + > /* Alias classes CE and CF as the FSP has a single queue */ > if (class == FSP_MCLASS_IPL) > class = FSP_MCLASS_SERVICE; > > - return 1ul << (class - FSP_MCLASS_FIRST); > + shift = class - FSP_MCLASS_FIRST; > + assert(shift < 64); > + return 1ul << shift; > } And that's gross. Don't do that. If you want to bound check class, do so with something like assert(class >= FSP_MCLASS_FIRST && class < ...) But that added shift variable is just hurting my eyes. > static struct fsp_cmdclass *__fsp_get_cmdclass(u8 class)
Benjamin Herrenschmidt <benh@au1.ibm.com> writes: >> --- a/hw/fsp/fsp.c >> +++ b/hw/fsp/fsp.c >> @@ -198,11 +198,15 @@ static struct fsp *fsp_get_active(void) >> >> static u64 fsp_get_class_bit(u8 class) >> { >> + u8 shift; >> + >> /* Alias classes CE and CF as the FSP has a single queue */ >> if (class == FSP_MCLASS_IPL) >> class = FSP_MCLASS_SERVICE; >> >> - return 1ul << (class - FSP_MCLASS_FIRST); >> + shift = class - FSP_MCLASS_FIRST; >> + assert(shift < 64); >> + return 1ul << shift; >> } > > And that's gross. Don't do that. > > If you want to bound check class, do so with something like > > assert(class >= FSP_MCLASS_FIRST && class < ...) > > But that added shift variable is just hurting my eyes. It's kind of incredibly awful, and this seems to be a hoop to jump through to shut LLVM up. This doesn't squash the warning: assert((class - FSP_MCLASS_FIRST) < 64); Which would be semantically the best.. but what's worse is looking at it again, class is u8, and FSP_MCLASS_FIRST is defined to 0xce - which means that expression is only ever going to be <=49 which is, amazingly enough, well within the allowed number of shifs for u64. I scream LLVM bug instead and will just drop this patch.
diff --git a/hw/fsp/fsp.c b/hw/fsp/fsp.c index ab625a55d16d..583476841ad1 100644 --- a/hw/fsp/fsp.c +++ b/hw/fsp/fsp.c @@ -198,11 +198,15 @@ static struct fsp *fsp_get_active(void) static u64 fsp_get_class_bit(u8 class) { + u8 shift; + /* Alias classes CE and CF as the FSP has a single queue */ if (class == FSP_MCLASS_IPL) class = FSP_MCLASS_SERVICE; - return 1ul << (class - FSP_MCLASS_FIRST); + shift = class - FSP_MCLASS_FIRST; + assert(shift < 64); + return 1ul << shift; } static struct fsp_cmdclass *__fsp_get_cmdclass(u8 class)
hw/fsp/fsp.c:205:13: warning: The result of the '<<' expression is undefined return 1ul << (class - FSP_MCLASS_FIRST); ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com> --- hw/fsp/fsp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)