Make signed overflow behave "correctly"

[cousteaulecommandant]

Fix for issue #4511
In short, the documentation says that signed integer overflow causes the value to "roll over". Actually, this results in undefined behavior, with potentially unpredictable results (see #4511 for an example). This patch adds a GCC option to treat signed integers the "right" way so that they roll over as the documentation claims.

[NicoHood]

I remember to have this issue. It would be nice to fix it. I dont know it this fixes it (i dont remember our test sketch, it was very weird stuff going on), but it would be very important to fix.

[buzztiaan]

hey this is excellent! could you guys please accept this pull request?

it will solve this issue : #4511

[buzztiaan]

original poster said he can update the PR if there is any dev actually willing to do his work and merge it :)

[cousteaulecommandant]

Bump!
I resolved the conflict this PR had and it can be merged now.

[cousteaulecommandant]

@cmaglie could you have a look at this, please? I think it could cause some potential trouble.

[Chris--A]

-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 other.

I think we'd need to do some profiling. On an AVR, certain optimisations being disabled could be quite a problem for many currently working projects that rely on code size or critical timing. This I'd assume would affect most integer arithmetic.

[cousteaulecommandant]

I think we'd need to do some profiling. On an AVR, certain optimisations being disabled could be quite a problem for many currently working projects that rely on code size or critical timing. This I'd assume would affect most integer arithmetic.

Personally I don't think this will affect much performance-wise (and in fact it might solve some hidden bugs), but I guess some profiling won't hurt. In any case, this issue must be fixed; Arduino documentation can't tell users they can do something they can't do (so either add the flag or fix the documentation).

In case it is found that a piece of code in the libraries does break with this de-optimization, I would:

• double-check that the optimization isn't actually triggering some kind of undefined behavior (is there a -fsanitize option available?)
• add the -fwrapv option anyway, and add an explicit #pragma GCC optimize "no-wrapv" or __attribute__ ((__optimize__ ("no-wrapv"))) to the file or function needing this optimization.

[matthijskooijman]

One argument against adding this flag is that it will cause problems when people move code from Arduino to other environments without this flag (also when not moving code per se, but when people start to work in other environments and now have expectations that are not true). This isn't a blocking argument per se, but it's good to realize it. Adding a note to the documentation might be sufficient to tackle this, as well.

[cousteaulecommandant]

