Is this a bug in rvalue promotion?

Projects Programming
1

I know Arduino language isn't strictly C/C++, but many of us just use and expect a lot of C behavior to work in sketches.
I was just debugging a weird time related bug and came across an unobvious casting / promotion issue. I'm sure others must have hit it but I'd appreciate some feedback as it feels like a compiler promotion bug to me. Certainly for me, it's been responsible for several bugs in sketches where I've tended to use expressions like 'valueMs = (numSeconds * 1000)', especially when numSeconds > 32.

I am using IDE 1.5.4.

So, if you do this:

unsigned long a = 0;
a = a + 33000;

You get 33000.
No surprises.

Now do this (quite common if a is a time in millisecond):

a = a + (33 * 1000);

You will get 4294934760.
The brackets don't matter.

If you repeat the above with 32000 instead, it will be fine.

So it seems rvalues are not promoted to the largest type's resolution if you use an expression like that. It looks like it is using 16bit signed short for the RHS. Isn't this wrong? At least in C/C++ world, where rvalues should get promoted to the largest size of the various operands, and since 'a' is a unsigned long and on the RHS, I would expect (33*1000) to get promoted to unsigned long.

You can work round it in various way,
e.g.

a = a + ((unsigned long)33*1000);
a = a + (33.0 * 1000);
etc.

i.e. force explicit upcasts but this is not obvious.

This triggered 'weird' bugs in code if you use expressions like this:

