Major Bug in VBA (for GB/GBC mode)

[Dwedit]

VisualBoyAdvance has many major bugs regarding Echo Ram (E000-FDFF) on the GB/GBC. It treats the echo ram as a separate block of memory instead of a mirror of (C000-DDFF).

I've gone through GB.cpp and found a list of which lines need to be changed:

gbCopyMemory function is not echo-ram aware. Needs a check to see if the source address is in Echo Ram area, then subtract 0x2000 from the source address.
We don't need to validate the destination address, since it only DMAs to either OAM or VRAM.

Replace it with:

void gbCopyMemory(u16 d, u16 s, int count)
{
	if (s>=0xE000 && s<0xFE00)
	{
		s-=0x2000;
	}
	while(count) 
	{
		gbMemoryMap[d>>12][d & 0x0fff] = gbMemoryMap[s>>12][s & 0x0fff];
		s++;
		d++;
		count--;
	}
}

gbWriteMemory needs this change:

Replace this:

	if(address < 0xfe00)
	{
		gbMemoryMap[address>>12][address & 0x0fff] = value;
		return;
	}

with this:

	if (address<0xE000)
	{
		gbMemoryMap[address>>12][address & 0x0fff] = value;
		return;
	}
	if(address < 0xfe00) 
	{
		gbMemoryMap[(address-0x2000)>>12][address & 0x0fff] = value;
		return;
	}

gbReadOpcode is just really bad code, it has a major bug in that the switch statement doesn't do anything!
Replace the switch:

	switch(address & 0xf000)

with this:

	switch((address>>12) & 0x000f)

Then right before the last return add this:

	if (address>=0xE000 && address<0xFE00)
	{
		return gbMemoryMap[(address-0x2000)>>12][address & 0x0fff];
	}

gbReadMemory needs this right before the last return:

	if (address>=0xE000 && address<0xFE00)
	{
		return gbMemoryMap[(address-0x2000)>>12][address & 0x0fff];
	}

I already posted this on the TASVideos forum for the guy who maintains the re-recording version, and I'm posting here too.

VBA CVS Version does NOT have the bug.
VBA-S 31.07.2006 does NOT have the bug.
Costis's version 1.80 does NOT have the bug.


VBA-rerecording-19 has the bug.
Official VBA Source code 1.7.2 has the bug.
Forgotten's 1.8.0 beta 3 has the bug.
VBA Smooth 6.5 has the bug.
Collectors Edition 1.0 has the bug.

So it only affects the outdated forks. Someone really needs to get the fixed-up VBA source code out there so that programmers don't base their versions on an old buggy version!

[Chris21]

oh nice! thx

[Hiei-YYH]

if you know what's wrong, why don't you fix and send a patch?

but i don't believe that will change something since it looks like vba is being "forgotten" since forgotten (lol) quited the project.

[djrobx]

Does someone happen to know a game that does not work or glitches due to this bug? According to his research the bug is fixed in the VBA CVS, but I do not see the fix he posted in the CVS version. If I knew an impacted game I could test by simply running it (and could then verify the result once fixed)

The CVS version does have some changes to the gbReadOpcode function but still has the completely inoperable switch statement!

[mudlord]

I would also like to know any affected games. That way, I can implement the patches tonight for the bugs in mine and djrobx's VBA build.

[cowfez]

What type of error might this bug cause? Perhaps someone knows of a specific bug but not the cause.

[Dwedit]

The bug was actually fixed a long time ago, the only problem is that nobody ever updated the official VBA source release or CVS, so I didn't know it was fixed a long time ago.
The newest (hard to find) builds of VBA don't have the problem.

[djrobx]

OK, but the "most current" official CVS clearly still has the error that makes that switch block useless. We should either fix it per your instructions or remove the switch block! I am just leery of fixing it without a test case.

Again, do you happen to rememeber a game that glitched or didn't run that lead you to find the bug?

Several of us have scoured the web looking for the various VBA sources. So many of the VBA projects appear to have been abandoned or source is "lost" or not available. So we are doing our best to re-integrate necessary changes. So many developers have done some great things with VBA, we want to consolidate the fragments and move forward.

[Squall-Leonhart]

i've never seen an issue with this, though, i did try a Tekkaman slade GB rom once and it wouldn't work :\

[Dwedit]

In GB.c, the fix to that switch block would have the effect only in rare situations where the program counter was into a bad memory area usually not containing code. If not fixed, it would just execute NOPs while in there. If fixed, it would fetch instructions from the hardware registers area.
Double check the ASM code generated by the invalid switch block, if the compiler just removes all of it, the the invalid switch block may have been a perverse "optimization". Otherwise, put it back in for the sake of accuracy.
Also, double check the part with mapperReadRam, it looks like it may fall back to reading from mapper ram anyway even if mapper ram is disabled or absent? I would think it would read some junk value instead, but I don't have a GB devkit to find out what it should read.