Well, all of the "Arduino language" is specific to Arduino anyway; there are things Arduino lets you do that are not standard (most of them such as types or special methods are part of the main library, but e.g. you don't need to declare a function before using it), so I'd say Arduino is already non-portable, and iiuc the philosophy is to favor ease of use over teaching correct C++.
I agree that clarifying that this is an Arduino-specific thing in the documentation would be important, though.

[NicoHood]

Currently the behavior is undefined. With the flag its defined. It does not mean without it could not be the same though. Since arduino is meant for beginners they should get less of those weird errors. you cannot tell how many times i had extremely confusing errors with this and how much time i spent on those errors. Beginners will likely just fail. If you port this code without adding the flag, then its your fault. In general its a bad idea to port arduino code, as the environment has tons of "weird" settings, so that does not matter, it only improves stuff.

+1 from my side to add this

[cousteaulecommandant]

Plus, the way it is, the code is still "not portable" anyway; for the code to be portable it would need to behave consistently across platforms, but UB makes everything unpredictable and potentially inconsistent even on a single platform.
Currently, the flag only removes an unpredictable behavior and a warning message that the IDE hides by default anyway, so not using it doesn't provide any benefit in this respect.

(It's good to consider possible regressions and other consequences before applying this change though; nobody likes regressions.)

[cousteaulecommandant]

What is needed for a decision on this PR? The longer it keeps sitting around unattended, the more prone it is to become obsolete and require constant maintenance (I have already fixed the merge conflicts once and I'm not going to fix them again just to see them appear again).

Regardless of whether or not this PR is accepted, Arduino should reflect what the documentation claims it does; otherwise users will get an inconsistent and unexplained behavior. If this is not going to be fixed, at least fix the documentation to stop falsely claiming that ints may "roll over", but procrastinating a decision because "some profiling may have to be done" (and neither doing the profiling nor fixing the documentation) is pointless.

[cmaglie]

I tried to compile the example StandardFirmata to test the output size:

• without -fwrap: 15146
• with -fwrap: 15220

without the option in place gcc is actually optimizing a bit more (at least in size, but I guess there is also a bit of speed improvement). If it was at no cost I've had absolutely no concerns in merging it but right now I think that enabling this option will give little-to-zero advantage.

Did anyone have an example where the signed-rollover may be of any help?
Since it was "undefined" until now, I'm sure that nobody is currently using it.

[cousteaulecommandant]

Thanks for fixing the documentation; it's a first step.

Did anyone have an example where the signed-rollover may be of any help?
Since it was "undefined" until now, I'm sure that nobody is currently using it.

Either nobody was relying on it, or somebody was relying on it unknowingly, but fortunately the effect was not visible for some reason. (Example: #4605 points out that pinMode() et al check for pin validity in an incorrect way, but undefined behavior only happens if they are actually called with an invalid pin.)

I tried to compile the example StandardFirmata to test the output size:

I'll look into that example code to see what may be happening. What board are you compiling for? I get 11092 for the executable + 979 for the static variables on an Arduino/Genuino Uno.

GCC has a very powerful tool, the sanitizer, which allows generating an executable that does a lot of extra checks, such as array boundaries or signed overflow with the -fsanitize=undefined flag, rather than just optimizing things, but I have no idea if this can somehow be used with Arduino. Maybe it would be nice to add this to Arduino somehow, for debugging purposes (probably not to mainstream Arduino, only to a developer's version).

[cmaglie]

What board are you compiling for?

I've compiled for Arduino Leonardo.

GCC has a very powerful tool, the sanitizer, which allows generating an executable that does a lot of extra checks, such as array boundaries or signed overflow with the -fsanitize=undefined flag, rather than just optimizing things, but I have no idea if this can somehow be used with Arduino.

interesting the man page says:

Enable UndefinedBehaviorSanitizer, a fast undefined behavior detector. Various computations are instrumented to detect undefined behavior at runtime.

so it instruments the code to check for undefined behaviours, is still not clear to me what happens when such condition is detected (how the instrumented code tells that it has detected something).

[cousteaulecommandant]

Found the cause, or at least part of it. It seems that that example does indeed benefit from this optimization.

An analysis of the generated .elf using avr-nm -Std shows that the following symbols change in size when applying the -fwrapv flag:

main: +4 bytes
twi_transmit: -2
_Z13sysexCallbackhhPh: +28
_Z17Serial1_availablev: +8
_Z19analogWriteCallbackhi: +12
_Z21reportDigitalCallbackhi: +28
_ZN14HardwareSerial9availableEv: -4

Comparing the result of the disassembled output (avr-objdump -DS) shows the points that result in different code sizes.
One problem with the -fwrapv is that if you do something like

if ((queryIndex + 1) >= I2C_MAX_QUERIES)

then the compiler cannot assume that this is the same as

if (queryIndex > I2C_MAX_QUERIES)

since this ignores the case where queryIndex is INT_MAX, in which case (queryIndex + 1) should roll over and become negative in the first case but not in the second one. This will never happen since queryIndex is a signed char, so it can't be >127, but apparently the compiler didn't realize this. Replacing >= with > and < with <= reduced the executable size by 22 bytes.
Another improvement happens when you replace argv[0] + (argv[1] << 7) with argv[0] | (argv[1] << 7) (which in the former case might cause UB if argv[1] = 255 and argv[0] ≥ 128, which I don't know if it's possible). This reduces the code in 10 bytes with -fwrapv and 2 bytes without it.

In short: there are probably several small optimizations that could be made, that may or may not be currently causing potential UB. Same applies to other examples. So I don't know what to think; after all we're speaking of a 0.5% size difference, and in exchange we get rid of a case of undefined behavior which may be harmful for new users.

---

@cmaglie

is still not clear to me what happens when such condition is detected

That makes two of us :P
On desktop it prints a "runtime error" message; on Arduino it just complains that libubsan doesn't exist. I suspect this library contains routines that get triggered when a runtime error happens. I don't know what would be needed to get this working on Arduino. Research would be necessary.
Maybe the -fsanitize-undefined-trap-on-error flag would be a better idea:

The -fsanitize-undefined-trap-on-error option instructs the compiler to report undefined behavior using __builtin_trap rather than a libubsan library routine. The advantage of this is that the libubsan library is not needed and is not linked in, so this is usable even in freestanding environments.

but I have no idea of what __builtin_trap is either.

[oqibidipo]

but I have no idea of what __builtin_trap is either.

https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/Other-Builtins.html#index-g_t_005f_005fbuiltin_005ftrap-3650

Built-in Function: void __builtin_trap (void)

This function causes the program to exit abnormally. GCC implements this function by using a target-dependent mechanism (such as intentionally executing an illegal instruction) or by calling abort. The mechanism used may vary from release to release so you should not rely on any particular implementation.

[CLAassistant]

All committers have signed the CLA.