TimeLineWidget overflows

[zonkmachine]

With compile option -DCMAKE_BUILD_TYPE=DEBUG
If you left-click and drag on the timeline you will eventually get negative numbers. On my machine this happens around ~5m19s at 140bpm. This is a bit early for the counter to overflow. It doesn't happen when playing, only if you click the timeline at a certain intervals. It looks like the computation is done by a too small integer when clicking the timeline.

To reproduce:

• On a debug build. -DCMAKE_BUILD_TYPE=DEBUG
• In Song Editor, zoom out to 12.5 or 25%
• Click on the timeline and drag. The time may or may not overflow to a negative number at intervals.

[zonkmachine]

Could be this bug in gcc: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49949
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4

[zonkmachine]

@lukas-w When I first bisected this it pointed to 13357e5 but that didn't make much sense unless it really is a gcc bug and the one I link to above fits the bill. It's a bit on the exotic side but I'm closing this as not our bug.

[zonkmachine]

I may have closed this prematurely. I'm trying to alter the optimization flags from -O2 to something else to see if I can verify the bug. How do you change them? If I search for -O2 I will find output from cmake like CMakeFiles/CMakeOutput.log:/usr/bin/cc -O2 -g -Wall ... but I can't find the source of it. There is the CMakeCache.txt but if I change the flags in there it doesn't seem to affect the compile in any way.

[tresf]

@zonkmachine have you already tried ... -DCMAKE_CXX_FLAGS_DEBUG=-O0 -DCMAKE_C_FLAGS_DEBUG=-O0 ?

[lukas-w]

I can reproduce this with any build type, not just Debug. The gcc bug you linked doesn't look related as it's about complex numbers.

This particular overflow happens at src/gui/TimeLineWidget#L374:

Engine::getSong()->setMilliSeconds(((((t.getTicks()))*60*1000/48)/Engine::getSong()->getTempo()));
//                                     ^~~~~~~~~~~~~~~~~~~~~^
//                                 product too big for 32bit int

The same calculation ticks * 60 * 1000 / 48 is also used in src/core/Song.cpp lines 270, 328, 348, 411, 576 and 645 with potential overflows.

[zonkmachine]

@zonkmachine have you already tried ... -DCMAKE_CXX_FLAGS_DEBUG=-O0 ... ?

Now I have, thanks!

cmake .. -DCMAKE_INSTALL_PREFIX=../target -DCMAKE_CXX_FLAGS_DEBUG=-O0 -DCMAKE_C_FLAGS_DEBUG=-O0 -DCMAKE_BUILD_TYPE=DEBUG

It looks like the command really works but the bug persists.

[softrabbit]

Let's peel that onion...
Engine::getSong()->setMilliSeconds(((((t.getTicks()))*60*1000/48)/Engine::getSong()->getTempo()));

Isn't it a bit more readable like this?
Engine::getSong()->setMilliSeconds( ( t.getTicks() * 60 * 1000 / 48 ) / Engine::getSong()->getTempo() );

Calculating left to right that will be an overflow at 2^31 / 60000 or right before 36000 ticks. Fits the bug description perfectly. (Divide ticks by 48 and then 140 BPM and end up at 5.3-ish minutes).

Optimization will apparently (at least on the compiler I tried) combine those constants into 1250, and the calculation won't overflow until after some hours. But doing the same in the source doesn't help readability much, as of now one can figure out what's going on from the numbers.

My suggestion for a quick fix:
Putting an ll suffix on one of the constants should be enough to force the calculation into long long (minimum 64 bits) and push the bug off to some distant future.

[zonkmachine]

Putting an ll suffix on one of the constants should be enough to force the calculation into long long (minimum 64 bits)

Yup, worked!

and push the bug off to some distant future.

👍

[jasp00]

But doing the same in the source doesn't help readability

This should work: t.getTicks() * ( 60 * 1000 / 48 )

[softrabbit]

| This should work: t.getTicks() * ( 60 * 1000 / 48 )

That should push the limit to some 4.5 hours at 140 BPM. Or half an hour at 999. I'd say good enough for all sane use cases.

[zonkmachine]

This should work: t.getTicks() * ( 60 * 1000 / 48 )

I must admit I'm a bit puzzled by this. I thought the literals were computed at compile time and would end up as 1250, with or without parentheses?

Isn't it a bit more readable like this?
Engine::getSong()->setMilliSeconds( ( t.getTicks() * 60 * 1000 / 48 ) / Engine::getSong()->getTempo() );

Yes please!

Edit: OT comment removed.

[zonkmachine]

OK. I had a go and fixed that line, working on the RC3 release. I hope no one else was busy with this?
Anyway, there were other potential issues here so I'm leaving this open for now.

Other potential problematic lines in Song.cpp #3284 (comment)

[jasp00]

This should work: t.getTicks() * ( 60 * 1000 / 48 )

t.getTicks() * 60 * 1000 / 48 without parenthesis is evaluated from left to right, like:

tick_t x = t.getTicks();
x *= 60;
x *= 1000;
x /= 48;

[michaelgregorius]

Isn't it a bit more readable like this?
Engine::getSong()->setMilliSeconds( ( t.getTicks() * 60 * 1000 / 48 ) / Engine::getSong()->getTempo() );

In my opinion something like this would be even more readable:

Song * song = Engine::getSong();
song->setMilliSeconds( t.convertToMilliseconds( song->getTempo() ) );

That way all the "magic" numbers and the fact that you need to use the MidiTime's ticks would be hidden in that MidiTime::convertToMilliseconds method. It could then also be reused so that these constants would not show up in as many places in the code as they currently do.

And some of the magic that's needed to instantiate the MidiTime instance t above that code might also be moved into MidiTime.

[zonkmachine]

@michaelgregorius It sounds like you have a good plan there. I vote you assign this to yourself and go ahead with it. 🚜

[michaelgregorius]

@zonkmachine Done in #3701!

[michaelgregorius]

I have just merged #3701 into master.

[zonkmachine]

Thanks! Sorry, I didn't review fast enough...
🐳