The GB echo ram bug is clearly visible, though it only affects games which read and write to out of bounds memory.
You can see the bug in Super Mario Land 2 (version 1.0).
# Enter a level previously cleared (Pumpkin Zone 1 is a great place to try this)
# Go down a pipe
# While going down the pipe, press Start then Select to exit the level
# Re-enter the level, mario will pipe down through the floor.
# The visible level becomes the Echo RAM area of memory.
## If emulated correctly, you will get empty space in pumpkin zone 1.
## If emulated incorrectly, you get a screen full of breakable blocks.
VBA versions prior to the fix gave a screen full of breakable blocks, because they made echo ram separate memory instead of a mirror of main memory. (Man, GB programmers would have killed for an extra 8k of ram!)
Fixed VBA versions will behave correctly, and get the screen of empty space.

VBA Rerecording 20 uses the newer GB emulation code, and that has the echo ram bug fixed.

I also made a custom VBA build and released it to the internet, it's called "vba_romwrite". It's identical to stock vba from sourceforge, but the cartridge rom area is writable. I made that version so I could test code designed to run on an M3.

[Squall-Leonhart]

if VBA Rerecording 20 has the fix, then mudlord can implement it in VBA-M

[mudlord]

The GB echo ram bug is clearly visible, though it only affects games which read and write to out of bounds memory.
You can see the bug in Super Mario Land 2 (version 1.0).
# Enter a level previously cleared (Pumpkin Zone 1 is a great place to try this)
# Go down a pipe
# While going down the pipe, press Start then Select to exit the level
# Re-enter the level, mario will pipe down through the floor.
# The visible level becomes the Echo RAM area of memory.
## If emulated correctly, you will get empty space in pumpkin zone 1.
## If emulated incorrectly, you get a screen full of breakable blocks.
VBA versions prior to the fix gave a screen full of breakable blocks, because they made echo ram separate memory instead of a mirror of main memory. (Man, GB programmers would have killed for an extra 8k of ram!)
Fixed VBA versions will behave correctly, and get the screen of empty space.

Thanks for the indepth bug description. I will check this out when I implement re-recording support to VBA-M.

EDIT: Thanks Dwedit, update is now in VBA-M SVN.

EDIT2: Seems, whenever I implement your lines I get a opcode error tho....

EDIT3: Fixed

[Dwedit]

Are you trying to apply the echo ram fix? I'd highly recommend against applying the code I posted in the first post, and instead use the newer official code which also fixes that bug. VBA Rerecording 20 took its GB code from the newest VBA source material, so if you don't have that code, ask nitsuja where he got it from. Nitsuja left the GBA code unchanged from version 1.7.2.

[mudlord]

Are you trying to apply the echo ram fix?

Yes

I'd highly recommend against applying the code I posted in the first post, and instead use the newer official code which also fixes that bug.

Ah okay, I'll then revert back to how the code originally was.

[Dwedit]

If you are even able to apply my fix, then you are using really old VBA code. That's all I'm saying. That means go get the newer code first.
The newer code doesn't even resemble the old code well enough to apply the same fixes.

EDIT:
Okay, it looks like there are still lots of problems even with the newer source code, so things still need fixing.

The difference between the 2004 and 2006 version of VBA's GBC emulation is like night and day... If you still have the 2004 version of VBA, throw out its GB emulation code.
Amazing that the old version had such bad timing...
Anyway, I'm looking over the GB emulation code again...

Double check the HDMA code to see if it can ever go out of bounds either at source or destination. Using normal boundaries, the source address can not exceed the end of RAM, and the destination can not be outside of VRAM. Should make sure that the code never violates those rules.

lots of other checks... I wonder if it's possible to just remap the two pages so that they will always act as echo ram? But I think that would screw up the rest of the FFxx area.

[djrobx]

That means go get the newer code first.

That's what we want to do. You say the "2006 version", but take a look here:

SourceForge.net Repository - [vba] View of /VisualBoyAdvance/src/gb/GB.cpp

This is the August 2006 official version, and it *still* has the bad switch block, and it still resembles your changes well enough to apply them.

VBA Rerecording 20 took its GB code from the newest VBA source material, so if you don't have that code, ask nitsuja where he got it from. Nitsuja left the GBA code unchanged from version 1.7.2.

The full sources to VBA rerecording 20 are there on your link. The bad switch block is in that version, too.

Mudlord, since Dwedit has confirmed that rerecording 020 works better, we should merge all of its updates in. You were going to add rerecording support anyway, so this seems like a good opportunity to do so. I highly suggest checking out WinMerge if you haven't already.

[mudlord]

Mudlord, since Dwedit has confirmed that rerecording 020 works better, we should merge all of its updates in. You were going to add rerecording support anyway, so this seems like a good opportunity to do so. I highly suggest checking out WinMerge if you haven't already.

Will do. And I have been using WinMerge as well. Made things very simple for the VBALink implementation, so its definately a excellent tool.

[Squall-Leonhart]

lol win merge, all i have handy is Total commanders Compare function and it highlights the differences, lets you copy them across, etc.

[pokemonhacker_]

Originally Posted by Dwedit

the invalid switch block may have been a perverse "optimization".

I think you're right on that one. If I had made that change for a real purpose, that means I would have tested the result on a game (or on the hardware), and obviously found out that my change wasn't working properly...

[MasterPhW]

Originally Posted by pokemonhacker_

I think you're right on that one. If I had made that change for a real purpose, that means I would have tested the result on a game (or on the hardware), and obviously found out that my change wasn't working properly...

Nice.... pokemonhacker is back! Did you check the VBA-M project?