confusing shift warning

confusing shift warning

[Marko Kreen]

Platform: x64_64 linux, uint64_t -> unsigned long
Sparse: today's git (6dcc36a)

Code:
--------------------------------------------
#include <stdint.h>

static void fn1(uint32_t val) { }

static void fn2(uint64_t val)
{
        fn1(val >> 32); // warning
}

static void fn3(unsigned int val) { }

static void fn4(unsigned long long val)
{
        fn3(val >> 32); // no warning
}
------------------------------

$ cgcc -c test.c
test.c:7:10: warning: shift too big (32) for type unsigned long

-------------------------------

The confusion raises from the fact that "unsigned long" _does_
support 32-bit shift, and anyway I'm using "uint64_t", which
I expect sparse to understand.

Guess - somewhere is hardwired that "long" == "32-bit"?

For reference:
$ cgcc -v -c test.c
sparse -v -c test.c -Dx86_64=1 -D__x86_64=1 -D__x86_64__=1
-D__CHAR_BIT__=8 -D__SCHAR_MAX__=127 -D__SHRT_MAX__=32767
-D__INT_MAX__=2147483647 -D__LONG_MAX__=9223372036854775807L
-D__LONG_LONG_MAX__=9223372036854775807LL -D__FLT_RADIX__=2
-D__FINITE_MATH_ONLY__=0 -D__DECIMAL_DIG__=33 -D__FLT_MANT_DIG__=24
-D__FLT_DIG__=6 -D__FLT_MIN_EXP__='(-125)' -D__FLT_MAX_EXP__=128
-D__FLT_MIN_10_EXP__='(-37)' -D__FLT_MAX_10_EXP__=38
-D__FLT_HAS_INFINITY__=1 -D__FLT_HAS_QUIET_NAN__=1
-D__FLT_DENORM_MIN__=1.40129846e-45F -D__FLT_EPSILON__=1.19209290e-7F
-D__FLT_MAX__=3.40282347e+38F -D__FLT_MIN__=1.17549435e-38F
-D__DBL_MANT_DIG__=53 -D__DBL_DIG__=15 -D__DBL_MIN_EXP__='(-1021)'
-D__DBL_MAX_EXP__=1024 -D__DBL_MIN_10_EXP__='(-307)'
-D__DBL_MAX_10_EXP__=308 -D__DBL_HAS_INFINITY__=1
-D__DBL_HAS_QUIET_NAN__=1 -D__DBL_DENORM_MIN__=4.9406564584124654e-324
-D__DBL_EPSILON__=2.2204460492503131e-16
-D__DBL_MAX__=1.7976931348623157e+308
-D__DBL_MIN__=2.2250738585072014e-308 -D__LDBL_MANT_DIG__=113
-D__LDBL_DIG__=33 -D__LDBL_MIN_EXP__='(-16381)'
-D__LDBL_MAX_EXP__=16384 -D__LDBL_MIN_10_EXP__='(-4931)'
-D__LDBL_MAX_10_EXP__=4932 -D__LDBL_HAS_INFINITY__=1
-D__LDBL_HAS_QUIET_NAN__=1
-D__LDBL_DENORM_MIN__=6.47517511943802511092443895822764655e-4966L
-D__LDBL_EPSILON__=1.92592994438723585305597794258492732e-34L
-D__LDBL_MAX__=1.18973149535723176508575932662800702e+4932L
-D__LDBL_MIN__=3.36210314311209350626267781732175260e-4932L
-U__SIZE_TYPE__ -D__SIZE_TYPE__=long\ unsigned\ int -Dunix=1
-D__unix=1 -D__unix__=1 -D__linux__=1 -D__linux=1 -Dlinux=linux
test.c:7:10: warning: shift too big (32) for type unsigned long


-- 
marko

Re: confusing shift warning

[Marko Kreen]

On 4/25/08, Marko Kreen <markokr@gmail.com> wrote:
> Platform: x64_64 linux, uint64_t -> unsigned long
>  Sparse: today's git (6dcc36a)

Oh, and another warning:

#include <sys/socket.h> gives:

 /usr/include/bits/socket.h:159:5: warning: constant
9223372036854775807L is so big it is long long

I used to ignore it as it was not in my code, but its probably same thing.

Ubuntu 8.04
gcc 4:4.2.3-1ubuntu3
glibc 2.7-10ubuntu3
linux-headers - 2.6.24-16.30

-- 
marko

Re: confusing shift warning

[Marko Kreen]

