# Checking the GPCS4 emulator: will we ever be able to play "Bloodborne" on PC?

An emulator is an application that enables a computer with one operating system to run programs designed for a completely different operating system\. Today we talk about GPCS4 — the emulator designed to run PS4 games on PC\. Recently, GPCS4 announced their first release, so we decided to check the project\. Let's see what errors PVS\-Studio managed to find in the source code of the emulator\.

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

## About the project

GPCS4 is a PlayStation 4 emulator written in C and C\+\+\.

Initially, the author of the project intended to investigate the PS4 architecture\. However, the project has evolved rapidly, and in early 2020, the developers of GPCS4 managed to run a game on their emulator — [We are Doomed](https://wccftech.com/playstation-4-emulator-gpcs4/)\. It was the first successful launch of a PS4 game on PC\. The game is far from perfect though, it runs at very low FPS and has graphical glitches\. Nevertheless, the [developer of the project](https://github.com/Inori/) is full of enthusiasm and continues to enhance the emulator\.

The [first release of GPCS4](https://github.com/Inori/GPCS4/releases/tag/v0.1.0) took place at the end of April 2022\. I downloaded and checked the project's v0\.1\.0\. Actually, at the time of publication of this article, v0\.2\.1 has already been released — the project is developing rapidly\. Let's move on to the errors and defects that the PVS\-Studio analyzer managed to find in the first release of the GPCS4 project\.

## Missing break

[V796](https://pvs-studio.com/en/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;
}
```

In this code fragment, the _break_ statement is missing in the _SCE\_AUDIO\_OUT\_PARAM\_FORMAT\_FLOAT\_MONO_ case statement\. As a result, the number of channels will be set incorrectly\.

## The pointer is checked after its use

[V595](https://pvs-studio.com/en/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;
}
```

In the fragment above, the _m\_moduleData _pointer is first dereferenced, and then compared with _nullptr_ in the _do\-while_ loop\.

Attentive readers might object: "It maybe that a valid pointer is passed to function\. And then in the _do\-while_ loop, this pointer is modified and can become a null pointer\. So there is no mistake here\." This is not the case\. Firstly, due to the _while \(false\)_ condition, the loop is iterated exactly once\. Secondly, the _m\_moduleData_ pointer is [not modified](https://github.com/Inori/GPCS4/blob/67442ae29df1916137a01b87b675375d8b48be0e/GPCS4/Loader/ELFMapper.cpp#L45-L88)\.

Another objection may be that using a reference is safe\. After all, this reference will be used only if the pointer is valid\. But no, this [code invokes undefined behavior](https://pvs-studio.com/en/blog/posts/cpp/0306/)\. It's an error\. Most likely you need to do a pointer check before dereferencing it:

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

## Double assignment

[V519](https://pvs-studio.com/en/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;
}
```

As you can guess from the _LOG\_SCE\_DUMMY\_IMPL _name, the implementation of the _sceKernelGetDirectMemoryType_ method will be changing\. Still, two assignments to the same _memoryType_ address looks strange\. This may have been the result of a failed code merge\.

## Buffer overflow

[V512](https://pvs-studio.com/en/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 \[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;
}
```

Sometimes one code line triggers several PVS\-Studio diagnostics\. The following example is one of those cases\. In this code fragment, an incorrect value is passed to the _memset_ function as the third argument\. The _sizeof\(param\-\>reserved\)_ expression will return the size of the _param\-\>reserved _array\. Multiplication by _sizeof\(uint32\_t\)_ will increase this value by 4 times, and the value will be incorrect\. So the _memset_ call will result in an overrun of the _param\-\>reserved_ array\. You need to remove the extra multiplication:

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

In total, the analyzer detected 20 such overflows\. Let me show another example:

[V512](https://pvs-studio.com/en/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;
}
```

In this code fragment, the _initParam\-\>reserved_ array goes out of bounds\.

## Learning to count to seven, or another buffer overflow

[V557](https://pvs-studio.com/en/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;
  // ....
}
```

The analyzer warns that an overflow of the _dynamicStates_ array may occur\. There are 4 checks in this code fragment:

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

Each of these checks is a check of one of the independent flags\. For example, the check of _if \(state\.useDynamicDepthBias\(\)\)_:

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

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

It turns out that all these 4 checks can be true at the same time\. Then 7 lines of the _'dynamicStates\[dynamicStateCount\+\+\] \=\.\.\.\.'_ kind will be executed\. On the seventh such line, there will be a call to _dynamicStates\[6\]_\. It's an array index out of bounds\.

To fix it, you need to allocate memory for 7 elements:

```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;
  // ....
}
```

## Incorrect flag usage

[V547](https://pvs-studio.com/en/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;
}
```

The _GetProtectFlag_ function converts a flag with file access permission from one format to another\. However, the function does this incorrectly\. The developer did not take into account that the value of _VMPF\_NOACCESS _is zero\. Because of this, the _if \(nOldFlag & VMPF\_NOACCESS\)_ condition is always false and the function will never return the _PAGE\_NOACCESS_ value\.

In addition, the _GetProtectFlag_ function incorrectly converts not only the _VMPF\_NOACCESS_ flag, but also other flags\. For example, the _VMPF\_CPU\_EXEC_ flag will be converted to the _PAGE\_EXECUTE\_READWRITE_ flag\.

When I was thinking how to fix this issue, my first thought was to write something like this:

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

However, in this case, this approach does not work\. The thing is, _PAGE\_NOACCESS_, _PAGE\_READONLY_ and other flags are Windows flags and they have their own specifics\. For example, there is no _PAGE\_WRITE _flag among them\. It is assumed that if there are write permissions, then at least there are also read permissions\. For the same reasons, there is no _PAGE\_EXECUTE\_WRITE_ flag\.

In addition, the bitwise "OR" with two Windows flags does not result in a flag that corresponds to the sum of the permissions: _PAGE\_READONLY \| PAGE\_EXECUTE \!\= PAGE\_EXECUTE\_READ_\. Therefore, you need to iterate through all possible flag combinations:

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

## Extra check

[V547](https://pvs-studio.com/en/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);
      }
    // ....
    }
  // ....
}
```

The _retAddress _pointer is checked twice in the code fragment above\. First, _if \(\!retAddress\)_ is checked\. If the pointer is null, execution proceeds to the next iteration of the _while_ loop\. Otherwise, the _retAddress _pointer is not null\. So the second _if \(retAddress\)_ check is always true, and it can be removed\.

## One more condition that is always true

[V547](https://pvs-studio.com/en/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;
    }
```

In this code fragment, the analyzer found the _if \(pipeConfig \=\= kPipeConfigP16\)_ condition that is always true\. Let's figure out why this is so\.

If the _getPipeConfig_ function call returns a value that doesn't equal _kPipeConfigP16_, the first condition will be true and the program execution will not proceed to the check of _if \(pipeConfig \=\= kPipeConfigP16\)_\.

It turns out that the second check of this variable is either not performed, or is always true\. But do not rush and remove it\. Maybe the first condition was added temporarily and will be removed in the future\.

## Copy paste error

[V517](https://pvs-studio.com/en/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;
  // ....
}
}
```

Here come the copy\-paste errors\. In this code snippet, the same _newArrayMode \=\= Gnm::kArrayMode2dTiledThin_ check is written twice\.

It's hard to say exactly how to fix this\. Most likely, the second check should be somewhat different\. Or maybe it is redundant and can be removed\.

## Why is it better to avoid complex expressions?

[V732](https://pvs-studio.com/en/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; // <=
  // ....
}
```

It looks like the programmer was expecting the following behaviour during the expression calculation:

* let the _type_ variable be less than _7_;
* then the _type < 7_ expression is _true_;
* a unary minus is applied to _true_, the result is _\-1_;
* the _\-1_ value is converted to an _unsigned char_, the result is _0b1111'1111_\.

However, that's what actually happens:

* let the _type_ variable be less than _7_;
* then the _type < 7_ expression is _true_;
* a unary minus is applied to _true_, the result is _1_;
* the _1_ value is converted to an _unsigned char_, the result is _0b0000'0001_\.

Although, the following _& 1_ operation leads to the same result\. By this happy coincidence, the whole code works as the developer intends\. However, it's better to correct this code\. Depending on the _type_ value, let's guess what value is assigned to the _v3_ variable\.

The first case: the _type_ variable is greater than or equal to 7\.

* Then the _type < 7_ expression is _false_;
* A unary minus is applied to _false_, the result is _false_\.
* False is converted to unsigned char, the result is _0b0000'0000_\.
* A bitwise "AND" with 0 always gives 0, so we get 0 as a result\.

The second case: the _type_ variable is less than 7\.

* As we found out earlier, the _\(uint8\_t\) is \(type < 7\)_ expression equals 1\.
* In this case, it makes sense to calculate the _0x43u \>\> type_ expression\.
* For convenience, let's write the binary representation of the number the following way: _0x43 \= 0b0100'0011_\.
* We are only interested in the least significant bit, because the bitwise "AND" with 1 will be applied to the result of the _0x43u \>\> type_ expression\.
* If _type_ equals 0, 1, or 6, the least significant bit will be 1, and the result of the entire expression will be 1\. In all other cases, the expression result will be 0\.

To conclude, if type is 0, 1 or 6, the value 1 is written to the v3 variable\. In all other cases, the value 0 is written to the v3 variable\. It is worth replacing a complex expression with a simpler and more understandable one — _\(type \=\= 0\) \|\| \(type \=\= 1\) \|\| \(type \=\= 6\)_\. Let me suggest the following code:

```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);
  // ....
}
```

I also replaced the 0, 1 and 6 numbers with the corresponding named enumeration values and wrote the subexpressions in a table form\.

## Corner case in move operator

[V794](https://pvs-studio.com/en/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;
}
```

If this operator is called and _'this \=\= &other'_, all fields of the current object will be cleared and data will be lost\. This behavior is incorrect, the check should be added\. Fixed code:

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

## Repeated assignment as a reason to refactor

[V1048](https://pvs-studio.com/en/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;
}
```

In this code fragment, the _true_ value is assigned twice to the _retVal_ variable\. Let's figure out why this is happening\. First, let's view all possible modifications to the variable _retVal_ prior to the assignment indicated by the analyzer\.

* The _retVal _variable is initialized to _false_\.
* If the _isEncodedSymbol_ function call returned _false_, the _true _value is assigned to _retVal_ and the _do\-while_ loop is interrupted\.
* The result of the _decodeSymbol_ function call is assigned to the _retVal _variable\. After that, if _retVal \=\= false_, the _do\-while_ loop is interrupted\.
* The same thing happens with two calls of the _getModNameFromId_ function\. If any of the calls returns _false_, the _do\-while_ loop is interrupted\.

Note that if the _do\-while_ loop was prematurely interrupted, the assignment indicated by the analyzer won't be executed\. This means that the suspicious _retVal \=\= true_ assignment will only be executed if all the function calls discussed above have returned _true_\. Therefore, the _retVal_ variable is already _true_, and the assignment does not make sense\.

And why use the _'do \.\.\. while\(false\)'_ construct at all? The thing is, this construct allows to make an early exit from the function with a single _return_\. For functions with a single _return_, in turn, named return value optimization — [NRVO](https://pvs-studio.com/en/blog/terms/6516/) – is more likely to be applied\. This compiler optimization avoids unnecessary copying or moving of the return object\. This is done by constructing the object directly at the function call location\. In this case, the function returns the lightweight _bool_ type, so the gain from NRVO is minor\. In addition, modern compilers are able to apply NRVO to functions [with multiple_ return_](https://godbolt.org/z/GhTv4WTzr) statements, if the same object is returned in all _return_ statements\.

The _GetSymbolInfo_ method does not contain errors and works as the programmer intended\. However, it's better to refactor the _GetSymbolInfo_ method and remove the _do\-while_ loop with the _retVal_ variable\. Let me suggest the following code:

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

I did the following:

* removed the _do\-while_ loop and the extra _retVal_ variable;
* replaced each _retVal_ variable check by a check of the result of the corresponding function call;
* replaced each _break_ of the _do\-while_ loop by the corresponding return statement — _true_ / _false_\. We know which value to return from the analysis of the _retVal_ variable we did earlier\.

In my opinion, such code is easier to read and maintain\.

## Conclusion

Of course, these are not all the errors and defects that we found in GPCS4\. Some cases were quite difficult to describe, so I did not include them in the article\.

We wish GPCS4 developers success in their further development of the emulator, and recommend checking the latest version of the project with PVS\-Studio analyzer\. You can just [download the analyzer distribution](https://pvs-studio.com/en/pvs-studio/try-free/) and request a free license for Open Source projects\. If you are interested in static analysis in general and PVS\-Studio in particular, it's time to try it\. You can also check GPCS4, or you can check your own project :\) Thank you for your attention\!