Re: [PATCH] arm64: pass machine size to sparse

Re: [PATCH] arm64: pass machine size to sparse

[Luc Van Oostenryck]

On Tue, Jun 20, 2017 at 04:49:39PM +0100, Will Deacon wrote:
> On Tue, Jun 20, 2017 at 02:24:43PM +0200, Luc Van Oostenryck wrote:
> > When using sparse on the arm64 tree we get many thousands of
> > warnings like 'constant ... is so big it is unsigned long long'
> > or 'shift too big (32) for type unsigned long'. This happens
> > because by default sparse considers the machine as 32bit and
> > defines the size of the types accordingly.
> > 
> > Fix this by passing the '-m64' flag to sparse so that
> > sparse can correctly define longs as being 64bit.
> > 
> > CC: Catalin Marinas <catalin.marinas@arm.com>
> > CC: Will Deacon <will.deacon@arm.com>
> > CC: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  arch/arm64/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > index f839ecd91..15d6c1476 100644
> > --- a/arch/arm64/Makefile
> > +++ b/arch/arm64/Makefile
> > @@ -62,7 +62,7 @@ LD		+= -EL
> >  UTS_MACHINE	:= aarch64
> >  endif
> >  
> > -CHECKFLAGS	+= -D__aarch64__
> > +CHECKFLAGS	+= -D__aarch64__ -m64
> 
> Looks fine to me, but just wondering whether or not we should also be
> passing something to indicate the endianness of the target. Does sparse care
> about that?

Yes, we should. sparse doesn't care per-se but some macros, structures
or whole portion of code may depends on '__{LITTLE,BIG}_ENDIAN__'
being defined or not, for example (but I don't think it will make
a big difference, at least nothing like the 629904 'constant is so big'
warnings we have now due to the missing -m64).

For this, you have two possibilities:
1) just doing something like PPC:
	+ifdef CONFIG_CPU_BIG_ENDIAN
	+CHECKFLAGS     += -D__BIG_ENDIAN__
	+else
	+CHECKFLAGS     += -D__LITTLE_ENDIAN__
	+endif
   This will work now. I can't send you a proper patch if you wish.

2) a cleaner solution, IMO, would be to teach sparse about
   -mlittle-endian/-mbig-endian. In fact I already wrote the patch
   earlier today. But of course, you will need to wait for the patch
   to reach sparse's master and then compile sparse yourself or wait
   for a new release (which shouldn't take much long, though).

Regards,
-- Luc Van Oostenryck

Re: [PATCH] arm64: pass machine size to sparse

[Will Deacon]

On Tue, Jun 20, 2017 at 06:24:32PM +0200, Luc Van Oostenryck wrote:
> On Tue, Jun 20, 2017 at 04:49:39PM +0100, Will Deacon wrote:
> > On Tue, Jun 20, 2017 at 02:24:43PM +0200, Luc Van Oostenryck wrote:
> > > When using sparse on the arm64 tree we get many thousands of
> > > warnings like 'constant ... is so big it is unsigned long long'
> > > or 'shift too big (32) for type unsigned long'. This happens
> > > because by default sparse considers the machine as 32bit and
> > > defines the size of the types accordingly.
> > > 
> > > Fix this by passing the '-m64' flag to sparse so that
> > > sparse can correctly define longs as being 64bit.
> > > 
> > > CC: Catalin Marinas <catalin.marinas@arm.com>
> > > CC: Will Deacon <will.deacon@arm.com>
> > > CC: linux-arm-kernel@lists.infradead.org
> > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > > ---
> > >  arch/arm64/Makefile | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > > index f839ecd91..15d6c1476 100644
> > > --- a/arch/arm64/Makefile
> > > +++ b/arch/arm64/Makefile
> > > @@ -62,7 +62,7 @@ LD		+= -EL
> > >  UTS_MACHINE	:= aarch64
> > >  endif
> > >  
> > > -CHECKFLAGS	+= -D__aarch64__
> > > +CHECKFLAGS	+= -D__aarch64__ -m64
> > 
> > Looks fine to me, but just wondering whether or not we should also be
> > passing something to indicate the endianness of the target. Does sparse care
> > about that?
> 
> Yes, we should. sparse doesn't care per-se but some macros, structures
> or whole portion of code may depends on '__{LITTLE,BIG}_ENDIAN__'
> being defined or not, for example (but I don't think it will make
> a big difference, at least nothing like the 629904 'constant is so big'
> warnings we have now due to the missing -m64).
> 
> For this, you have two possibilities:
> 1) just doing something like PPC:
> 	+ifdef CONFIG_CPU_BIG_ENDIAN
> 	+CHECKFLAGS     += -D__BIG_ENDIAN__
> 	+else
> 	+CHECKFLAGS     += -D__LITTLE_ENDIAN__
> 	+endif
>    This will work now. I can't send you a proper patch if you wish.
> 
> 2) a cleaner solution, IMO, would be to teach sparse about
>    -mlittle-endian/-mbig-endian. In fact I already wrote the patch
>    earlier today. But of course, you will need to wait for the patch
>    to reach sparse's master and then compile sparse yourself or wait
>    for a new release (which shouldn't take much long, though).

If you do that, you could also teach it that AArch64 is 64-bit ;)

Will

Re: [PATCH] arm64: pass machine size to sparse

[Luc Van Oostenryck]

