Signed overflow not always wrapping correctly

[cousteaulecommandant]

Summary: Please consider adding the -fwrapv flag to the list of flags passed to Arduino's compiler. It fixes potential undefined behavior an Arduino user should not deal with.

---

• According to the Arduino documentation, when an int exceeds its limit value it "rolls over" (wraps) to the other side, modulo 2^16 (or 2^32).
• According to the C++ standard, when a signed integer such as int exceeds its limit value it triggers "undefined behavior" and the implementation is free to go crazy when it happens (doing this with unsigned integers is fine though).
• According to GCC, taking care of undefined behavior is none of its business and can neglect it for the sake of efficiency (e.g. when the optimization flag -O2 is used).

As a result, code such as this acts weird, and might catch someone off-guard:

bool will_overflow(int x) {
    int y = x+1;
    return y < x; // compiler assumes this will never happen => will always return false
}

    will_overflow(INT_MAX)  // returns false instead of true, even though INT_MAX+1 overflows

The GCC-based compiler used by Arduino warns about this when you enable the -Wall flag (which is disabled by default in Arduino):
sketch.ino: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]

There are 2 ways to fix this:

• Fix the documentation (Fix int documentation on overflow (which is UB) reference-en#23) and explain inexperienced programmers what undefined behavior is.
• Make the compiler treat signed overflow "correctly", which the GCC family allows by using the -fwrapv flag.

In general I'm against "fixing" bugs by documenting them (although technically this is not a bug), plus the current behavior is rather counter-intuitive and hard to understand for people with a basic notion of how binary works, so I think the best solution is to fix this in a user-friendly way, i.e. adding the -fwrapv flag.

[matthijskooijman]

Adding -fwrapv sounds reasonable to me. I don't like deviating from the standard, but the standards says it's undefined behaviour, so there is no harm in picking the path of least surprise. The only harm I can see is that third-party cores will not automatically pick up this change (and might never), leading to some inconsistency between cores, but that's probably not a very big problem.

[matthijskooijman]

(also, thanks for the well-explained and researched request!)

[cousteaulecommandant]

Well, since I didn't see much motion I decided to make my own PR to see if it's correct and it gets accepted

[bperrybap]

It's been many years since I tracked this down, but an issue that I've run into is that on the AVR, the compiler is inconsistent in how it treats char vs unsigned char and signed char when the signs match.
In other words, when char defaults to being signed it doesn't always behave the same way as signed char, and if char defaults to being unsigned it doesn't always behave the same way as unsigned char and even more disturbing was that forcing the sign of the char type to be the same as its default makes it behave differently.

I wonder if this option would resolve this to make things consistent?
(I guess I need to go back and find my test cases and try it.....)

[cmaglie]

I've edited the "int" page and removed the wrong indication about rollover: http://edit.arduino.cc/en/Reference/Int

So the reference is ok now. Let's continue on #4987 #4624 about fwrap option.