if ((unsigned long)(millis() - waitUntil) >= (60 * 1000) {

Example to repro the problem:

unsigned long a = 0;
unsigned long b = 0;
void loop() {
Serial.print ("a = ");
Serial.println (a,DEC);
Serial.print ("b = ");
Serial.println (b,DEC);
a = a + 33000;
b = b + (33*1000); // not ok
delay (250);
}

My ouput (a and b should be the same but they are not):
a = 0
b = 0
a = 33000
b = 4294934760
a = 66000
b = 4294902224
a = 99000
b = 4294869688
a = 132000
b = 4294837152

Chris

2

It is standard C/C++

33 * 1000

is the product of two 16-bit integers. The result is a 16-bit integer - even if it doesn't fit. This is then promoted to 32-bits.
Use

33 * 1000L

to force the issue.

Pete

3

The problem is that of when things happen.

Take the following:

a = a + (33 * 1000);

When you compile it, you don't end up with

acc = 33
temp = 1000
acc = acc * 1000
acc = acc + a
a = acc

(acc = accumulator)

Why don't you get that? Because 33 and 1000 are literals, and 33 * 1000 is a literal sum. The preprocessor will evaluate that part for you, so you end up with, after preprocessor optimization:

a = a + 33000;

Now, the preprocessor doesn't know much about what size it's working with at that time, and being an 8-bit compiler it works with a default size of 16 bits for literals. So, the preprocessor would actually end up with the signed value equivalent of 33000 in 16 bits, which is -32536.

So, the sum actually ends up as:

a = a + -32536;

or,

a = a - 32536;

And an unsigned long of 0 with 32536 subtracted from it is 4294934760.

So, you can force the preprocessor to know what size the values are with casting, or a simpler way is with suffixes:

a = a + (33UL * 1000UL);

which forces those literals to be Unsigned Long.

Oh, and yes, the Arduino language IS strictly C++. It just doesn't have access to all the libraries you'd get on a full operating system.

4

Thanks for the reply guys, I know casting etc will fix it and I knew it was a 16-bit interference. However, I don't think its obvious - there's some guys banding about code to handle millis() properly and using (sec x 1000) in the test, which I copied, and that is plain wrong without being explicit about the literal type.

I would've at least expected a compiler warning about loss of precision.

Been spending too much time on 32/64-bit OS's I suppose.
To illustrate this I've compiled essentially the same code on gcc on my Mac, it works fine.

#include <stdio.h>
int main (int argc, char **argv)
{
unsigned long a = 0;
unsigned long b = 0;
a = a + 33000;
b = b + (33 * 1000);
printf ("a = %lu\n", a);
printf ("b = %lu\n", b);
}

hriss-MacBook-Pro:~ chris$ gcc p.c
Chriss-MacBook-Pro:~ chris$ ./a.out
a = 33000
b = 33000

5

That'll be because your mac is 64-bit - it'll be working with at minimum 32-bit values for literals, not 16.

6

Sure, I know that, but why no warning about loss of precision?
I've found two other sketches with this 'sleeping' bug now.

7

The Arduino IDE turns off the warnings. IMHO, not a good thing to do, but there it is.

Because 33 and 1000 are literals, and 33 * 1000 is a literal sum. The preprocessor will evaluate that part for you

The preprocessor doesn't touch that at all. When the compiler parses 33 * 1000 it sees that both operands of the multiply are constants and the compiler's optimizer evaluates (according to the rules of C/C++) the expression and replaces it with a new constant - if the required optimization flag has been specified.

Pete

8

Arduino very helpfully hides all warnings (-w), which is just bloody stupid.

Changing that to -Wall gives you:

untitled1.ino: In function ‘void setup()’:
Warning at line 5 in file untitled1.ino:
    integer overflow in expression

el_supremo:
The Arduino IDE turns off the warnings. IMHO, not a good thing to do, but there it is.

Because 33 and 1000 are literals, and 33 * 1000 is a literal sum. The preprocessor will evaluate that part for you

The preprocessor doesn't touch that at all. When the compiler parses 33 * 1000 it sees that both operands of the multiply are constants and the compiler's optimizer evaluates (according to the rules of C/C++) the expression and replaces it with a new constant - if the required optimization flag has been specified.

Pete

Sorry, I didn't mean the preprocessor, as in avr-cpp, but the preprocessing phase of the compilation optimization where it reduces literal expressions to single values.

9

Thanks guys, I like to use -Wall in my (new) projects at work, from now on always gonna do it on Arduino projects.
We should make this the bloomin default, helps new peeps write better code too.

10

What... and expose all the warnings in the Arduino core?!

11

hehe, well if we keep hiding them no one will fix them.
I think its a crime this compiles without warning right now by default:

int c;
if (c == 1)

i.e. doesn't even gripe about uninit'd vars.

Seems fairly easy to modify the IDE for -Wall if I have the IDE on linux/Mac, but not so easy on Windows - any simple trick or do I have to change compiler.java myself and rebuild? Think its time to use a proper IDE...

12

@majenko

Arduino very helpfully hides all warnings (-w), which is just bloody stupid.

No it's not stupid. Just think of the number of panicked newbies if they saw the full list of warnings. Ans most of them in the standard lib's preovided with the IDE, and all of those repeated questions about each warning. Then thing of the work for those (unlike your self) who can tell which matter and which don't!

It's far from stupid, it's the only sane way to go.

Mark

13

holmes4:
most of them in the standard lib's preovided with the IDE

Two wrongs don't make a right. The Arduino runtime library should not contain code that generates compilation warnings. Warnings serve a useful and important purpose and they should not be ignored. Disabling them by default is stupid.

14

a = a + (33 * 1000);

Aren't all the RHS operands supposed to be promoted to "long" since "a" is a long (and also used on the RHS)?

The easiest workaround is:a = a + (33 * 1000L);

15

holmes4:
@majenko

Arduino very helpfully hides all warnings (-w), which is just bloody stupid.

No it's not stupid. Just think of the number of panicked newbies if they saw the full list of warnings. Ans most of them in the standard lib's preovided with the IDE, and all of those repeated questions about each warning. Then thing of the work for those (unlike your self) who can tell which matter and which don't!

It's far from stupid, it's the only sane way to go.

Mark

The bigger stupidity was hardcoding the compiler & linker options in the java code
and then not providing the ability to alter them without modifying the IDE source code
and rebuilding the IDE.

At least now with the 1.5x IDE, you can go in and modify the compiler and linker options
by editing the platform.txt file.

The ideal solution would be a preferences option in the IDE to turn on -Wall (all warnings)
so those that care about creating clean code could do so and those that
don't could leave it off.

--- bill

16

UECIDE allows you to set your own c / cpp / ld flags (and more - you can modify every aspect of a core / board / compiler) from within a sketch itself:

#pragma parameter cppflags=-g  -Os -Wall  -ffunction-sections  -fdata-sections

Though, I am gradually cleaning up and standardizing the cores' and compilers' flags and adding better control options, so on some cores you can just go:

#pragma parameter warnings=-Wall