Главная
>
Публикации
>
C++
>
PVS-Studio и 3DO-эмуляторы

Максим Гришин
Статей: 1

PVS-Studio и 3DO-эмуляторы

Зарекался больше не касаться эмуляции 3DO консоли, каюсь. Но тут у меня появилась возможность поработать с такой экзотической штукой как статический анализатор кода, а именно PVS-Studio. Первое на чем я решил опробовать анализатор конечно же стал мой эмулятор консоли 3DO (Phoenix Project). В начале 90-х была такая приставка, первая 32-х битная консоль с CD-приводом, помню нам с братом ее отец из Москвы привез, с тех пор никак оторваться не могу :-). Ну а раз подвернулся случай, то за одно и все основные проекты по эмуляции 3DO проверим. Итак, поехали...

FreeDOCore – оригинальное ядро из репозитория в GoogleCode

Сайт проекта: http://www.freedo.org/.

Ревизия: 8.

Справка: первый и можно сказать единственный эмулятор данной консоли, все остальные так или иначе основаны на данном коде.

Ошибка записи.

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

Line 673 - clio.cpp

Такую прелесть обычный компилятор даже за 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;
}

В данном случае не только лишний код, но и ошибка эмуляции, протокол XBUS двунаправленный, а в данном случае он работает всегда на чтение, что конечно не критично для эмуляции привода компакт-дисков, но все же является грубой и потенциально опасной ошибкой для эмулируемых игр – а вдруг какой из них взбредет в голову прожечь болванку?! А если серьезно, то вместо записи в эмулируемый интерфейс XBUS произойдет порча памяти, указанной в DMA регистрах, ну и конечно с таким подходом никак не получится сэмулировать такой раритет, как FZ-EM256 3DO Memory Unit.

Ошибка чтения.

V614: Potentially uninitialized variable 'val' used.

Line 803 - clio.cpp

Сначала я подумал, что это ерунда, но вдруг вспомнил о привидениях в FIFO...

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

Здесь в ряде случаев возможно чтение непредсказуемых значений, поскольку переменная val инициализируется только при выполнении условия. В теории FIFO DSP процессора после опустошения должно возвращать последнее считанное из него значение, эдакого призрака. И хотя на практике чтения из пустого FIFO происходить не должны, но кто знает, вдруг после исправления заведется еще одна игра?

Итого две достойных внимания ошибки, честно говоря, думал будет больше.

FourDO – модифицированное ядро из репозитория на SourceForge

Сайт проекта: http://www.fourdo.com/.

Ревизия: 387.

Справка: Данный проект прожил две жизни, первая – когда авторы начали самостоятельно писать эмулятор с нуля, но увы пациент впал в кому, а после вскрытия исходных кодов FreeDO, началась вторая жизнь, уже с новыми органами. Итак, посмотрим, как приживаются имплантаты...

Исправленная, но все еще ошибка.

Сразу хочу отметить последнюю ошибку в оригинальном ядре (V614: Potentially uninitialized variable 'val' used. Line 803 - clio.cpp), с привидениями там расправились без лишних (или таки с лишними?) разговоров:

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

А зря расправились! Истинная проблема осталась нерешенной, зато со стороны все красиво, и о проблеме никто бы мог больше и не вспомнить. Наиболее элегантным решением было бы объявить переменную val как static и инициализировать нулем, а более правильным – вынести за пределы функции и внести в список переменных для быстрых сохранений, ну а блок else – удалить, чтобы не смущал.

Неисправленная ошибка.

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

Line 673 - clio.cpp

Здесь не ступала нога "Создателя" – все как в оригинале. Для тех, кто не в теме, "Создатель" - это один из авторов FourDO – Виктор (уж не знаю, его это имя или нет, тот еще Штирлиц ), он же автор 3DOPlay, еще одного форка FreeDO, ошибки в нем те же что и в оригинале. С 3DOPlay была забавная история: Виктор решил потроллить и сказал, что он Создатель Эмулятора 3DO, а разработчики FreeDO якобы украли у него код. К великому моему стыду, я как соавтор FreeDO, не смог пройти мимо и принимал активное участие в боевых действиях против его проекта 3DOPlay - отличное надо сказать название, но кто-то ляпнул - три дупла и понеслась... В итоге Виктор перешел в команду FourDO, и надо отдать должное, он единственный, кто хоть что-то сделал в плане развития эмуляции 3DO, помимо авторов первоисточника.

Пока еще не ошибка.

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

Приведенный ниже код полностью работоспособен, но вызывает серьезную обеспокоенность.

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;

В оригинальном проекте Rez2T имел тип int, вероятно авторы таким образом решили избавиться от предупреждений по преобразованию типов, избавились, но если вдруг кто-то решит убрать принудительное приведение к типу signed int – появится риск словить исключение сопроцессора при делении Nfrac16 на Rez2T.

И еще один кусочек кода вызывает у меня серьезную обеспокоенность на этот раз здоровьем разработчиков из команды FourDO:

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

Приведенный код с точки зрения анализатора правильный, но с точки зрения здравого смысла, делать это (что "это" я могу лишь догадываться) при учете тактов эмулируемого процессора – форменное безобразие, ниже код из оригинала:

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

В целом можно сказать, что пациент ни жив, ни мертв, изменений в ядре эмулятора мало, не все в лучшую сторону, да и нет их уже более года, судя по репозиторию.

Phoenix Emu-Project – новому ядру – новые ошибки

Сайт проекта: http://www.arts-union.ru

Версия: 1.7

Справка: заново написанный эмулятор 3DO, с маниакальной целью доведения эмуляции 3DO до совершенства, хотя задумывался мной как мультисистемный эмулятор с соответствующей инфраструктурой кода, но пока только 3DO.

Ошибка – текстуры облезли!

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

Ошибка, допущенная по невнимательности и приводящая к текстурным дефектам в процессе игры, поскольку если флаги CEL у текстур в кэше отличаются, а это останется незамеченным, и в остальном они одинаковы, то будет использован неправильный шейдер для отображения текстуры. Ниже правильный вариант:

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

Ошибка - мусор на экране!

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

Тут все просто – VDLP пал жертвой опять же невнимательности при модификации связанной с добавлением поддержки игр формата PAL. Раньше эмулятор поддерживал только NTSC игры, которых подавляющее большинство, и буфер кадра имел фиксированный размер 320 на 240 пикселей, поэтому был объявлен внутри класса в виде массива, без выделений памяти.


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

И чтобы ошибка ушла с глаз долой (в буквальном смысле, ибо первый, едва уловимый кадр при старте игры заполнен мусором), можно написать, например, так:

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

Ошибка – а диска-то нет!

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

Line 375 - dumplibrary.cpp

И снова невнимательность... До обращения к объекту надо бы проверить его корректность, поэтому последние две строчки надо поменять местами. Иначе при отсутствии нужных образов получим исключение в процессе загрузки сохраненных игр.


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

Что тут сказать? Мне надо быть внимательней или просто отдыхать по вечерам вместо программирования эмуляторов :-).

Заключение

Итак, мой первый опыт показал, что статический анализ кода, вещь несомненно полезная и способная сэкономить немало времени и нервов разработчиков. Но для эму-сцены цена вопроса конечно высока, как и в случае с декомпилятором Hex-Ray для ARM, который мог бы продвинуть эмуляцию 3DO на много шагов вперед.