Re: [PATCH v1] soc: rockchip: power-domain: Fix clang warning about negative shift count

[Matthias Kaehlcke]

El Sat, Mar 11, 2017 at 01:03:48PM +0100 Heiko Stuebner ha dit:

> Hi Matthias,
> 
> Am Freitag, 10. MÃrz 2017, 18:21:53 CET schrieb Matthias Kaehlcke:
> > The following warning is generated when building with clang:
> > 
> > drivers/soc/rockchip/pm_domains.c:726:22: error: shift count is negative
> > [-Werror,-Wshift-count-negative] [RK3399_PD_TCPD0]       = DOMAIN_RK3399(8,
> > 8, -1, false),
> >                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/soc/rockchip/pm_domains.c:101:2: note: expanded from macro
> > 'DOMAIN_RK3399' DOMAIN(pwr, status, req, req, req, wakeup)
> >         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/soc/rockchip/pm_domains.c:88:27: note: expanded from macro 'DOMAIN'
> >         .req_mask = (req >= 0) ? BIT(req) : 0,          \
> >                                  ^~~~~~~~
> > include/linux/bitops.h:6:24: note: expanded from macro 'BIT'
> > 
> > The BIT macro is evaluated with the negative value -1, even though the
> > resulting value would not be assigned. To fix this we only pass values
> > between 0 and 63 to BIT(). Unfortunately this means that we lose the
> > benefit of the compiler checking for out of bounds errors.
> 
> I tend to disagree here. This looks more like a case of "fix your compiler".
> 
> That conditional seems perfectly valid as the BIT(req) will never be reached 
> if req < 0 - your clang simply doesn't recognize the pattern somehow, while 
> for example gcc does.

My interpretation is that with clang the '(req >= 0) ?' condition is
not evaluated by the preprocessor, but only by the compiler. This seems to
be different with gcc.

> Catering to specific whims of specific compilers feels somehow wrong, as what 
> will happen if some imaginary third compiler requires another different hack 
> to be satisfied?

I'll check with the clang developers if clang can be changed to behave
like gcc in this aspect.

Thanks

Matthias

> > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > ---
> >  drivers/soc/rockchip/pm_domains.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/soc/rockchip/pm_domains.c
> > b/drivers/soc/rockchip/pm_domains.c index 1c78c42416c6..6f2bb1222992 100644
> > --- a/drivers/soc/rockchip/pm_domains.c
> > +++ b/drivers/soc/rockchip/pm_domains.c
> > @@ -77,13 +77,15 @@ struct rockchip_pmu {
> > 
> >  #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain,
> > genpd)
> > 
> > +#define RK_MASK(bit) ((bit >= 0) ? BIT(bit & 0x3f) : 0)
> > +
> >  #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
> > -{						\
> > -	.pwr_mask = (pwr >= 0) ? BIT(pwr) : 0,		\
> > -	.status_mask = (status >= 0) ? BIT(status) : 0,	\
> > -	.req_mask = (req >= 0) ? BIT(req) : 0,		\
> > -	.idle_mask = (idle >= 0) ? BIT(idle) : 0,	\
> > -	.ack_mask = (ack >= 0) ? BIT(ack) : 0,		\
> > +{							\
> > +	.pwr_mask = RK_MASK(pwr),			\
> > +	.status_mask = RK_MASK(status),			\
> > +	.req_mask = RK_MASK(req),			\
> > +	.idle_mask = RK_MASK(idle),			\
> > +	.ack_mask = RK_MASK(ack),			\
> >  	.active_wakeup = wakeup,			\
> >  }
> 
>