#377 Fix for bug ticket #2030: Plus/4 emulator hangs after a while

[Uffe Jakobsen]

This patch fixes bug ticket #2030: Plus/4 emulator hangs after a while

https://sourceforge.net/p/vice-emu/bugs/2030/

Fix typecast for clock related marco - add explicit typecast to sections that can causes negative overflow

[gpz]

did you try changing the type of cycles_per_line to int instead (less casts are better)?

[Uffe Jakobsen]

did you try changing the type of cycles_per_line to int instead (less casts are better)?

ted.cycles_per_line is already an int (declared in tedtypes.h)

Implicit type conversion is really the issue here

The issue is that the (existing) logic of the calculation of (from ted-irq.c) (line - current_line) needs to be able to become negative (as line may not be larger than current_line in all cases)
Due to Implicit type conversion I cannot see a way without adding an explicit (int) cast

That result - multiplied with the cycles_per_line will then be added (or subtracted if negative) from the calculated clock (uin64_t).
In case of subtraction the clock may go longer back then the current clock - but the (existing) code will later on correct that by adding the total number of clocks for a whole screen to the result...

The cast could also be placed like this (ted.cycles_per_line * (int)(line - current_line)));

But I felt that is was important to indicate that the whole calculation result needed to be int - hence this:
(int)(ted.cycles_per_line * (line - current_line))

The whole logic is as-is kind of awkward - but I did not want to rewrite too much - so I just fixed the needed details - in order to make a patch with minimum intrusion.

PS: the above represent my personal conclusions and deductions from debugging the code - it may be wrong :-)

[Olaf Seibert]

About (int)(ted.cycles_per_line * (line - current_line))):
both line and current_line are unsigned, hence the difference between them is unsigned, hence the product is unsigned. I think that we should fix the issue closest to the source, so both line and current_line should be cast to int, so that the difference is a proper int. Otherwise, if it's unsigned but should be negative, it's a very large number and casting that to int is (I think) not well defined or even Undefined Behaviour(TM).

[gpz]

yes, sounds reasonable. plus: add a comment that explains why this happens :)

[Thomas Winkler]

Awesome, great!
Thank you!

Is this Fix in thje next nightly included?

[Thomas Winkler]

I can confirm it is fixed with r45224.

Thank you very much!

[gpz]

The patch wasn't applied yet :D

[Thomas Winkler]

Okay, it is NOT solved.
Much better but NOT solved.

Emulator doesn't do this READY thing.

But it freezes.
Cursor is blinking normally.
But no keyboard entrty is possible after a long while.

[Uffe Jakobsen]

@Thomas Winkler:

The easiest test is just to run this commandline:

xplus4 -warp -keybuf "10 print ti: goto 10\nrun\n"

If the TI output counter passes 144896 and continues without freeze - then the issue is solved :-)

[Uffe Jakobsen]

Thanks for the feedback

Attached is a revised patch - it may not be complete as I'm not sure if @gpz wants the comment/ explanation in the commit message (as it is now) - or if it should be duplicated four times in the patch - one for every change - or something else...

Comments are welcome :-)

[gpz]

I meant a short comment in the code of course - the your git commit message will be lost anyway when i apply the patch locally and then commit via svn :)

[Uffe Jakobsen]

Well, technically it is possible to copy the commit message and add it as a SVN message - but I got the point :-D

[Uffe Jakobsen]

patch rev3 uploaded - let me know if anything is missing/wrong

/Uffe :-)

[gpz]

The comments are fine now - however, it produces a bunch of new warnings that should also be fixed :) see https://pastebin.com/AYd2jb6n

[Uffe Jakobsen]

I have lots of warnings from other parts of the source - just not the ones that you have :-)

This is the warnings I have with GCC-14.1.1 (on archlinux)
https://pastebin.com/bJwGGGHx

Now I switched to clang-18.1.8 (on archlinux)
Now warnings that GCC gave are now gone - but I see the warnings that you reported.

I'll look into them :-)

[gpz]

Yes, but those "array out of bounds" things are the only ones that should be there (we can't remove them without rewriting a lot of stuff) :)

[Uffe Jakobsen]

Just for the record

I did a build of latest trunk with clang - that is without this patch

And the warnings you refer to are still there - they are not a result of this patch...

I'll be happy to try and fix them - but I think that it should be in a separate patch

[gpz]

OH! yes, same here - guess i didn't make a full build for a while (not since my last compiler update). In that of course its fine to apply your patch. Applied in r45233

And of course feel free and go ahead and fix those warnings :) (I wonder now what got you interested in xplus4...)

[Uffe Jakobsen]

Thanks :-)

BTW: just above the TED_LINE_START_CLK in tedtypes.h there are two FIXME: notes - they refer to the two functions that my patch has added type casts into.
But I have no idea if this is what the FIXME statement refers to - or something else ?
In other words should these FIXME be deleted or ?

/* FIXME: assigned to (CLOCK)ted.raster_irq_clk in ted-irq.c:ted_irq_set_raster_line() */
/* FIXME: assigned to (CLOCK)ted.raster_irq_clk in ted-mem.c:ted1c1d_store() */
#define TED_LINE_START_CLK(clk)     ((CLOCK)(ted.last_emulate_line_clk + (((clk) - ted.last_emulate_line_clk) >= 114UL ? 114UL : 0UL)))

[gpz]

I don't remember.... could be i added them some weeks ago, when i tried to fix this stuff :) So yeah, probably should be removed now.

[Uffe Jakobsen]

@gpz you are right the FIXME comments was added by you about 2 months ago in commit rev 45166 - so I guess they can be removed

[Querino]

https://sourceforge.net/p/vice-emu/code/45166/#diff-7

maybe the (unsigned int) should be back in #define TED_RASTER_CYCLE(clk)

[Uffe Jakobsen]

yes but that is another patch - right now we are discussing a possible clean up of comments as a tail-fix of the already applied patch