Ian Lance Taylor - Re: PATCH RFC: -Wstrict-overflow (original) (raw)

This is the mail archive of the gcc-patches@gcc.gnu.orgmailing list for the GCC project.

• From: Ian Lance Taylor
• To: Mark Mitchell
• Cc: gcc-patches at gcc dot gnu dot org
• Date: 01 Feb 2007 08:03:18 -0800
• Subject: Re: PATCH RFC: -Wstrict-overflow
• References: <m34pq6vaxj.fsf@localhost.localdomain> 45C17C4F.8030502@codesourcery.com

Mark Mitchell mark@codesourcery.com writes:

../../trunk/gcc/bb-reorder.c: In function âfind_tracesâ: ../../trunk/gcc/bb-reorder.c:266: warning: assuming signed overflow is undefined when simplifying division

I think these warnings should be fixed before the patch goes in. In other words, I think we should make sure that no new warnings are introduced during the GCC build process by the patch: either by modifying GCC to not trigger the warnings or by modifying the patch to not warn about stuff.

I've considered that. It's a tradeoff: we can eliminate the warning in all cases, but it tends to make the resulting code a little uglier. As you say later, it's a matter of: we know the values which will appear here are such that the calculations will never overflow.

You looked at the bb-reorder.c case, which would be easy enough to fix by changing the values stored in the branch_threshold array.

Here are some other examples:

calls.c:885: warning: assuming signed overflow is undefined when negating a division

The code is: bytes -= bitsize / BITS_PER_UNIT; The optimization is effectively changing this to bytes += bitsize / - BITS_PER_UNIT; I think this is a little uglier.

A somewhat similar case:

cfghooks.c:858: warning: assuming signed overflow is undefined when negating a division

The code is: dummy->frequency -= EDGE_FREQUENCY (e); where EDGE_FREQUENCY is

#define EDGE_FREQUENCY(e) (((e)->src->frequency
* (e)->probability
+ REG_BR_PROB_BASE / 2)
/ REG_BR_PROB_BASE)

Here the compiler is similarly pushing the negation of the division into a negation of the constant REG_BR_PROB_BASE. I don't see any clean way to fix this.

Another example:

real.c:979: warning: assuming signed overflow is undefined when changing X +- C1 cmp C2 to X cmp C1 +- C2

The code is: if (REAL_EXP (r) <= 0) where REAL_EXP is

#define REAL_EXP(REAL)
((int)((REAL)->uexp ^ (unsigned int)(1 << (EXP_BITS - 1))) \

• (1 << (EXP_BITS - 1)))

Here the compiler is moving the constant (1 << (EXP_BITS - 1)) from one side of the comparison to the other. Again, I don't see any clean way to fix this.

In other words: the goal is laudable, and I worked on it for a while, but it's hard to fix these cases cleanly. People don't often directly write the kind of code which can be simplified using undefined signed overflow; they tend to simplify it themselves. The cases arise when using macros, where the compiler sees the the macro is hiding an expression which can be simplified. Adjusting the code to avoid the warnings generally means unpacking the macro. And that makes the code less readable and harder to maintain--the macro is there for a reason. However, we do generally want to issue the warning: the compiler really is relying on undefined signed overflow, and something unexpected will indeed happen if the values are close to INT_MAX or INT_MIN.

That's why I unfortunately don't think it's realistic to fix all the warnings. Other opinions welcome.

I think a clearer way to word the warning might be something like:

warning: assuming overflow does not occur when simplifying division note: signed overflow is undefined, use unsigned arithmetic if you note: want two's complement overflow

with the note issued only once.

Good idea, thanks.

Ian

• Follow-Ups:
  • Re: PATCH RFC: -Wstrict-overflow
    * From: Mark Mitchell
  • Re: PATCH RFC: -Wstrict-overflow
    * From: Alexandre Oliva
• References:
  • PATCH RFC: -Wstrict-overflow
    * From: Ian Lance Taylor