Signed overflow

[cousteau]

I have mentioned this on GitHub a couple of times now and it always ends up being sort of ignored.  I think it's an important issue with potentially harmful side effects.

The bug report is #4511 - Signed overflow not always wrapping correctly
In short, the Arduino documentation on int says that, if you have an int x = 32767, it's fine to just add 1 to it, and then it "rolls over to the negatives" and becomes -32768 (assuming 16 bit ints as is the case for AVR).
However, this is not true: according to the C++ standard, signed overflow is undefined behavior, so the compiler is free to do optimizations that don't work if signed overflow happens.  GCC does indeed perform these optimizations, at least with the -O2 optimization flag that is currently being used.  Example:

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

#include <limits.h> // INT_MAX
void setup() {
  Serial.begin(9600);
  if (will_overflow(INT_MAX)) Serial.println("Overflows");
  else Serial.println("Doesn't overflow");
}

void loop() { }

will display "Doesn't overflow", but it should show "Overflows", since INT_MAX is 32767 and thus INT_MAX+1 should be -32768, according to the documentation.  This is because the compiler knows that overflow should never happen, and thus optimizes the function (since it was told to optimize stuff): it makes it always return false.

If you enable compiler warnings (-Wall at least), which are disabled by default in Preferences (and I recommend enabling), you'll see that the compiler is actually warning you about this:
warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
   return y < x; // compiler assumes this will never happen => will always return false

So one of these two things MUST be done:

1. Fix the documentation to fit the compiler.
2. Fix the compiler to fit the documentation.

Personally I think option 2 is the best, as the average Arduino user is not expected to be bothered by the C++ standard little details.  Fortunately, GCC has a -fwrapv flag that makes it behave exactly as documented: it treats signed overflow the same way as unsigned overflow: it just "wraps" the number.  The only possible downside of this is that some optimizations that the compiler may be doing would stop working; then again it's possible that those optimizations were already causing some unnoticed trouble.

I tried to make a pull request several times (#4624), but the affected file is modified often so the PR always ends up "rotting" and becoming unmergeable before it can get accepted.

[Cristian Maglie]

Hi

The doc has been fixed, I followed up on the issues.

C

Il 26/01/2017 19:17, cousteau ha scritto:
> I have mentioned this on GitHub a couple of times now and it always ends
> up being sort of ignored. I think it's an important issue with
> potentially harmful side effects.
>
> The bug report is #4511 - Signed overflow not always wrapping correctly

> <https://github.com/arduino/Arduino/issues/4511>

> In short, the Arduino documentation on int

> <https://www.arduino.cc/en/Reference/Int> says that, if you have an int

> x = 32767, it's fine to just add 1 to it, and then it "rolls over to the
> negatives" and becomes -32768 (assuming 16 bit ints as is the case for AVR).
> However, this is not true: according to the C++ standard, signed

> overflow is *undefined behavior,* so the compiler is free to do

> optimizations that don't work if signed overflow happens. GCC does
> indeed perform these optimizations, at least with the -O2 optimization
> flag that is currently being used. Example:
>

> *bool* will_overflow(*int* x) {
> *int* y = x+1;
> *return* y < x; /// compiler assumes this will never happen => will
> always return false/
> }
>
> *#include* <limits.h>/// INT_MAX/
> *void* setup() {
> Serial.begin(9600);
> *if* (will_overflow(INT_MAX)) Serial.println("Overflows");
> *else* Serial.println("Doesn't overflow");
> }
>
> *void* loop() { }

>
> will display "Doesn't overflow", but it should show "Overflows", since
> INT_MAX is 32767 and thus INT_MAX+1 should be -32768, according to the

> documentation. This is because *the compiler knows that overflow should
> never happen,* and thus optimizes the function (since it was told to

> optimize stuff): it makes it always return false.
>
> If you enable compiler warnings (-Wall at least), which are disabled by

> default in Preferences (and I recommend enabling), you'll see that *the
> compiler is actually warning you about this:*

> warning: assuming signed overflow does not occur when assuming that (X +
> c) < X is always false [-Wstrict-overflow]
> return y < x; // compiler assumes this will never happen => will
> always return false
>
> So one of these two things MUST be done:
>

> 1. Fix the documentation to fit the compiler.
> 2. Fix the compiler to fit the documentation.

>
> Personally I think option 2 is the best, as the average Arduino user is
> not expected to be bothered by the C++ standard little details.

> Fortunately, GCC has a *-fwrapv* flag that makes it behave exactly as

> documented: it treats signed overflow the same way as unsigned overflow:
> it just "wraps" the number. The only possible downside of this is that
> some optimizations that the compiler may be doing would stop working;
> then again it's possible that those optimizations were already causing
> some unnoticed trouble.
>
>
> I tried to make a pull request several times (#4624

> <https://github.com/arduino/Arduino/pull/4624>), but the affected file

> is modified often so the PR always ends up "rotting" and becoming
> unmergeable before it can get accepted.
>

> --
> You received this message because you are subscribed to the Google
> Groups "Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to developers+...@arduino.cc
> <mailto:developers+...@arduino.cc>.


--
Cristian Maglie <c.ma...@arduino.cc>