I know I promised not to touch upon the topic of 3DO console emulators anymore - well, sorry for breaking that promise. You see, I've recently had an opportunity to try such an exotic thing as a static code analyzer - PVS-Studio, to be exact. The first project I decided to try it on was, naturally, my 3DO console emulator (Phoenix Project). It was the first 32-bit console with a CD drive, dating back to the beginning of the 90-s. Dad bought it in Moscow as a present for me and my brother - and I've been fond of it since then :-). And since I've got such an opportunity, why not check all the other 3DO emulators too? Here we go...
Project website: http://www.freedo.org/.
Revision: 8.
Note: it is the first and actually the only emulator of this console because all the others make use of its code one way or another.
Writing error.
V523: The 'then' statement is equivalent to the 'else' statement.
Line 673 - clio.cpp
An ordinary compiler doesn't treat this lovely error even as a Warning:
if((cregs[0x404])&0x200)
{
ptr=0;
while(len>=0)
{
b3=_xbus_GetDataFIFO();
...
}
cregs[0x400]|=0x80;
}
else
{
ptr=0;
while(len>=0)
{
b3=_xbus_GetDataFIFO();
...
}
cregs[0x400]|=0x80;
}In this case, it's not only excess code but an emulation error as well; the XBUS protocol is two-party, but in this particular case it always performs reading only, which is not a critical issue for CD drive emulation, of course, but still a crude and potentially dangerous error for emulated games - what if one of them decides to burn through a CD?! Joking apart, this code will spoil the memory area specified in the DMA registers instead of writing data into the emulated XBUS interface. And it surely won't let you emulate such a rarity as FZ-EM256 3DO Memory Unit.
Reading error.
V614: Potentially uninitialized variable 'val' used.
Line 803 - clio.cpp
At first I thought it to be an insignificant defect, but then I recalled the ghosts in FIFO...
unsigned short __fastcall _clio_EIFIFO(unsigned short channel)
{
unsigned int val,base,mask;
...
if(FIFOI[channel].StartAdr!=0)//channel enabled
{
...
}
return val;
}This code may cause reading some unpredictable values in certain cases as the val variable is initialized only when executing the condition. Theoretically, the DSP's FIFO must return the last value, kind of a ghost, read from it when cleared. In practice, the program can't read from empty FIFO, but who knows - what if a new game will be born after fixing?
Thus, we have two errors worthy of attention. Honestly, I thought there would be more.
Project website: http://www.fourdo.com/.
Revision: 387.
Note: This project has lived two lives: the first was when the authors had started writing the emulator from scratch on their own, but the patient unfortunately fell into a coma; and it was after the autopsy of FreeDO's source codes that the project started its second life with new organs. Let's see how well it gets along with implants...
A fixed error which is still an error.
I want to pass right on to the last discussed error in the original core (V614: Potentially uninitialized variable 'val' used. Line 803 - clio.cpp): the programmers have exorcised that ghost without much talking (or perhaps with quite a lot of?):
unsigned short __fastcall _clio_EIFIFO(unsigned short channel)
{
unsigned int val,base,mask;
base=0x400+(channel*16);
mask=1<<channel;
if(FIFOI[channel].StartAdr!=0)
{
...
}
else
{
val=0;
// JMK SEZ: What is this?
// It was commented out along with this whole "else" block,
// but I had to bring this else block back from the dead
// in order to initialize val appropriately.
}
return val;
}Well, they shouldn't have done that! The underlying problem was left unsolved but given a neat look so that nobody would have ever recalled it. The smartest solution would be to declare the val variable as static and initialize it to zero; the most correct solution would be to take it out of the function bounds and add into the list of variables for quick save - well, and delete the else block so that it wouldn't embarrass anyone.
Unfixed error.
V523: The 'then' statement is equivalent to the 'else' statement.
Line 673 - clio.cpp
The foot of "the Creator" has never stepped here - just like in the original. A note for those out of the swim: "the Creator" is one of the FourDO's authors, Victor (I don't know for sure if this is his true name or not; he's quite a bit of a Stierlitz ); he is also the author of 3DOPlay, another FreeDO's fork with the same errors inherited from the original project. There was a funny story about 3DOPlay: Victor decided to troll the community and said he was the Creator of the 3DO Emulator and FreeDO's developers had stolen his code. Shame on me, I, as a co-author of FreeDO, couldn't avoid getting involved into the conflict and was actively fighting against his project 3DOPlay. By the way, this is truly a very nice name for a project, but someone had come up with the nickname "three holes" (a literal translation of the Russian phrase "tri dupla" sounding similar to "3DOPlay", which makes a pun - a comment by translator) and thus the fight had begun... The story ended up with Victor moving to the FourDO team, and he actually deserves a highest praise as he was the only person who contributed to 3DO emulation development besides the authors of the original project.
Not an error yet.
V550: An odd precise comparison: Rez2T != 0. It's probably better to use a comparison with defined precision: fabs(A - B) > Epsilon.
Line 778 - madam.cpp
The code below works absolutely well, but it does make me worry quite a bit.
static double Rez0T,Rez1T,Rez2T,Rez3T;
...
Rez2T=(signed int)((M20*V0+M21*V1+M22*V2)/65536.0);
if(Rez2T!=0) M=Nfrac16/(double)Rez2T;
else M=Nfrac16;In the original project, Rez2T used to have the int type. I guess the authors must have tried to get rid of the type conversion warnings that way, and they did it alright, but if someone decides to remove the forced conversion to the signed int type, there'll be a potential danger of getting an exception from the co-processor when dividing Nfrac16 by Rez2T.
And here is another piece of code that makes me worry about the health of the FourDO team's members:
void __fastcall _qrz_PushARMCycles(unsigned int clks)
{
uint32 arm,cnt;
int timers=21000000; //default
int sp=0;
if(sdf>0) sdf--;
if(sf>0) sf--;
if(unknownflag11>0)unknownflag11--;
if(ARM_CLOCK<0x5F5E10)ARM_CLOCK=0x5F5E10;
if(ARM_CLOCK>0x2FAF080)ARM_CLOCK=0x2FAF080;
if(speedfixes>0&&speedfixes<0x186A1)
{/*sp=0x2DC6C0;*/ speedfixes--;}
else if(speedfixes>0x186A1&&speedfixes<0x30D41)
{/*if(sdf==0)sp=0x4C4B40; */speedfixes--;}
else if(speedfixes<0) {sp=0x3D0900; speedfixes++;}
else if(speedfixes>0x30D41) {/*sp=0x249F00;*/ speedfixes--;}
else if(speedfixes==0x30D41||speedfixes==0x186A1) speedfixes=0;
if((fixmode&FIX_BIT_TIMING_2)&&sf<=2500000)
{sp=0; timers=21000000; if(sf==0)sp=-(0x1C9C380-ARM_CLOCK);}
if((fixmode&FIX_BIT_TIMING_1)/*&&jw>0*/&&sf<=1500000)
{/*jw--;*/timers=1000000;sp=-1000000;}
if((fixmode&FIX_BIT_TIMING_4)/*&&jw>0*/)
{/*jw--;*/timers=1000000;sp=-1000000;}
if((fixmode&FIX_BIT_TIMING_3)&&(sf>0&&sf<=100000)/*&&jw>0*/)
{/*jw--;*/timers=900000;}
if((fixmode&FIX_BIT_TIMING_5)&&sf==0/*&&jw>0*/)
{/*jw--;*/timers=1000000;}
if((fixmode&FIX_BIT_TIMING_6)/*&&jw>0*/)
{/*jw--;*/timers=1000000; if(sf<=80000)sp=-23000000;}
if(fixmode&FIX_BIT_TIMING_7){sp=-3000000; timers=21000000;}
if((sf>0x186A0&&!(fixmode&FIX_BIT_TIMING_2))||
((fixmode&FIX_BIT_TIMING_2)&&sf>2500000))
sp=-(12200000-ARM_CLOCK);
if((ARM_CLOCK-sp)<0x2DC6C0)sp=-(0x2DC6C0-ARM_CLOCK);
if((ARM_CLOCK-sp)!=THE_ARM_CLOCK)
{ THE_ARM_CLOCK=(ARM_CLOCK-sp);
io_interface(EXT_ARM_SYNC,(void*)THE_ARM_CLOCK);
//fix for working with 4do
}
arm=(clks<<24)/(ARM_CLOCK-sp);
qrz_AccARM+=arm*(ARM_CLOCK-sp);
if( (qrz_AccARM>>24) != clks )
{
arm++;
qrz_AccARM+=ARM_CLOCK;
qrz_AccARM&=0xffffff;
}
qrz_AccDSP+=arm*SND_CLOCK;
qrz_AccVDL+=arm*(VDL_CLOCK);
if(_clio_GetTimerDelay())qrz_TCount+=arm*((timers)/
(_clio_GetTimerDelay()));
}The analyzer finds this code correct, but common sense tells us it is just awfully bad to do that (what exactly "that", I can only guess), taking into account the emulated processor's clock rate. Here is the original code sample:
void __fastcall _qrz_PushARMCycles(unsigned int clks)
{
uint32 arm;
arm=(clks<<24)/ARM_CLOCK;
qrz_AccARM+=arm*ARM_CLOCK;
if( (qrz_AccARM>>24) != clks )
{
arm++;
qrz_AccARM+=ARM_CLOCK;
qrz_AccARM&=0xffffff;
}
qrz_AccDSP+=arm*SND_CLOCK;
qrz_AccVDL+=arm*(VDL_CLOCK);
if(_clio_GetTimerDelay())
qrz_TCount+=arm*((__temporalfixes?12500000:25000000)/
(_clio_GetTimerDelay()));
}Speaking generally, the patient is neither dead nor alive; there are few changes in the emulator's core, not all of them to the best; and they are all more than a year old according to the repository.
Project website: http://www.arts-union.ru [RU]
Version: 1.7
Note: this is a 3DO emulator written from scratch with a maniacal purpose of making 3DO emulation as perfect as possible, though I actually meant it to be a multi-system emulator with the corresponding code infrastructure. Well, here's only 3DO for now.
Error: stripped textures!
V501: There are identical sub-expressions to the left and to the right of the '!=' operator: val.flags != val.flags.
Line 207 - gfx_celengine.h
struct gfxCelTexturePDECAttrib
{
uint32 pre0;
uint32 flags;
int plutcount;
uint16 plut[32];
bool operator==(const gfxCelTexturePDECAttrib &val) const
{
if(val.pre0!=pre0)return false;
if(val.flags!=val.flags)return false;
if(val.plutcount!=plutcount)return false;
for(int i=0;i<val.plutcount;i++)
{
if(val.plut[i]!=plut[i])return false;
}
return true;
}
};This is an error made from inattention and leading to defects in textures while playing. The reason is that when textures stored in the cache are only different in their CEL flags but identical in all the other parameters, this subtle difference may stay unnoticed, thus leading to using an incorrect shader for texture representation. The correct code should look like this:
if(val.flags!=flags)return false;Error: garbage on the screen!
V579: The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument.
Line 36 - vdlp_3do.cpp
This issue is very simple: VDLP fell a victim of inattention (again) during code modification about adding support for PAL games. The emulator had used to support NTSC games only, which is the most widely spread format, and the frame buffer had used to have a fixed size of 320 x 240 pixels - that's why it was declared inside a class as an array, without memory allocation.
screen=new uint8[384*288*3*4];
memset(screen,0,sizeof(screen));To drive the error out of sight (I mean literally - because the first frame which is hardly noticeable is filled with garbage when the game starts), we can use the following code:
memset(screen,0,sizeof(uint8)*384*288*3*4);Error: no CD found!
V595: The 'adapt' pointer was utilized before it was verified against nullptr. Check lines: 375, 376.
Line 375 - dumplibrary.cpp
My inattention again... Before addressing an object, I should have checked if it was correct, so the last two lines should be swapped. Otherwise, we'll get an exception when trying to load a saved game, with the required images absent.
dumpAdapter *adapt=createDumpAdapter(j,
inf->Parent()->Attribute("attach").toString());
adapt->SetSign(signs[names[i]]);
if(!adapt)break;What to say about all this? I ought to be more attentive or simply have enough rest in the evenings instead of programming emulators :-).
So, my first experience has shown that static code analysis is obviously a useful technology that can help developers save quite a lot of time and nerves. Of course, the price is pretty high for the emulators' area - just like it is for the Hex-Ray decompiler for ARM, that could push 3DO emulation much farther.