Strange bug with if statement (UB maybe)

Help & Information
1

Screenshot

(image for attraction)

I’m currently working on exact copy of Bounce game, which was installed on old Nokia Phones. Now i have strange bug and its behavior is like this:

  1. Change this on line 20:
/* Strange bug if split ternary to if-else */
level = Level(prevScene == Scene::NEW_GAME ? menuNewGame.choice - 1 : level.levelNo + 1);

to this:

if (prevScene == Scene::NEW_GAME) level = Level(menuNewGame.choice - 1);
else level = Level(level.levelNo + 1);

and get stucked Arduboy after choosing a level. I don’t understand what compiler can change radically, if logic is same as previous.
Can anyone advise how to solve this problem and how to debug such cases if you are not strong in asm?

GitHub repo: https://github.com/YariKartoshe4ka/Arduboy-Bounce-Nokia

1 Like
2

It really shouldn’t make any difference but maybe try adding the curly brackets {} to the if else statement to make sure that is explicit?

To debug maybe inverting the if statement logic and testing the behavior. Or adding additional if else statements.

**sarcasm also make sure you are printing out all of your variables to the serial console too :slight_smile:

3

The only thing that comes to mind is that the ternary operator forces both sides to use the same type, which then gets converted to uint8_t to pass to the Level constructor. With separate constructor calls, you might have different types that then get converted. Best way to tell is to just output the values there to see if you’re ending up with different levelNo values in the two case.

4

try maybe

