[RFH] gcc constant expression warning...

[RFH] gcc constant expression warning...

[Junio C Hamano]

With the recent gcc, we get:

sha1_file.c: In check_packed_git_:
sha1_file.c:527: warning: assuming signed overflow does not
occur when assuming that (X + c) < X is always false
sha1_file.c:527: warning: assuming signed overflow does not
occur when assuming that (X + c) < X is always false

when compiling with

    -O2 -Werror -Wall -Wold-style-definition \
    -ansi -pedantic -std=c99 -Wdeclaration-after-statement

The offending lines are:

        if (idx_size != min_size) {
                /* make sure we can deal with large pack offsets */
                off_t x = 0x7fffffffUL, y = 0xffffffffUL;
                if (x > (x + 1) || y > (y + 1)) {
                        munmap(idx_map, idx_size);

Re: [RFH] gcc constant expression warning...

[Florian Weimer]

* Junio C. Hamano:

> The offending lines are:
>
>         if (idx_size != min_size) {
>                 /* make sure we can deal with large pack offsets */
>                 off_t x = 0x7fffffffUL, y = 0xffffffffUL;
>                 if (x > (x + 1) || y > (y + 1)) {
>                         munmap(idx_map, idx_size);

x and y must be unsigned for this test to work (signed overflow is
undefined).

Re: [RFH] gcc constant expression warning...

[Daniel Barkalow]

On Sun, 28 Oct 2007, Florian Weimer wrote:

> * Junio C. Hamano:
> 
> > The offending lines are:
> >
> >         if (idx_size != min_size) {
> >                 /* make sure we can deal with large pack offsets */
> >                 off_t x = 0x7fffffffUL, y = 0xffffffffUL;
> >                 if (x > (x + 1) || y > (y + 1)) {
> >                         munmap(idx_map, idx_size);
> 
> x and y must be unsigned for this test to work (signed overflow is
> undefined).

I believe the test is trying to determine if signed addition on numbers of 
a certain size is safe in this environment. Doing the test with unsigned 
variables would cause the test to give a predictable but irrelevant 
result. I think gcc is being annoying in assuming that signed overflow 
doesn't occur (even when it must), rather than assuming that the result of 
signed overflow is some arbitrary and likely not useful value. If we have 
an overflow possible with off_t in the way we'd use it, then one of those 
tests should be automatically true due to the limited size of the type 
(except that I think the test should be >= instead of >). I think we 
should be able to assume that the result of a signed overflow, whatever 
undefined value it is, is a possible value of its type and therefore not 
more than the maximum value of its type, but gcc may be screwing this up.

It's probably best just to test the size of off_t.

	-Daniel
*This .sig left intentionally blank*

Re: [RFH] gcc constant expression warning...

[Antti-Juhani Kaijanaho]

Junio C Hamano <gitster@pobox.com> kirjoitti 28.10.2007:
> With the recent gcc, we get:
>
> sha1_file.c: In check_packed_git_:
> sha1_file.c:527: warning: assuming signed overflow does not
> occur when assuming that (X + c) < X is always false
> sha1_file.c:527: warning: assuming signed overflow does not
> occur when assuming that (X + c) < X is always false
>
> when compiling with
>
>     -O2 -Werror -Wall -Wold-style-definition \
>     -ansi -pedantic -std=c99 -Wdeclaration-after-statement

-ansi and -std=c99 in the same command line is a bit weird, BTW :)

> The offending lines are:
>
>         if (idx_size != min_size) {
>                 /* make sure we can deal with large pack offsets */
>                 off_t x = 0x7fffffffUL, y = 0xffffffffUL;
>                 if (x > (x + 1) || y > (y + 1)) {
>                         munmap(idx_map, idx_size);

The second if line invokes undefined behavior if off_t cannot represent
0x7fffffffUL + 1.  GCC apparently takes that as a license to ignore
overflow and rewrite that if as "if (0) { ...".

A fast fix is to compile with -fwrapv or with -fno-strict-overflow:

-fstrict-overflow
    Allow the compiler to assume strict signed overflow rules, depending
    on the language being compiled. For C (and C++) this means that
    overflow when doing arithmetic with signed numbers is undefined,
    which means that the compiler may assume that it will not happen.
    This permits various optimizations. For example, the compiler will
    assume that an expression like i + 10 > i will always be true for
    signed i. This assumption is only valid if signed overflow is
    undefined, as the expression is false if i + 10 overflows when using
    twos complement arithmetic. When this option is in effect any
    attempt to determine whether an operation on signed numbers will
    overflow must be written carefully to not actually involve overflow.

    See also the -fwrapv option. Using -fwrapv means that signed
    overflow is fully defined: it wraps. When -fwrapv is used, there is
    no difference between -fstrict-overflow and -fno-strict-overflow.
    With -fwrapv certain types of overflow are permitted. For example,
    if the compiler gets an overflow when doing arithmetic on constants,
    the overflowed value can still be used with -fwrapv, but not
    otherwise.

    The -fstrict-overflow option is enabled at levels -O2, -O3, -Os. 

-fwrapv
    This option instructs the compiler to assume that signed arithmetic
    overflow of addition, subtraction and multiplication wraps around
    using twos-complement representation. This flag enables some
    optimizations and disables others. This option is enabled by default
    for the Java front-end, as required by the Java language
    specification. 

(From the GCC 4.2.2 manual.)

A correct fix would be to check for the size of off_t in some other (and
defined) manner, but I don't know off_t well enough to suggest one.
Considering that the size of off_t won't change at runtime, the test
ought to be compile (or configure) time.  Reading POSIX, there seem to
be some rather cumbersome sysconf stuff one could test for, and of
course CHAR_BIT * sizeof(off_t) also tells one something.  GNU autoconf
might also have a solution at hand.

-- 
Antti-Juhani Kaijanaho, Jyväskylä, Finland

Re: [RFH] gcc constant expression warning...

[Linus Torvalds]

On Sun, 28 Oct 2007, Antti-Juhani Kaijanaho wrote:
>
> A correct fix would be to check for the size of off_t in some other (and
> defined) manner, but I don't know off_t well enough to suggest one.

In this case, it's trying to make sense that "off_t" can hold more than 32
bits. So I think that test can just be rewritten as

	if (sizeof(off_t) <= 4) {
		munmap(idx_map, idx_size);
		return error("pack too large for current definition of off_t in %s", path);
	}

instead.

			Linus

Re: [RFH] gcc constant expression warning...

[Nicolas Pitre]

On Sun, 28 Oct 2007, Linus Torvalds wrote:

> 
> 
> On Sun, 28 Oct 2007, Antti-Juhani Kaijanaho wrote:
> >
> > A correct fix would be to check for the size of off_t in some other (and
> > defined) manner, but I don't know off_t well enough to suggest one.
> 
> In this case, it's trying to make sense that "off_t" can hold more than 32
> bits. So I think that test can just be rewritten as
> 
> 	if (sizeof(off_t) <= 4) {
> 		munmap(idx_map, idx_size);
> 		return error("pack too large for current definition of off_t in %s", path);
> 	}
> 
> instead.

The test must also make sure off_t isn't signed, since in that case it 
can only hold 31 bits.


Nicolas

Re: [RFH] gcc constant expression warning...

[Stephen Rothwell]

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

On Sun, 28 Oct 2007 20:55:32 -0400 (EDT) Nicolas Pitre <nico@cam.org> wrote:
>
> The test must also make sure off_t isn't signed, since in that case it 
> can only hold 31 bits.

Posix says:
	"blkcnt_t and off_t shall be signed integer types."
 
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFH] gcc constant expression warning...

[Linus Torvalds]

On Sun, 28 Oct 2007, Nicolas Pitre wrote:
> 
> The test must also make sure off_t isn't signed, since in that case it 
> can only hold 31 bits.

Sinc eneither 31 _nor_ 32 bits is really enough, it's perfectly fine to 
just check that the size of "off_t" is *bigger* than 4 bytes, which my 
pseudo-patch did.

		Linus