On Tue, Jun 20, 2017 at 06:04:01PM +0100, Will Deacon wrote:
> On Tue, Jun 20, 2017 at 06:24:32PM +0200, Luc Van Oostenryck wrote:
> > On Tue, Jun 20, 2017 at 04:49:39PM +0100, Will Deacon wrote:
> > > Looks fine to me, but just wondering whether or not we should also be
> > > passing something to indicate the endianness of the target. Does sparse care
> > > about that?
> > 
> > Yes, we should. sparse doesn't care per-se but some macros, structures
> > or whole portion of code may depends on '__{LITTLE,BIG}_ENDIAN__'
> > being defined or not, for example (but I don't think it will make
> > a big difference, at least nothing like the 629904 'constant is so big'
> > warnings we have now due to the missing -m64).
> > 
> > For this, you have two possibilities:
> > 1) just doing something like PPC:
> > 	+ifdef CONFIG_CPU_BIG_ENDIAN
> > 	+CHECKFLAGS     += -D__BIG_ENDIAN__
> > 	+else
> > 	+CHECKFLAGS     += -D__LITTLE_ENDIAN__
> > 	+endif
> >    This will work now. I can't send you a proper patch if you wish.
> > 
> > 2) a cleaner solution, IMO, would be to teach sparse about
> >    -mlittle-endian/-mbig-endian. In fact I already wrote the patch
> >    earlier today. But of course, you will need to wait for the patch
> >    to reach sparse's master and then compile sparse yourself or wait
> >    for a new release (which shouldn't take much long, though).
> 
> If you do that, you could also teach it that AArch64 is 64-bit ;)

Well, currently sparse is arch agnostic and I think it's a good thing.
For the few arch specifities you have to use some of the '-m' flags
(like -m32/-m64, -msize-long). So, for the moment the '-m64' flag
is needed, thus the patch.

-- Luc

Re: [PATCH] arm64: pass machine size to sparse

[Will Deacon]

On Tue, Jun 20, 2017 at 09:07:40PM +0200, Luc Van Oostenryck wrote:
> On Tue, Jun 20, 2017 at 06:04:01PM +0100, Will Deacon wrote:
> > On Tue, Jun 20, 2017 at 06:24:32PM +0200, Luc Van Oostenryck wrote:
> > > On Tue, Jun 20, 2017 at 04:49:39PM +0100, Will Deacon wrote:
> > > > Looks fine to me, but just wondering whether or not we should also be
> > > > passing something to indicate the endianness of the target. Does sparse care
> > > > about that?
> > > 
> > > Yes, we should. sparse doesn't care per-se but some macros, structures
> > > or whole portion of code may depends on '__{LITTLE,BIG}_ENDIAN__'
> > > being defined or not, for example (but I don't think it will make
> > > a big difference, at least nothing like the 629904 'constant is so big'
> > > warnings we have now due to the missing -m64).
> > > 
> > > For this, you have two possibilities:
> > > 1) just doing something like PPC:
> > > 	+ifdef CONFIG_CPU_BIG_ENDIAN
> > > 	+CHECKFLAGS     += -D__BIG_ENDIAN__
> > > 	+else
> > > 	+CHECKFLAGS     += -D__LITTLE_ENDIAN__
> > > 	+endif
> > >    This will work now. I can't send you a proper patch if you wish.
> > > 
> > > 2) a cleaner solution, IMO, would be to teach sparse about
> > >    -mlittle-endian/-mbig-endian. In fact I already wrote the patch
> > >    earlier today. But of course, you will need to wait for the patch
> > >    to reach sparse's master and then compile sparse yourself or wait
> > >    for a new release (which shouldn't take much long, though).
> > 
> > If you do that, you could also teach it that AArch64 is 64-bit ;)
> 
> Well, currently sparse is arch agnostic and I think it's a good thing.
> For the few arch specifities you have to use some of the '-m' flags
> (like -m32/-m64, -msize-long). So, for the moment the '-m64' flag
> is needed, thus the patch.

Ok, I'll pick your patch up then. The endianness case is more interesting,
because I don't think __BIG_ENDIAN__ and __LITTLE_ENDIAN__ are the defines
to use; the kernel seems to omit the trailing underscores afaict.

Will

Re: [PATCH] arm64: pass machine size to sparse

[Luc Van Oostenryck]

On Wed, Jun 21, 2017 at 05:12:54PM +0100, Will Deacon wrote:
> On Tue, Jun 20, 2017 at 09:07:40PM +0200, Luc Van Oostenryck wrote:
> > Well, currently sparse is arch agnostic and I think it's a good thing.
> > For the few arch specifities you have to use some of the '-m' flags
> > (like -m32/-m64, -msize-long). So, for the moment the '-m64' flag
> > is needed, thus the patch.
> 
> Ok, I'll pick your patch up then. The endianness case is more interesting,
> because I don't think __BIG_ENDIAN__ and __LITTLE_ENDIAN__ are the defines
> to use; the kernel seems to omit the trailing underscores afaict.

I just checked, it's a bit messy.
The kernel seems to rely, at least partly, maybe mostly, on
CONFIG_CPU_{BIG,LITTLE}_ENDIAN and then __{BIG,LITTLE}_ENDIAN is
defined via include/uapi/linux/byteorder/{big,little}_endian.h

Otherwise, from the compiler side it seems that __BYTE_ORDER__ &
__ORDER_{BIG,LITTLE}_ENDIAN__ is defined consistently.
But, for example for arm64, 'gcc -mlittle-endian' gives
	#define __LITTLE_ENDIAN__ 1
while 'gcc -mbig-endian' doesn't define __BIG_ENDIAN__ but
	#define __ARM_BIG_ENDIAN 1

*shrugh*

I've added support for the -m{big,little}-endian flag to sparse
but it seems it will be more complicated for the defines.

-- Luc