```
int L;
if (prevScene == Scene::NEW_GAME) 
 L = menuNewGame.choice - 1;
else 
 L=level.levelNo + 1;
level = Level(L);
1 Like
5

I replicated this behavior in the debugger. The ternary form optimizes to in-place construction within the level variable, while the if-statement results in the construction of a temporary Level object followed by assignment to level. Both are valid code generation choices by the compiler, though the bad behavior results in the latter case because it happens to produce a stack overflow, likely due to the additional stack space allocated for the temporary Level object.

In the screenshot you can see that the stack frame for sceneInitGame is 855 bytes, and a stack overflow occurs after calling to the Level constructor.

image
image732×564 47.2 KB

To avoid being at the mercy of the compiler you can refactor to use a initialization method in Level instead of construct and assign.

4 Likes
6

Wow nice find! That’s incredible @brow1067

7

It’s similar, but technically it’s not the same logic.

The ternary case is more similar to what @rapso suggested - it’s the argument to the constructor that depends on the condition, and the constructor is called regardless.

The second case makes the entire assignment conditional, so the compiler would have to do extra work on the latter code to produce the same result because it would have to look at both branches and establish what they have in common (i.e. they’re both calling the same constructor and both performing an assignment).

(That said, some versions of GCC might be able to optimise this case. I’m not sure which version of GCC is in use here.)

Either way, an if is never 100% the same as the conditional operator, they have different semantics. Sometimes they can produce the same observable effects, but they follow different paths to get there.

This seems surprising at first, until you realise that Level is actually a really huge type.
(Well, ‘really huge’ by Arduboy standards. On desktop you’d be laughing.)

By my calculations, sizeof(Level::entities) alone is 672, and sizeof(Level::states) is another 150 on top of that…

All in all I make it to be about 845 bytes.

So one Level object plus Arduboy2’s frame buffer (1024 bytes) already takes up 1869 of the available 2560 bytes, attempting to allocate a second Level object is going to exhaust the RAM regardless of where it’s allocated.

There is one other option as well: placement new.

// Manually destroy the existing object
level.~Level();

// Use placement new to construct the new object
new (&level) Level (prevScene == Scene::NEW_GAME ? menuNewGame.choice - 1 : level.levelNo + 1);

Using if that would be:

// Manually destroy the existing object
level.~Level();

if(prevScene == Scene::NEW_GAME)
	// Use placement new to construct the new object
	new (&level) Level (menuNewGame.choice - 1);
else
	// Use placement new to construct the new object
	new (&level) Level (level.levelNo + 1);

Though personally I’d advise sticking to the conditional operator anyway because it just makes more sense in this case.

If the only statement inside the body of both the if and the else is an assignment to the same variable then a conditional operator nearly always makes more sense anyway.

(Note: In this case you could get away with not calling the destructor, and it’ll be optimised away anyway because in this case Level’s destructor doesn’t do anything, but there are some types that would require it, so I added it for the sake of completion/correctness.)

2 Likes
8

Thank you all very much!!! @brow1067 is my hero :smiley:. Which debugger is in the screenshot?
Thanks for the great explanation @Pharap.

I would never have thought that this is an overflow. it seemed that initialization takes place on the spot and not in a separate (temporary) object. Fixed by replacing constructors with void init methods

2 Likes
9

In this case it’s technically an assignment, not initialisation, because the destination object already exists.

// Initialisation:
X x = X();

// Assignment:
// (Because x is already declared)
x = X();

// Initialisation:
// (Implicitly calls the default constructor)
Y y;

// Assignment:
// (Because y is already declared)
y = Y();

(Initialisation is usually part of either a variable declaration or a new expression.)

For assignments creating the temporary object is actually the default behaviour.
Eliding that temporary is an optimisation, and it can only be performed in certain circumstances.

For initialisation it depends on the language version.
As of C++17, X x = X() would guarantee copy elision.
Prior to that it’s up to the compiler. I expect most compilers would attempt it in at least some circumstances, but C++17 is the first language version to make it an actual requirement. (Arduino is stuck using C++11.)

2 Likes
10

That’s super interesting! I didn’t know that standards could guarantee specific compiler optimizations. Do you know if c++17 guarantees copy elision for assignment from a temporary:

// initial construction
X x;
X x2;

// copy assignment operator
x = x2;

// guaranteed?
//     X::X() to be called with this == &x instead of a temporary
//     copy assignment operator elided
x = X();
11

Assuming the cppreference article is up to date, copy elision is only guaranteed for return statements (so-called ‘(named) return value optimisation’) and object initialisation, but seemingly not for assignment, even after C++17’s rule changes.

I.e.

// Initialisation: Temporary _must_ be elided
X x = X();

// Assignment: Temporary _may_ be elided, but this isn't required
x = X();

Notably, the elision occurs even if the copy/move constructors would have side effects (e.g. printing to the screen, memory allocation) or even if they would be inaccessible or deleted.

Though according to the article (assuming I’m reading it correctly) as of C++17 this isn’t considered an optimisation because it’s now mandated behaviour. (I.e. there’s no copy to elide because the standard behaviour is now to not produce one in the first place - one can’t remove what isn’t there.)

In essence, it’s as if X x = X(arguments); were being rewritten to X x(arguments);, or that both forms have the same semantics (as of C++17). (I don’t know if they do technically have the same semantics, but it’s at least comparable.)

(I understand their need for pedantry, but I still consider it an optimisation in relation to the behaviour of the previous language version.)

Just to give another example: C++14 added some allocation elision rules. (Unlike the earlier example, this one is acknowledged as an optimisation, in part because it only happens under certain circumstances.)

They’re practically irrelevant for Arduboy development because there isn’t enough memory to warrant using non-placement new, but it would be relevant for other systems, and would arguably fall under the class of ‘standard-mandated optimisations’.

Lastly, just to give a non-C++ example, most LISP variants (e.g. Scheme, Clojure, Racket) are heavily predicated on tail call optimisation - trying to implement a LISP variant without tail call optimisation would probably result in a lot of stack overflows because LISP code is often written in a tail-recursive way (and that practice is expected and encouraged).

It’s been a long time since I glanced at a LISP-like standard, but I would presume it must be standard-mandated given how important it is to normal LISP functioning.

(Apologies if anyone’s eyes glossed over at any point.)

1 Like