On 4/25/08, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Marko Kreen <markokr@gmail.com>:
>
>
> > Platform: x64_64 linux, uint64_t -> unsigned long
> > Sparse: today's git (6dcc36a)
> >
>  ...
>
> > Guess - somewhere is hardwired that "long" == "32-bit"?
> >
>
>  Of course not.  But -m64 should be specified to enable 64-bit type sizes.
>
>
> > For reference:
> > $ cgcc -v -c test.c
> > sparse -v -c test.c -Dx86_64=1 -D__x86_64=1 -D__x86_64__=1
> >
>  ...
>
>  cgcc fails to add -m64.  It must be a bug in cgcc.  This would not produce
> the warning:
>
>  cgcc -m64 -no-compile Test.c

Ok, that somewhat explains it.  But:

    } elsif ($spec eq 'x86_64') {
        return (' -Dx86_64=1 -D__x86_64=1 -D__x86_64__=1' .
                &integer_types (8, 16, 32, $m32 ? 32 : 64, 64) .
                &float_types (1, 1, 33, [24,8], [53,11], [113,15]) .
                &define_size_t ($m32 ? "unsigned int" : "long unsigned int"));

It seems that cgcc defaults to 64-bit on x86_64, but sparse to 32-bit.
Isn't it sparse bug then?  Shouldnt it follow platform defaults?

-- 
marko

Re: confusing shift warning

[Marko Kreen]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

On 4/25/08, Marko Kreen <markokr@gmail.com> wrote:
>  It seems that cgcc defaults to 64-bit on x86_64, but sparse to 32-bit.
>  Isn't it sparse bug then?  Shouldnt it follow platform defaults?

It can be fixed from cgcc side, but I still think its sparse bug,
the different defaults can still cause confusion in future.

Patch attached.

Sorry about attachement, but I don't trust gmail with copy-paste...

-- 
marko

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-cgcc-always-pass-m32-m64-to-sparse-on-x86_64-def.patch --]
[-- Type: text/x-diff; name=0001-cgcc-always-pass-m32-m64-to-sparse-on-x86_64-def.patch, Size: 947 bytes --]

From: Marko Kreen <markokr@gmail.com>
Date: Fri, 25 Apr 2008 11:52:47 +0300
Subject: [PATCH] cgcc: always pass -m32/-m64 to sparse on x86_64, defaulting to -m64.

Sparse internally defaults to -m32.  By explicitly giving -mXX,
cgcc now follows platform standard on x86_64 and it future-proof
against fixing sparse internal default.

Signed-off-by: Marko Kreen <markokr@gmail.com>
---
 cgcc |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/cgcc b/cgcc
