PVS-Studio and 3DO emulators

Renounce no longer concern emulation 3DO console, I confess. But then I had the opportunity to work with such an exotic thing as a static code analyzer, namely, PVS-Studio . The first thing I decided to try out the analyzer of course became my 3DO console emulator (Phoenix Project). In the beginning of the 90s there was such a prefix, the first 32-bit console with a CD-drive, I remember my brother and I brought her father from Moscow, since then I can not tear myself away. Well, once the case has turned up, then for one and all the main projects for 3DO emulation we will check. So let's go ...

FreeDOCore - the original kernel from the repository in GoogleCode.

Project website : http://www.freedo.org/ .



Audit : 8.

')

Help : the first and you can say the only emulator of this console, all the rest one way or another based on this code.



Write error



V523: The 'then' statement is equivalent to the 'else' statement.



Line 673 - clio.cpp







The usual compiler, even for Warning, does not consider such charm:

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, not only an extra code, but also an emulation error, XBUS is bidirectional, but in this case it always works on reading, which of course is not critical for emulating a CD-ROM drive, but it is still a gross and potentially dangerous error for emulated games. - what if some of them would burn a pig in the head ?! But seriously, instead of writing to the emulated XBUS interface, the memory specified in the DMA registers will be corrupted, and of course with such an approach it is impossible to emulate such a rarity as the FZ-EM256 3DO Memory Unit.



Read error



V614: Potentially uninitialized variable 'val' used.



Line 803 - clio.cpp







At first I thought it was nonsense, but suddenly I remembered the ghosts in the FIFO ...

 unsigned short __fastcall _clio_EIFIFO(unsigned short channel) { unsigned int val,base,mask; ... if(FIFOI[channel].StartAdr!=0)//channel enabled { ... } return val; } 

Here in some cases it is possible to read unpredictable values, since the variable val is initialized only when the condition is met. In theory, the FIFO of the DSP processor after emptying should return the last value read from it, such a ghost. And although in practice reading from an empty FIFO should not occur, but who knows, suddenly after correction another game will start?



Total two noteworthy mistakes, to be honest, I thought there would be more.

FourDO - modified kernel from SourceForge repository

Project website : http://www.fourdo.com/ .



Audit : 387.



Help : This project has lived two lives, the first - when the authors began to write their own emulator from scratch, but alas, the patient fell into a coma, and after opening the source codes of FreeDO, a second life began, already with new organs. So, let's see how implants get accustomed ...



Corrected, but still bug.







I just want to point out the latest error in the original kernel (V614: Potentially uninitialized variable 'val' used. Line 803 - clio.cpp), with the ghosts there they did it without any extra (or with too much?) Conversations:

 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; } 

And in vain they did away with it! The true problem remained unresolved, but from the outside everything was beautiful, and no one could remember the problem anymore. The most elegant solution would be to declare the variable val as static and initialize to zero, and more correct to take it out of the function and put it into the list of variables for fast saves, and the else block is to remove so as not to confuse.



Uncorrected error.



V523: The 'then' statement is equivalent to the 'else' statement.



Line 673 - clio.cpp







The “Creator” foot has not stepped here - everything is as in the original. For those who are not in the subject, the “Creator” is one of the FourDO authors - Victor (I don’t know if this is his name or not, he is still Stirlitz), he is also the author of 3DOPlay, another FreeDO fork, the same errors in it as in the original. There was a funny story with 3DOPlay: Victor decided to make a hit and said that he was the Creator of the 3DO Emulator, and the FreeDO developers allegedly stole the code from him. To my great shame, I, as a FreeDO co-author, could not pass by and took an active part in the hostilities against his 3DOPlay project - I must say a great name, but someone blurted out - three hollows and rushed ... As a result, Victor joined the FourDO team, and we must pay tribute, he is the only one who has at least done something in terms of the development of 3DO emulation, besides the authors of the original source.



Not a mistake yet.



V550: An odd comparison: Rez2T! = 0. This is a better way to use precision: fabs (A - B)> Epsilon.



Line 778 - madam.cpp







The code below is fully functional, but is of serious concern.

 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 had an int type, probably the authors thus decided to get rid of type conversion warnings, got rid, but if someone suddenly decides to remove a forced cast to a signed int type, there will be a risk to catch the coprocessor exception by dividing Nfrac16 by Rez2T .



And another piece of code causes my serious concern this time to the health of the developers from the FourDO team:

 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())); } 

From the analyzer’s point of view, the code given is correct, but from the point of view of common sense, I can only guess at “this” when taking into account the cycles of the emulated processor - a disgrace, below is the code from the original:

 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())); } 

In general, it can be said that the patient is neither alive nor dead, there are few changes in the emulator core, not everything is for the better, and there are no more than a year already, judging by the repository.

Phoenix Emu-Project - new kernel - new bugs.

Project website : http://www.arts-union.ru



Version : 1.7



Reference : re-written 3DO emulator, with the maniacal goal of bringing 3DO emulation to perfection, although I thought of it as a multisystem emulator with the appropriate code infrastructure, but so far only 3DO.



Error - texture peeled off!



V501: There are identical sub-expressions to the left and 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; } }; 

Mistakes made by inattention and leading to texture defects during the game, because if the CEL flags of the textures in the cache are different, and this goes unnoticed, and otherwise they are the same, the wrong shader will be used to display the texture. Below is the correct option:

 if(val.flags!=flags)return false; 

Error - trash on the screen!



It is a V579: It is possibly a mistake. Inspect the third argument.



Line 36 - vdlp_3do.cpp



Everything is simple here - the VDLP fell victim to inattention again when modifying the associated with the addition of support for PAL games. Previously, the emulator supported only NTSC games, of which the overwhelming majority, and the frame buffer had a fixed size of 320 by 240 pixels, therefore it was declared inside the class as an array, without memory allocations.

 screen=new uint8[384*288*3*4]; memset(screen,0,sizeof(screen)); 

And so that the error goes out of sight (literally, because the first, barely perceptible frame at the start of the game is filled with garbage), you can write, for example, like this:

 memset(screen,0,sizeof(uint8)*384*288*3*4); 

Error - and there is no disk!



V595: The 'adapt' pointer was used before it was verified against nullptr. Check lines: 375, 376.



Line 375 - dumplibrary.cpp



And again carelessness ... Before addressing the object, it would be necessary to check its correctness, therefore, the last two lines should be swapped. Otherwise, in the absence of the necessary images, we get an exception in the process of loading saved games.

 dumpAdapter *adapt=createDumpAdapter(j, inf->Parent()->Attribute("attach").toString()); adapt->SetSign(signs[names[i]]); if(!adapt)break; 

What is there to say? I need to be more careful or just relax in the evenings instead of programming emulators.

Conclusion

So, my first experience showed that static code analysis, a thing that is undoubtedly useful and can save a lot of time and nerves of developers. But for an emu scene, the issue price is certainly high, as is the case with the Hex-Ray decompiler for ARM, which could move the 3DO emulation many steps ahead.