[RFH] gcc constant expression warning...

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);

* Junio C. Hamano:

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

On Sun, 28 Oct 2007, Florian Weimer wrote:

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*

Junio C Hamano <gitster@pobox.com> kirjoitti 28.10.2007:

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

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.

On Sun, 28 Oct 2007, Antti-Juhani Kaijanaho wrote:

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

On Sun, 28 Oct 2007, Linus Torvalds wrote:

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

Nicolas

On Sun, 28 Oct 2007 20:55:32 -0400 (EDT) Nicolas Pitre <nico@cam.org> wrote:

Posix says:
	"blkcnt_t and off_t shall be signed integer types."
 

On Sun, 28 Oct 2007, Nicolas Pitre wrote:

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