index 4fab530..d80ea07 100755
--- a/cgcc
+++ b/cgcc
@@ -238,6 +238,7 @@ sub add_specs {
 		&define_size_t ($m64 ? "long unsigned int" : "unsigned int"));
     } elsif ($spec eq 'x86_64') {
 	return (' -Dx86_64=1 -D__x86_64=1 -D__x86_64__=1' .
+		($m32 ? " -m32" : " -m64") .
 		&integer_types (8, 16, 32, $m32 ? 32 : 64, 64) .
 		&float_types (1, 1, 33, [24,8], [53,11], [113,15]) .
 		&define_size_t ($m32 ? "unsigned int" : "long unsigned int"));
-- 
1.5.5.1.57.g5909c

Re: confusing shift warning

[Marko Kreen]

On 4/25/08, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Marko Kreen <markokr@gmail.com>:
> > Patch attached.
>
>  NACK.  $m64 is only set if "-m64" was explicitly passed to cgcc.
>
>  It looks like cgcc code expect sparse to handle it, and sparse expects it
> from cgcc.

Do you mean the variables are not set?  But they are:

  my $m32 = 0;
  my $m64 = 0;

Or do you mean I cannot assume one of them as set, unless
overrided on command line?  But other sections (spact, i86, ppc)
do exactly that?

Or do you mean I cannot give extra -mXX arguement unless given by user?
But as you said, sparse (by policy) wont follow platform default,
but I want cgcc to follow it.  How do I achieve that then?

-- 
marko

Re: confusing shift warning

[Marko Kreen]

On 4/25/08, Pavel Roskin <proski@gnu.org> wrote:
> Quoting Marko Kreen <markokr@gmail.com>:
> > On 4/25/08, Pavel Roskin <proski@gnu.org> wrote:
> > > Quoting Marko Kreen <markokr@gmail.com>:
> > > > Patch attached.
> > >
> > >  NACK.  $m64 is only set if "-m64" was explicitly passed to cgcc.
> > >
> > >  It looks like cgcc code expect sparse to handle it, and sparse expects
> it
> > > from cgcc.
> >
> > Do you mean the variables are not set?  But they are:
> >
> >  my $m32 = 0;
> >  my $m64 = 0;
>
>  I mean, they are both 0 unless -m32 or -m64 is specified.

No, see other sections - they assume one of them is set unless overrided.

> > Or do you mean I cannot assume one of them as set, unless
> > overrided on command line?  But other sections (spact, i86, ppc)
> > do exactly that?
>
>  You cannot assume either of them to be 1 until cgcc is fixed.

But how should the fix look like if you don't like mine?

> > Or do you mean I cannot give extra -mXX arguement unless given by user?
>
>  No, I don't mean it.  It should be fine to add the -mXX argument.

-- 
marko

Re: confusing shift warning

[Pavel Roskin]

On Fri, 2008-04-25 at 17:37 +0300, Marko Kreen wrote:

> >  I mean, they are both 0 unless -m32 or -m64 is specified.
> 
> No, see other sections - they assume one of them is set unless overrided.

If you are fixing a bug, you cannot rely on the code being correct.  Try
printing those variables:

printf("m32=%d, m64=%d\n", $m32, $m64);

It will be two zeroes.

> > > Or do you mean I cannot assume one of them as set, unless
> > > overrided on command line?  But other sections (spact, i86, ppc)
> > > do exactly that?
> >
> >  You cannot assume either of them to be 1 until cgcc is fixed.
> 
> But how should the fix look like if you don't like mine?

Well, it looks like cgcc is seriously broken.  In particular, it would
define x86_64 even if -m32 is specified, but it should define i386
instead.

Also, cgcc looks at uname output to determine the target CPU.  That
would default to 64 bit if a 32-bit system runs on a 64-bit kernel.  I
think cgcc should be running gcc instead to dump the machine settings.
I realize that it might be slower, but correctness is important here.
On the other hand, I'm not sure if we can rely on having gcc installed.

I would probably introduce a variable that would hold the memory model.
It could be ILP32 or LP64, but we could eventually support LLP64 (win64)
and even LP32 (win16).  It could be determined based on -m32/-m64
switches and uname output, but be could switch to "gcc -dumpmachine"
later.

The architecture would be adjusted based on the selected memory model.
And then it would be passed to add_specs().

-- 
Regards,
Pavel Roskin

Re: confusing shift warning

[Marko Kreen]

On 4/25/08, Pavel Roskin <proski@gnu.org> wrote:
> On Fri, 2008-04-25 at 17:37 +0300, Marko Kreen wrote:
>  > >  I mean, they are both 0 unless -m32 or -m64 is specified.
>  >
>  > No, see other sections - they assume one of them is set unless overrided.
>
> If you are fixing a bug, you cannot rely on the code being correct.  Try
>  printing those variables:
>
>  printf("m32=%d, m64=%d\n", $m32, $m64);
>
>  It will be two zeroes.

I think you have misunderstood the point of the variables - they
only signify whether either argument was given on command line,
nothing more.  Each arch defaults implicitly to either of them,
so it needs to check if the other one was given, overriding the default.

>  > > > Or do you mean I cannot assume one of them as set, unless
>  > > > overrided on command line?  But other sections (spact, i86, ppc)
>  > > > do exactly that?
>  > >
>  > >  You cannot assume either of them to be 1 until cgcc is fixed.
>  >
>  > But how should the fix look like if you don't like mine?
>
> Well, it looks like cgcc is seriously broken.  In particular, it would
>  define x86_64 even if -m32 is specified, but it should define i386
>  instead.

You may be right here.  Seems like the x86_64 arch was tested only
with kernel compiles with -m64 always given.

>  Also, cgcc looks at uname output to determine the target CPU.  That
>  would default to 64 bit if a 32-bit system runs on a 64-bit kernel.  I
>  think cgcc should be running gcc instead to dump the machine settings.
>  I realize that it might be slower, but correctness is important here.
>  On the other hand, I'm not sure if we can rely on having gcc installed.
>
>  I would probably introduce a variable that would hold the memory model.
>  It could be ILP32 or LP64, but we could eventually support LLP64 (win64)
>  and even LP32 (win16).  It could be determined based on -m32/-m64
>  switches and uname output, but be could switch to "gcc -dumpmachine"
>  later.
>
>  The architecture would be adjusted based on the selected memory model.
>  And then it would be passed to add_specs().

Ok, this is somewhat of out my league.  But I think that having good
default and forcing user to use -mX for non-default case is good enough.

-- 
marko