# Проверяем эмулятор GPCS4, или сможем ли когда\-нибудь поиграть в "Bloodborne" на PC

Эмулятор – это приложение, способное имитировать запуск программы, предназначенной для одной платформы, на другой\. Примером эмулятора является GPCS4, предназначенный для запуска игр для PS4 на PC\. Недавно состоялся первый релиз GPCS4, и мы решили проверить этот проект\. Давайте посмотрим, какие ошибки удалось найти PVS\-Studio в исходном коде этого эмулятора\.

![0955_GPCS4_ru/image1.png](https://import.viva64.com/docx/blog/0955_GPCS4_ru/image1.png)

## О проекте

GPCS4 – это эмулятор PlayStation 4, написанный на C и C\+\+\.

Изначально целью автора проекта было исследовать архитектуру PS4\. Однако проект быстро развивался, и уже в начале 2020 года разработчикам GPCS4 удалось запустить на своём эмуляторе игру [We are Doomed](https://habr.com/ru/news/t/487050/)\. Это был первый успешный стабильный запуск на PC игры для PS4\. Впрочем, сам игровой процесс пока что был далёк от идеала: игра тормозила, картинка дёргалась\. Тем не менее, [основной разработчик проекта](https://github.com/Inori/) полон энтузиазма и продолжает совершенствовать эмулятор\.

В конце апреля 2022 года состоялся [первый релиз GPCS4](https://github.com/Inori/GPCS4/releases/tag/v0.1.0)\. Я скачал и проверил версию 0\.1\.0 проекта\. К слову, на момент публикации статьи уже вышел релиз 0\.2\.1 – проект развивается очень быстро\. Давайте перейдём к ошибкам и недочётам, которые удалось найти анализатору PVS\-Studio в первом релизе проекта GPCS4\.

## Потерянный break

[V796](https://pvs-studio.ru/ru/docs/warnings/v796/) \[CWE\-484\] It is possible that 'break' statement is missing in switch statement\. AudioOut\.cpp 137

```cpp
static AudioProperties getAudioProperties(uint32_t param)
{
  uint32_t format       = param & 0x000000ff;
  AudioProperties props = {};

  switch (format)
  {
    // ....
    case SCE_AUDIO_OUT_PARAM_FORMAT_S16_8CH_STD:
    {
      props.nChannels   = 8;
      props.bytesPerSample  = 2;
      props.audioFormat = RTAUDIO_FORMAT_SINT16;
      break;
    }
    case SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_MONO:
    {
      props.nChannels   = 1;
      props.bytesPerSample  = 4;
      props.audioFormat = RTAUDIO_FORMAT_FLOAT32;         // <=
    }
    case SCE_AUDIO_OUT_PARAM_FORMAT_FLOAT_STEREO:
    {
      props.nChannels   = 2;
      props.bytesPerSample  = 4;
      props.audioFormat = RTAUDIO_FORMAT_FLOAT32;
      break;
    }
  }
  return props;
}
```

В данном фрагменте кода для случая _SCE\_AUDIO\_OUT\_PARAM\_FORMAT\_FLOAT\_MONO_ потерян _break_\. Из\-за этого количество каналов будет выставлено неправильно\.

## Проверка указателя после использования

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'm\_moduleData' pointer was utilized before it was verified against nullptr\. Check lines: 49, 53\. ELFMapper\.cpp 49

```cpp
struct NativeModule { /*....*/ };

class ELFMapper
{
  // ....
  NativeModule *m_moduleData;
};

bool ELFMapper::validateHeader()
{
  bool retVal      = false;
  auto &fileMemory = m_moduleData->m_fileMemory;
  do
  {
    if (m_moduleData == nullptr)
    {
      LOG_ERR("file has not been loaded");
      break;
    }
    // ....
  } while (false);
  
  return retVal;
}
```

В приведённом фрагменте указатель _m\_moduleData_ сначала разыменовывается, а затем в цикле _do\-while_ сравнивается с _nullptr_\.

Внимательные читатели могут возразить: "Может быть, на вход функции всегда приходит валидный указатель? А потом в цикле _do\-while_ этот указатель модифицируется и может стать нулевым? Тогда и ошибки тут нет"\. В данном случае это не так\. Во\-первых, из\-за условия _while \(false\)_ выполнится ровно одна итерация цикла\. Во\-вторых, указатель _m\_moduleData_ [не модифицируется](https://github.com/Inori/GPCS4/blob/67442ae29df1916137a01b87b675375d8b48be0e/GPCS4/Loader/ELFMapper.cpp#L45-L88)\.

Другое возражение может заключаться в том, что взятие ссылки – это безопасно\. Ведь использоваться эта ссылка будет, только если указатель валидный\. Но нет, такой [код содержит неопределённое поведение](https://pvs-studio.ru/ru/blog/posts/cpp/0306/)\. Это ошибка\. Скорее всего, нужно сделать проверку указателя до его первого разыменования:

```cpp
bool ELFMapper::validateHeader()
{
  bool retVal      = false;
  
  do
  {
    if (m_moduleData == nullptr)
    {
      LOG_ERR("file has not been loaded");
      break;
    }

    auto &fileMemory = m_moduleData->m_fileMemory;
    // ....
  } while (false);

  return retVal;
}
```

## Повторное присвоение

[V519](https://pvs-studio.ru/ru/docs/warnings/v519/) \[CWE\-563\] The '\* memoryType' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 54, 55\. sce\_kernel\_memory\.cpp 55

```cpp
int PS4API sceKernelGetDirectMemoryType(sce_off_t start, int *memoryType, 
    sce_off_t *regionStartOut, sce_off_t *regionEndOut)
{
  LOG_SCE_DUMMY_IMPL();
  *memoryType = SCE_KERNEL_WB_GARLIC;
  *memoryType = SCE_KERNEL_WC_GARLIC;
  return SCE_OK;
}
```

Как можно догадаться по названию _LOG\_SCE\_DUMMY\_IMPL_, реализация метода _sceKernelGetDirectMemoryType_ ещё будет меняться\. И всё же, два присваивания по одному и тому же адресу _memoryType_ выглядят странно\. Возможно, так получилось в результате неудачного слияния кода\.

## Переполнение буфера

[V512](https://pvs-studio.ru/ru/docs/warnings/v512/) \[CWE\-119\] A call of the 'memset' function will lead to overflow of the buffer 'param\-\>reserved'\. sce\_gnm\_draw\.cpp 420

[V531](https://pvs-studio.ru/ru/docs/warnings/v531/) \[CWE\-131\] It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. sce\_gnm\_draw\.cpp 420

```cpp
struct GnmCmdPSShader
{
  uint32_t              opcode;
  gcn::PsStageRegisters psRegs;
  uint32_t              reserved[27];
};

int PS4API sceGnmSetPsShader350(uint32_t* cmdBuffer, uint32_t numDwords, 
                                const gcn::PsStageRegisters *psRegs)
{
  // ....
  memset(param->reserved, 0, sizeof(param->reserved) * sizeof(uint32_t)); 
  return SCE_OK;
}
```

Иногда бывает, что на одну и ту же строчку кода указывают сразу несколько диагностик PVS\-Studio\. Так получилось и в этом примере\. В данном фрагменте кода в функцию _memset_ передаётся неправильное значение третьим аргументом\. Выражение _sizeof\(param\-\>reserved\)_ уже вернёт размер массива _param\-\>reserved_\. Умножение на _sizeof\(uint32\_t\)_ увеличит это значение в 4 раза, и значение получится неправильным\. Поэтому в результате вызова _memset_ произойдёт выход за границу массива _param\-\>reserved_\. Нужно убрать лишнее умножение:

```cpp
int PS4API sceGnmSetPsShader350( /*....*/ )
{
  // ....
  memset(param->reserved, 0, sizeof(param->reserved));
  return SCE_OK;
}
```

Всего анализатор обнаружил 20 подобных переполнений, приведу ещё один пример:

[V512](https://pvs-studio.ru/ru/docs/warnings/v512/) \[CWE\-119\] A call of the 'memset' function will lead to overflow of the buffer 'initParam\-\>reserved'\. sce\_gnm\_dispatch\.cpp 16

```cpp
uint32_t PS4API sceGnmDispatchInitDefaultHardwareState(uint32_t* cmdBuffer,
                                                       uint32_t numDwords)
{
  // ....
  memset(initParam->reserved, 0,
         sizeof(initParam->reserved) * sizeof(uint32_t));
  return initCmdSize;
}
```

В этом фрагменте кода происходит выход за границу массива _initParam\-\>reserved_\.

## Учимся считать до семи, или ещё одно переполнение буфера

[V557](https://pvs-studio.ru/ru/docs/warnings/v557/) \[CWE\-787\] Array overrun is possible\. The 'dynamicStateCount \+\+' index is pointing beyond array bound\. VltGraphics\.cpp 157

```cpp
VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const
{
  // ....
  std::array<VkDynamicState, 6> dynamicStates;
  uint32_t                      dynamicStateCount = 0;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_VIEWPORT;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_SCISSOR;
  if (state.useDynamicDepthBias())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BIAS;
  if (state.useDynamicDepthBounds())
  {
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS;
    dynamicStates[dynamicStateCount++] =
                             VK_DYNAMIC_STATE_DEPTH_BOUNDS_TEST_ENABLE;
  }
  if (state.useDynamicBlendConstants())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_BLEND_CONSTANTS;
  if (state.useDynamicStencilRef())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_STENCIL_REFERENCE;
  // ....
}
```

Анализатор предупреждает, что может произойти переполнение массива _dynamicStates_\. В данном фрагменте кода есть 4 проверки:

* if \(state\.useDynamicDepthBias\(\)\)
* if \(state\.useDynamicDepthBounds\(\)\)
* if \(state\.useDynamicBlendConstants\(\)\)
* if \(state\.useDynamicStencilRef\(\)\)

Каждая из них – это проверка одного из независимых флагов\. Например, проверка _if \(state\.useDynamicDepthBias\(\)\)_:

```cpp
bool useDynamicDepthBias() const
{
  return rs.depthBiasEnable();
}

VkBool32 depthBiasEnable() const
{
  return VkBool32(m_depthBiasEnable);
}
```

Получается, все эти 4 проверки могут быть истинными одновременно\. Тогда выполнится 7 строк вида _'dynamicStates\[dynamicStateCount\+\+\] \= \.\.\.\.'_\. На седьмой такой строке произойдёт обращение к _dynamicStates\[6\]_\. Это выход за границу массива\.

Для исправления нужно выделить память на 7 элементов:

```cpp
VkPipeline VltGraphicsPipeline::createPipeline(/* .... */) const
{
  // ....
  std::array<VkDynamicState, 7> dynamicStates; // <=
  uint32_t                      dynamicStateCount = 0;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_VIEWPORT;
  dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_SCISSOR;
  if (state.useDynamicDepthBias())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BIAS;
  if (state.useDynamicDepthBounds())
  {
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_DEPTH_BOUNDS;
    dynamicStates[dynamicStateCount++] =
                             VK_DYNAMIC_STATE_DEPTH_BOUNDS_TEST_ENABLE;
  }
  if (state.useDynamicBlendConstants())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_BLEND_CONSTANTS;
  if (state.useDynamicStencilRef())
    dynamicStates[dynamicStateCount++] = VK_DYNAMIC_STATE_STENCIL_REFERENCE;
  // ....
}
```

## Неправильное использование флага

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) \[CWE\-570\] Expression 'nOldFlag & VMPF\_NOACCESS' is always false\. PlatMemory\.cpp 22

```cpp
#define PAGE_NOACCESS           0x01
#define PAGE_READONLY           0x02
#define PAGE_READWRITE          0x04
#define PAGE_EXECUTE            0x10
#define PAGE_EXECUTE_READ       0x20
#define PAGE_EXECUTE_READWRITE  0x40

enum VM_PROTECT_FLAG
{
  VMPF_NOACCESS  = 0x00000000,
  VMPF_CPU_READ  = 0x00000001,
  VMPF_CPU_WRITE = 0x00000002,
  VMPF_CPU_EXEC  = 0x00000004,
  VMPF_CPU_RW    = VMPF_CPU_READ | VMPF_CPU_WRITE,
  VMPF_CPU_RWX   = VMPF_CPU_READ | VMPF_CPU_WRITE | VMPF_CPU_EXEC,
};

inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  uint32_t nNewFlag = 0;
  do
  {
    if (nOldFlag & VMPF_NOACCESS)
    {
      nNewFlag = PAGE_NOACCESS;
      break;
    }

    if (nOldFlag & VMPF_CPU_READ)
    {
      nNewFlag = PAGE_READONLY;
    }

    if (nOldFlag & VMPF_CPU_WRITE)
    {
      nNewFlag = PAGE_READWRITE;
    }

    if (nOldFlag & VMPF_CPU_EXEC)
    {
      nNewFlag = PAGE_EXECUTE_READWRITE;
    }

  } while (false);
  return nNewFlag;
}
```

Функция _GetProtectFlag_ конвертирует флаг с правами доступа к файлу из одного формата в другой\. Однако делает это некорректно\. Программист не учёл, что значение _VMPF\_NOACCESS_ равно нулю\. Из\-за этого условие _if \(nOldFlag & VMPF\_NOACCESS\)_ всегда ложное и функция никогда не вернёт значение _PAGE\_NOACCESS_\.

Кроме того, функция _GetProtectFlag_ неправильно конвертирует не только флаг _VMPF\_NOACCESS_, но и другие\. Например, флаг _VMPF\_CPU\_EXEC_ будет сконвертирован во флаг _PAGE\_EXECUTE\_READWRITE_\.

Когда я думал, как это исправить, первой мыслью было написать что\-то в таком роде:

```cpp
inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  uint32_t nNewFlag = PAGE_NOACCESS;
  if (nOldFlag & VMPF_CPU_READ)
  {
    nNewFlag |= PAGE_READ;
  }

  if (nOldFlag & VMPF_CPU_WRITE)
  {
    nNewFlag |= PAGE_WRITE;
  }

  if (nOldFlag & VMPF_CPU_EXEC)
  {
    nNewFlag |= PAGE_EXECUTE;
  }

  return nNewFlag;
}
```

Однако в данном случае такой подход не работает\. Дело в том, что _PAGE\_NOACCESS_, _PAGE\_READONLY_ и остальные – это Windows\-флаги со своей спецификой\. Например, среди них нет флага _PAGE\_WRITE_\. Считается, что если есть права на запись, то как минимум есть права еще и на чтение\. По тем же причинам нет флага _PAGE\_EXECUTE\_WRITE_\.

Кроме того, побитовое "ИЛИ" двух Windows\-флагов не даёт в результате флаг, соответствующий сумме прав: _PAGE\_READONLY \| PAGE\_EXECUTE \!\= PAGE\_EXECUTE\_READ_\. Поэтому нужно перебирать все возможные комбинации флагов:

```cpp
inline uint32_t GetProtectFlag(VM_PROTECT_FLAG nOldFlag)
{
  switch (nOldFlag)
  {
    case VMPF_NOACCESS:
      return PAGE_NOACCESS;
    case VMPF_CPU_READ:
      return PAGE_READONLY;
    case VMPF_CPU_WRITE: // same as ReadWrite
    case VMPF_CPU_RW:
      return PAGE_READWRITE;
    case VMPF_CPU_EXEC:
      return PAGE_EXECUTE;
    case VMPF_CPU_READ | VMPF_CPU_EXEC:
      return PAGE_EXECUTE_READ:
    case VMPF_CPU_WRITE | VMPF_CPU_EXEC: // same as ExecuteReadWrite
    case VMPF_CPU_RWX:
      return PAGE_EXECUTE_READWRITE;
    default:
      LOG("unknown PS4 flag");
      return PAGE_NOACCESS;
  }
}
```

## Лишняя проверка

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) \[CWE\-571\] Expression 'retAddress' is always true\. Memory\.cpp 373

```cpp
void* MemoryAllocator::allocateInternal(void* addrIn, size_t len,
                                        size_t alignment, int prot)
{
  // ....
  while (searchAddr < SCE_KERNEL_APP_MAP_AREA_END_ADDR)
    {
      // ....
      void* retAddress = VMAllocate(reinterpret_cast<void*>(regionAddress), len,
                                    plat::VMAT_RESERVE_COMMIT, uprot);
      if (!retAddress)
      {
        searchAddr = reinterpret_cast<size_t>(mi.pRegionStart) + mi.nRegionSize;
        continue;
      }
      // ....
      if (retAddress)
      {
        // unlikely
        plat::VMFree(retAddress);
      }
    // ....
    }
  // ....
}
```

В этом фрагменте кода указатель _retAddress_ проверяется дважды\. Сначала делается проверка _if \(\!retAddress\)_\. Если указатель нулевой, то выполнение перейдёт к следующей итерации цикла _while_\. Иначе указатель _retAddress_ ненулевой\. Поэтому вторая проверка _if \(retAddress\)_ всегда истинная, и её можно убрать\.

## Ещё одно всегда истинное условие

[V547](https://pvs-studio.ru/ru/docs/warnings/v547/) \[CWE\-571\] Expression 'pipeConfig \=\= kPipeConfigP16' is always true\. GnmDepthRenderTarget\.h 170

```cpp
uint8_t getZReadTileSwizzleMask(void) const
    {
      // From IDA
      auto pipeConfig = getPipeConfig();
      auto zfmt       = getZFormat();
      auto tileMode   = getTileMode();
      if (pipeConfig != kPipeConfigP16 ||     // <=
        zfmt == kZFormatInvalid ||
        !GpuAddress::isMacroTiled(tileMode))
      {
        return 0;
      }

      auto     dataFormat          = DataFormat::build(zfmt);
      auto     totalBitsPerElement = dataFormat.getTotalBitsPerElement();
      uint32_t numFragments          = 1 << getNumFragments();
      uint32_t shift               = 0;
      NumBanks numBanks            = {};
      if (pipeConfig == kPipeConfigP16)      // <=
      {
        GpuAddress::getAltNumBanks(&numBanks, tileMode,
                                   totalBitsPerElement, numFragments);
        shift = 4;
      }
      else
      {
        GpuAddress::getNumBanks(&numBanks, tileMode,
                                totalBitsPerElement, numFragments);
        shift = 3;
      }

      return (this->m_regs[2] & (((1 << (numBanks + 1)) - 1) << shift)) >> 4;
    }
```

В данном фрагменте кода анализатор нашёл всегда истинное условие _if \(pipeConfig \=\= kPipeConfigP16\)_\. Давайте разберёмся, почему это так\.

Если вызов функции _getPipeConfig_ вернул значение, не равное _kPipeConfigP16_, то первое условие будет верным и выполнение программы не достигнет проверки _if \(pipeConfig \=\= kPipeConfigP16\)_\.

Получается, что вторая проверка этой переменной либо не выполняется, либо всегда истинна\. Но не стоит спешить и убирать её\. Возможно, первое условие было добавлено временно и будет убрано в дальнейшем\.

## Ошибка копипасты

[V517](https://pvs-studio.ru/ru/docs/warnings/v517/) \[CWE\-570\] The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 469, 475\. GnmGpuAddress\.cpp 469

```cpp
int32_t sce::GpuAddress::adjustTileMode(/* .... */)
{
switch(microTileMode)
{
  case Gnm::kMicroTileModeThin:
    if      (newArrayMode == Gnm::kArrayMode3dTiledThick)
      *outTileMode = Gnm::kTileModeThick_3dThick;
    else if      (newArrayMode == Gnm::kArrayMode2dTiledThick)
      *outTileMode = Gnm::kTileModeThick_2dThick;
    else if (newArrayMode == Gnm::kArrayMode1dTiledThick)
      *outTileMode = Gnm::kTileModeThick_1dThick;
    else if (newArrayMode == Gnm::kArrayMode3dTiledThin)
      *outTileMode = Gnm::kTileModeThin_3dThin; // ....
    else if (newArrayMode == Gnm::kArrayMode3dTiledThinPrt)
      *outTileMode = Gnm::kTileModeThin_3dThinPrt; // ....
    else if (newArrayMode == Gnm::kArrayMode2dTiledThin)                  // <=
      *outTileMode = Gnm::kTileModeThin_2dThin; // ....
    else if (newArrayMode == Gnm::kArrayMode2dTiledThinPrt)
      *outTileMode = Gnm::kTileModeThin_2dThinPrt; // ....
    else if (newArrayMode == Gnm::kArrayModeTiledThinPrt)
      *outTileMode = Gnm::kTileModeThin_ThinPrt; // ....
    else if (newArrayMode == Gnm::kArrayMode2dTiledThin)                  // <=
      *outTileMode = Gnm::kTileModeThin_2dThin;
    else if (newArrayMode == Gnm::kArrayMode1dTiledThin)
      *outTileMode = Gnm::kTileModeThin_1dThin;
    else
      break;
    return kStatusSuccess;
  // ....
}
}
```

Не обошлось и без ошибок копипасты\. В данном фрагменте кода дважды написана одна и та же проверка _newArrayMode \=\= Gnm::kArrayMode2dTiledThin_\.

Сложно сказать, как именно нужно это поправить\. Скорее всего, вторая проверка должна быть несколько другой\. А может быть, она лишняя, и её можно убрать\.

## Почему лучше избегать сложных выражений?

[V732](https://pvs-studio.ru/ru/docs/warnings/v732/) \[CWE\-480\] Unary minus operator does not modify a bool type value\. Consider using the '\!' operator\. GnmRenderTarget\.h 237

```cpp
typedef enum RenderTargetChannelType
{
  kRenderTargetChannelTypeUNorm            = 0x00000000,
  kRenderTargetChannelTypeSNorm            = 0x00000001,
  kRenderTargetChannelTypeUInt             = 0x00000004,
  kRenderTargetChannelTypeSInt             = 0x00000005,
  kRenderTargetChannelTypeSrgb             = 0x00000006,
  kRenderTargetChannelTypeFloat            = 0x00000007,
} RenderTargetChannelType;

void setDataFormat(DataFormat format)
{
  // ....
  int v3;
  RenderTargetChannelType  type;  // [rsp+4h] [rbp-3Ch]
  __int64                  v9;  // [rsp+10h] [rbp-30h]
  bool typeConvertable = format.getRenderTargetChannelType(&type);
  v2 = type | kRenderTargetChannelTypeSNorm;
  v3  = (uint8_t) - (type < 7) & (uint8_t)(0x43u >> type) & 1; // <=
  // ....
}
```

Похоже, что программист ожидал следующего поведения при вычислении выражения:

* пусть переменная _type_ меньше _7_;
* тогда выражение _type < 7_ равно _true_;
* затем к _true_ применяется унарный минус, получается _\-1_;
* значение _\-1_ приводится к _unsigned char_, получается _0b1111'1111_\.

Однако на самом деле происходит следующее:

* пусть переменная _type_ меньше _7_;
* тогда выражение _type < 7_ равно _true_;
* затем к _true_ применяется унарный минус, получается _1_;
* значение _1_ приводится к _unsigned char_, получается _0b0000'0001_\.

Впрочем, следующая операция _& 1_ приводит к одному и тому же результату\. По этой счастливой случайности, код целиком работает так, как ожидает программист\. Тем не менее, стоит поправить этот код\. Попробуем понять, какое значение присваивается переменной _v3_ в зависимости от значения _type_\.

Первый случай: переменная _type_ больше или равна 7\.

* Тогда выражение _type < 7_ равно _false_\.
* К _false_ применяется унарный минус, получается _false_\.
* False приводится к unsigned char, получается _0b0000'0000_\.
* Побитовое "И" с 0 всегда даёт 0, поэтому в результате получаем 0\.

Второй случай: переменная _type_ меньше 7\.

* Как уже выяснили раньше, выражение _\(uint8\_t\) \- \(type < 7\)_ равно 1\.
* В данном случае есть смысл вычислять выражение _0x43u \>\> type_\.
* Для удобства запишем бинарное представление числа 0x43 \= 0b0100'0011\.
* Нас интересует только младший бит, потому что к результату выражения _0x43u \>\> type_ применится побитовое "И" с 1\.
* Если _type_ равен 0, 1 или 6, то младший бит будет равен 1, и результат всего выражения будет 1\. Во всех других случаях получится 0\.

Итого, если type равен 0, 1 или 6, то в переменную v3 будет записано значение 1, а во всех остальных случаях – значение 0\. Стоит заменить сложное выражение на более простое и понятное _\(type \=\= 0\) \|\| \(type \=\= 1\) \|\| \(type \=\= 6\)_\. Предлагаю следующий код:

```cpp
typedef enum RenderTargetChannelType
    {
      kRenderTargetChannelTypeUNorm            = 0x00000000,
      kRenderTargetChannelTypeSNorm            = 0x00000001,
      kRenderTargetChannelTypeUInt             = 0x00000004,
      kRenderTargetChannelTypeSInt             = 0x00000005,
      kRenderTargetChannelTypeSrgb             = 0x00000006,
      kRenderTargetChannelTypeFloat            = 0x00000007,
    } RenderTargetChannelType;

void setDataFormat(DataFormat format)
{
  // ....
  int v3;
  RenderTargetChannelType  type;  // [rsp+4h] [rbp-3Ch]
  __int64                  v9;  // [rsp+10h] [rbp-30h]
  bool typeConvertable = format.getRenderTargetChannelType(&type);
  v2                   = type | kRenderTargetChannelTypeSNorm;
  v3                   = (type == kRenderTargetChannelTypeUNorm)
                      || (type == kRenderTargetChannelTypeSNorm)
                      || (type == kRenderTargetChannelTypeSrgb);
  // ....
}
```

Здесь я также заменил числа 0, 1 и 6 на соответствующие именованные значения перечисления и записал подвыражения в табличном виде\.

## Краевой случай в операторе перемещения

[V794](https://pvs-studio.ru/ru/docs/warnings/v794/) The assignment operator should be protected from the case of 'this \=\= &other'\. VltShader\.cpp 39

```cpp
VltShaderConstData& VltShaderConstData::operator=(VltShaderConstData&& other)
{
  delete[] m_data;
  this->m_size = other.m_size;
  this->m_data = other.m_data;
  other.m_size = 0;
  other.m_data = nullptr;
  return *this;
}
```

Если при вызове такого оператора окажется, что _'this \=\= &other'_, то все поля текущего объекта будут очищены и данные потеряются\. Такое поведение некорректно, нужно добавить проверку\. Исправленный код:

```cpp
VltShaderConstData& VltShaderConstData::operator=(VltShaderConstData&& other)
{
  if (this == std::addressof(other))
  {
    return *this;
  }

  delete[] m_data;
  this->m_size = other.m_size;
  this->m_data = other.m_data;
  other.m_size = 0;
  other.m_data = nullptr;
  return *this;
}
```

## Повторное присвоение как повод для рефакторинга

[V1048](https://pvs-studio.ru/ru/docs/warnings/v1048/) \[CWE\-1164\] The 'retVal' variable was assigned the same value\. Module\.cpp 129

```cpp
bool NativeModule::getSymbolInfo( /* .... */) const
{
  bool retVal = false;
  do
  {
    uint32_t modId = 0, libId = 0;
    if (modName == nullptr || libName == nullptr || nid == nullptr)
      {
        break;
      }
      if (!isEncodedSymbol(encSymbol))
      {
        *modName = "";
        *libName = "";
        *nid     = 0;
        retVal   = true;
        break;
      }
      retVal = decodeSymbol(encSymbol, &modId, &libId, nid);
      if (!retVal)
      {
        LOG_ERR("fail to decode encoded symbol");
        break;
      }
      retVal = getModNameFromId(modId, mods, modName);
      if (!retVal)
      {
        LOG_ERR("fail to get module name for symbol: %s in %s",
        encSymbol.c_str(), fileName.c_str());
        break;
      }
      retVal = getLibNameFromId(libId, libs, libName);
      if (!retVal)
      {
        LOG_ERR("fail to get library name");
        break;
      }
      retVal = true;                                                      // <=
    } while (false);
  return retVal;
}
```

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

* Переменная _retVal_ инициализируется значением _false_\.
* Если вызов функции _isEncodedSymbol_ вернул _false_, то переменной _retVal_ присваивается значение _true_ и прерывается цикл _do\-while_\.
* Переменной _retVal_ присваивается результат вызова функции _decodeSymbol_\. После этого если _retVal \=\= false_, то цикл _do\-while_ прерывается\.
* То же самое происходит и с двумя вызовами функции _getModNameFromId_\. Если любой из вызовов вернёт _false_, то цикл _do\-while_ прерывается\.

Заметим, что если цикл _do\-while_ был досрочно прерван, то и указанное анализатором присваивание не выполнится\. Это значит, подозрительное присваивание _retVal \=\= true_ будет выполнено, только если все рассмотренные выше вызовы функций вернули _true_\. Поэтому переменная _retVal_ уже равна _true_, и присваивание не имеет смысла\.

А зачем вообще использовать конструкцию '_do \.\.\. while\(false\)_'? Дело в том, что такая конструкция позволяет сделать ранний выход из функции с одним _return_\. К функциям с одним _return_, в свою очередь, с большей вероятностью будет применена [NRVO](https://pvs-studio.ru/ru/blog/terms/6516/) – named return value optimization\. Эта оптимизация компилятора позволяет избежать лишнего копирования или перемещения возвращаемого объекта, конструируя его сразу на месте вызова функции\. В данном случае функция возвращает легковесный тип _bool_, поэтому выигрыш от NRVO будет незначительным\. Кроме того, современные компиляторы умеют применять NRVO и к функциям [с несколькими _return_](https://godbolt.org/z/GhTv4WTzr), если во всех _return_ возвращается один и тот же объект\.

Метод _getSymbolInfo_ не содержит ошибки и работает так, как задумал программист\. Однако стоит провести рефакторинг метода _getSymbolInfo _и убрать цикл _do\-while_ вместе с переменной _retVal_\. Предлагаю следующий код:

```cpp
bool NativeModule::getSymbolInfo( /* .... */) const
{
  uint32_t modId = 0, libId = 0;
  if (modName == nullptr || libName == nullptr || nid == nullptr)
  {
    return false;
  }

  if (!isEncodedSymbol(encSymbol))
  {
    *modName = "";
    *libName = "";
    *nid     = 0;
    return true;
  }

  if (!decodeSymbol(encSymbol, &modId, &libId, nid))
  {
    LOG_ERR("fail to decode encoded symbol");
    return false;
  }

  if (!getModNameFromId(modId, mods, modName))
  {
    LOG_ERR("fail to get module name for symbol: %s in %s",
    encSymbol.c_str(), fileName.c_str());
    return false;
  }

  if (!getLibNameFromId(libId, libs, libName))
  {
    LOG_ERR("fail to get library name");
    return false;
  }

  return true;
}
```

Здесь я сделал следующее:

* убрал цикл _do\-while_ и лишнюю переменную _retVal_;
* каждую проверку переменной _retVal_ заменил на проверку результата вызова соответствующей функции;
* каждый _break_ цикла _do\-while_ заменил на возврат нужного значения _true_ / _false_\. Какое именно значение вернуть, знаем из анализа переменной _retVal_, который провели ранее\.

На мой взгляд, такой код проще читать и поддерживать\.

## Заключение

Конечно, это не все ошибки и недочёты, которые мы нашли в GPCS4\. Некоторые случаи было довольно сложно описать, поэтому я не включил их в статью\.

Мы желаем разработчикам GPCS4 успехов в дальнейшем развитии эмулятора и рекомендуем проверить текущую версию проекта анализатором PVS\-Studio\. Для этого достаточно [скачать дистрибутив](https://pvs-studio.ru/ru/pvs-studio/try-free/) анализатора и запросить бесплатную лицензию для Open Source проектов\. Если вас заинтересовал статический анализ в целом и PVS\-Studio в частности – самое время попробовать\. Вы можете проверить GPCS4 вслед за нами, а можете проверить свой собственный проект:\) Спасибо за внимание\!