FS-UAE 2.5.26dev: Glitches with Amiga chipset screen if Accuracy >-1

[FrodeSolheim]

The good news is that I have managed to pinpoint "when it started to go wrong" a little better. The flickering occurs when WinUAE 2.7.1b2 changes are merged into FS-UAE. Specifically, custom.cpp (it works with 2.7.1b2 changes and custom.cpp from 2.7.1b1).

The bad news is that custom.cpp saw a lot of changes going from 2.7.1b1 to 2.7.1b2.

[FrodeSolheim]

Hmmm, removing this code seemingly fixes the problem:

#ifdef SPEEDUP
/* SPEEDUP code still needs this hack */
int delayoffset = fetchmode_size - (((plfstrt - HARD_DDF_START_REAL) & fetchstart_mask) << 1);
delay1 += delayoffset;
delay2 += delayoffset;
toscr_delay_adjusted[0] = (delay1 & delaymask) << toscr_res;
toscr_delay_adjusted[0] |= shdelay1 >> (RES_MAX - toscr_res);
toscr_delay_adjusted[1] = (delay2 & delaymask) << toscr_res;
toscr_delay_adjusted[1] |= shdelay2 >> (RES_MAX - toscr_res);
#endif

(I realize this might be far from a real solution, but it hopefully gives at least some insight...)

[Toni Wilen]

Originally Posted by FrodeSolheim

Hmmm, removing this code seemingly fixes the problem:

It should break all horizontally scrolling programs..

But does forcing "delayoffset" to zero also fix it?

[FrodeSolheim]

Originally Posted by Toni Wilen

It should break all horizontally scrolling programs..

I can confirm this

Originally Posted by Toni Wilen

But does forcing "delayoffset" to zero also fix it?

No, still glitching :-/

[Toni Wilen]

Too weird..

Does toscr_delay_adjusted[] change when glitch happens? It should not change in WB (at least unless WB screen is horizontally scrolled).

[FrodeSolheim]

Originally Posted by Toni Wilen

Too weird..

It gets weirder...

Originally Posted by Toni Wilen

Does toscr_delay_adjusted[] change when glitch happens? It should not change in WB (at least unless WB screen is horizontally scrolled).

No, at least not for visible vpos (it flips between 0 and 32 when considering all vpos).

But anyway, onto the weird stuff...

Turns out that that the "fix" involving removing some lines did not work in the latest FS-UAE versions. So I went back to the post-2.7.1beta2 merged version where this "fix" removed the flickering.

The strange thing was that setting toscr_delay_adjusted[0] and toscr_delay_adjusted[1] to 0 (after the existing calculations) did not fix the flickering. This was unexpected since they should be 0 by default (static storage), and should also stay 0 when the code is commented out. So manually setting toscr_delay_adjusted[0] and [1] to 0 should have the same effect, except it didn't....

Smells indeed of undefined behavior / compiler optimizations / something...

Testing further, going back to the "fix" of commenting out the lines setting toscr_delay_adjusted[] (now the flickering disappear), but then adding volatile to the declaration (static volatile int toscr_delay_adjusted[2] -- and the flickering reappears .

So, still very strong indications of undefined behavior (of course, I must double-check my test results so far, but...)

EDIT: Disabling compiler optimizations also breaks the "fix" (not surprisingly, as volatile too interferes with optimizations). So presumably, when toscr_delay_adjusted[] is never set, the compiler optimizer assumes (correctly) that this is effectively a constant, and optimizes (away?) some other code as a result. I'm not yet sure if this is just a wild goose chase or not.

[Toni Wilen]

Does behavior also change if you remove "static"? It should force compiler to not "optimize" it away (if it for some reason does it now).

[FrodeSolheim]

Originally Posted by Toni Wilen

Does behavior also change if you remove "static"? It should force compiler to not "optimize" it away (if it for some reason does it now).

Yes, this has the same effect as adding volatile (as expected).

The plan now is to make liberal use of #pragma GCC optimize in the source file to selectively disable optimizations for parts of the file at a time, to try to narrow down where something suspicious is happening.

[FrodeSolheim]

I have fixed it (More details tomorrow, too late already!)

EDIT: the undefined behavior has also been identified now.

[FrodeSolheim]

Yesterday I debugged the source, narrowing in on the problem by progressively compiling less and less parts of custom.cpp without optimizations until the behavior changed again. It turned out that the problem was located in long_fetch_64 and the preceeding shift functions.

I suspected there was an issue with the shift operations (implemented using a 2-item uae_u64 array), so I rewrote the shift code using GCC's 128 bit integer (__int128_t), and with this change, the flickering problem disappeared.

After having confirmed that the problem was located, I took a closer look at the shifting code. The problem is the following lines:

t |= p[0] >> (64 - n);
...
t |= p[1] << (64 - n);

When n is 0 (delay is 0), (64 - n) is 64, and shifting a with shift count == bit length is undefined behavior in C++. What actually happens instead (on my computer, and possibly all x86 computers) is that bits are copied instead of shifted.

This also explains why the flickering disappeared when I disabled the code responsible for setting the delay. When the compiler can prove delay is always 0, the shift functions are simply optimized away.

(I do not really know why the problem does not seem to manifest itself in WinUAE. According to MSVC docs, this shift operation is undefined for MSVC as well. Perhaps it's the same reason the problem does not appear in cycle-exact mode in FS-UAE, i.e. several conditions have to be fulfilled at the same time for the flickering to occur.)

I have submitted a pull request for WinUAE with a fix

[Toni Wilen]

Great. I don't think this was first undefined shift bug

Just wondering why no one said it does not happen in OCS/ECS modes. It would have immediately made it much easier to narrow down. I thought it was all-modes bug because no one said anything.. (long_fetch_64 is AGA only)

[jbl007]

I'm sorry to dig out this old thread again but I still have problems with emulated AGA Amigas. Visual glitches with the default overscan settings are fixed, but now there are glitches custom overscan settings. Also there a glitches with "Chaos Engine AGA" if the player (and the screen) moves horizontally. Vertical movement is OK.
I tried whdload-version and different disk-variants with different configs, but no change.
uae_chipset=ocs "fixes" the problem, but disables AGA of course...
Tested 2.7.5dev and latest git-master.

[FrodeSolheim]

Yes, I get some flickering here as well, with Banshee (in-game) for example. I have merged UAE code from WinUAE 3.2.2, but flickering still occurs. I've also double-checked with WinUAE in Wine, and the flickering does not appear to happen there, at least not with Banshee. It looks like I'll need to do some tedious debugging...

[FrodeSolheim]

@jbl007 I have pushed fix to github. Care to test?

[jbl007]

Small change, big impact. -> It works now.