Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid signed integer overflow in example sketch #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edgar-bonet
Copy link

Running the example sketch SimpleEMGFilters.ino on an Arduino Uno, I got the output:

Squared Data: 1600
Squared Data: 20164
Squared Data: 4294951489

The last line is suspicious: not only is it too large to be plausible, it is even way larger than INT_MAX, which should not be possible for an int.

Further investigation revealed the source of the problem to be an integer overflow in the computation of the square of dataAfterFilter. Signed integer overflow is undefined behavior in C++. Undefined behavior means that anything can happen, including “impossible” results like above.

Computing the square as a long solves the issue.

The `int' type is only 16-bits long in some Arduinos. On those devices,
the square of an analog reading, filtered or not, may not fit in an
`int'. Use `long' instead, in order to avoid signed integer overflow,
which in C++ is undefined behavior.
@edgar-bonet
Copy link
Author

According to the letter of the C++ standard, anything (really anything!) can happen in the presence of undefined behavior. Accordingly, it may seem futile to try to second-guess what the compiler may be thinking. In this case, however, the compiler behavior turns out to be understandable. Taking a look at what it did can give some insight on the kind of surprises that undefined behavior can produce.

In this case, the filter returned 223. Then the line

int envelope = sq(dataAfterFilter);

computed the square of 223, which is 49729 (larger than INT_MAX), and overflowed modulo 216 to −15807. This value was given to Serial.println(). Within Print::print(int n, int base), the number is casted to long and passed to Print::print(long n, int base). Within this method, we have the following code path specific to base 10:

if (n < 0) {
  int t = print('-');
  n = -n;
  return printNumber(n, 10) + t;
}
return printNumber(n, 10);

And here is where things break apart: n is −15807. However, the compiler knows (through link-time optimization) that this number is a square, and it also knows that signed overflows should never happen (it's the programmer's responsibility to avoid them). It can thus legitimately assume that the number is non-negative. Thus, as an optimization, it completely omits the code handling the case n < 0, and calls printNumber(n, 10) with a negative n. Since this argument is of type unsigned long, it wraps modulo 232 to 4294951489, hence the observed behavior.

@QuinIMG
Copy link

QuinIMG commented Jun 26, 2023

Hehe, I ran into this issue a few months ago after I yoinked the example code for a project I'm working on. Wish I had read this pull request back then instead of going down the rabbit hole of debugging it myself 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants