Broken SD emulation in 140

The Uzebox now have a fully functional emulator! Download and discuss it here.
Post Reply
CunningFellow
Posts: 1445
Joined: Mon Feb 11, 2013 8:08 am
Location: Brisbane, Australia

Broken SD emulation in 140

Post by CunningFellow »

Uzem140 was not working with T2K. So started some testing. It could not find the file utempset.lvl.

So then I modifed the SDTest.HEX to just dump the contents of Sector 0x0001
SDTest.hex
(38.39 KiB) Downloaded 784 times
Uzem 1.30 gives this on my machine

EB3C90 757A656D53446500 0002 40 0100 02 0002 00

So I traced that out by hand

EB3C90 = Bootjump
757A656D53446500 = OEMName "uzemSDe"
0002 = 512 = Bytes per sector
40 = 64 = Sectors Per Cluster
0100 = 01 = Reserved Sector Count
02 = 02 = Table Count
0002 = 512 = Root Entry Count

The Uzem140 I just compiled gives

EB3C90 757A656D5344650000 0002 4000 0100 0200

All the 8 bit values are padded out to align on 16 bit boundaries.

The struct is "packed" so I don't know why this is happening.
CunningFellow
Posts: 1445
Joined: Mon Feb 11, 2013 8:08 am
Location: Brisbane, Australia

Re: Broken SD emulation in 140

Post by CunningFellow »

I don't know if it is my enviroment (my GCC is newer) or if it is 1.40 but I found this page

http://www.bttr-software.de/forum/mix_e ... p?id=11767

and added

-mno-ms-bitfields

to the global flags in the makefile and it now gets the directory structure correct
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: Broken SD emulation in 140

Post by Artcfox »

I ran git bisect, and found the first change that broke T2K:

Code: Select all

ff59f19b458a1cf36f07065ed29e7d475df93f2d is the first bad commit
commit ff59f19b458a1cf36f07065ed29e7d475df93f2d
Author: Uze <uze@belogic.com>
Date:   Fri Aug 28 18:38:31 2015 -0400

    -fixed SD CMD12

:040000 040000 b2b787d49d8fcfb963a4d85bf52b68e969654f19 01ffdcd4a188d2e47574e65bf6a440c478af8ded M	tools

Code: Select all

diff --git a/tools/uzem/avr8.cpp b/tools/uzem/avr8.cpp
index ada66f2..d81c45a 100644
--- a/tools/uzem/avr8.cpp
+++ b/tools/uzem/avr8.cpp
@@ -2216,11 +2216,12 @@ void avr8::update_spi(){
 
         case 0x4c: //CMD12 =  STOP_TRANSMISSION
             SPDR = 0x00;
-            spiState = SPI_RESPOND_SINGLE;
-            spiResponseBuffer[0] = 0x4; // card is in "trans" state
-            spiResponseBuffer[1] = 0xff; //ready
+           	spiState = SPI_RESPOND_SINGLE;
+           	spiResponseBuffer[0] = 0xff; //stuff byte
+           	spiResponseBuffer[1] = 0xff; //stuff byte
+           	spiResponseBuffer[2] = 0x00; // card is ready //in "trans" state
+           	spiResponseEnd = spiResponsePtr+3;
             spiResponsePtr = spiResponseBuffer;
-            spiResponseEnd = spiResponsePtr+2;
             spiByteCount = 0;
             break;
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: Broken SD emulation in 140

Post by Artcfox »

I found the bug!

The following lines got transposed in that fix:

Code: Select all

spiResponseEnd = spiResponsePtr+3;
spiResponsePtr = spiResponseBuffer;
It should read:

Code: Select all

spiResponsePtr = spiResponseBuffer;
spiResponseEnd = spiResponsePtr+3;
now T2K works correctly with the latest uzem140 branch! :D
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: Broken SD emulation in 140

Post by Artcfox »

I also partially solved why SD emulation doesn't work with the Emscripten build. I got it to break in a native build when I changed the compiler from g++ to clang++. My best guess is that the current implementation is relying on undefined behavior, and we just got lucky that it works with g++. The most likely culprit is the unions inside the SDfat structure. See this page for why. I ran into this kind of undefined behavior when I tried using unions for my FAT code, and also for my NECIR library, and the solution was to just not use them and you'll avoid type-punning.

Edit: So I got rid of the unions for the spiArg stuff, but clang++ is still inserting a "ud2" instruction (undefined instruction) into the generated assembly inside:

Code: Select all

int SDEmu::seek(int pos) {
	position = pos;
}

Code: Select all

0 in SDEmu::seek of SDEmulator.cpp:225
1 in avr8::SDSeekToOffset of avr8.cpp:2839
2 in avr8::update_spi of avr8.cpp:2578
3 in avr8::update_hardware of avr8.cpp:868
4 in avr8::exec of avr8.cpp:1846
5 in main of uzem.cpp:437

Code: Select all

0x0000000000416760 <SDEmu::seek(int)+0>:  push   %rbp
0x0000000000416761 <SDEmu::seek(int)+1>:  mov    %rsp,%rbp
0x0000000000416764 <SDEmu::seek(int)+4>:  mov    %rdi,-0x10(%rbp)
0x0000000000416768 <SDEmu::seek(int)+8>:  mov    %esi,-0x14(%rbp)
0x000000000041676b <SDEmu::seek(int)+11>: mov    -0x10(%rbp),%rdi
0x000000000041676f <SDEmu::seek(int)+15>: mov    -0x14(%rbp),%esi
0x0000000000416772 <SDEmu::seek(int)+18>: mov    %esi,0x10a200(%rdi)
0x0000000000416778 <SDEmu::seek(int)+24>: ud2    
and it just exits because of the undefined instruction opcode. I looked at the generated assembly, and in both places where it inserted the "ud2" opcode was where it was writing to position.

Further searching on the Internet reveals:
LLVM generates a ud2 in some cases where it can locally prove code is
unreachable because it has undefined behavior. You're most likely
hitting that.
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: Broken SD emulation in 140

Post by Artcfox »

Found the bug!

Both the SDEmu::read and SDEmu::seek functions were declared with a return value of int, but did not return a value, which is definitely undefined behavior, and thus the clang++ compiler was free to do anything it wanted to do. Luckily all it wanted to do was equivalent to: "I'm going to make you fix this undefined behavior by inserting an illegal opcode."

Changing the return value of those functions to void allows a clang++ compiled Uzem to successfully play ROMs that use the SD card, and this allows Emscripten builds to play ROMs that use the SD card as well! :D

We really should just enable every warning possible and fix anything that generates a warning. Uze, if I do that, would you accept the PR?

Edit: I fixed most of the warnings, and sent a PR.
User avatar
uze6666
Site Admin
Posts: 4801
Joined: Tue Aug 12, 2008 9:13 pm
Location: Montreal, Canada
Contact:

Re: Broken SD emulation in 140

Post by uze6666 »

Great find, thanks for fixing this. I have applied your 3 pull request.
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: Broken SD emulation in 140

Post by Artcfox »

uze6666 wrote:Great find, thanks for fixing this. I have applied your 3 pull request.
Awesome, thank you!

I ended up compiling the latest and greatest Clang/LLVM from source code last night, and playing around with all the different sanitizers it has to offer. Right away it started finding things, which is pretty amazing. I'm definitely going to be using Clang/LLVM in combination with the sanitizers in the future to help me track down bugs in any C/C++ project faster, even if the project ultimately gets compiled with gcc.
Post Reply