Usage of memcmp in operator== problematic

[timo-oster]

Hi,

I'm using nvpro_core code in combination with the Vulkan C++ wrapper in our code. I recently ran valgrind on it and found an issue. nvpro_core is using memcmp to implement comparison operators, e.g. for SamplerPool::SamplerState in samplers_vk.hpp. Vulkan structs can contain padding, and initialization of padding bytes in C++ is not defined by the standard. When I pass a VkSamplerCreateInfo into SamplerPool::acquireSampler that was converted from a vk::SamplerCreateInfo, then that struct contains 4 padding bytes between sType and pNext which are uninitialized memory. This means that two VkSamplerCreateInfo which are otherwise identical could be compared as not equal by memcmp. Since SamplerState, which contains a VkSamplerCreateInfo member, is used as a key to a map , this could lead to bugs (the content of the padding bytes are copied into the SamplerState by my compiler when assigning to the createInfo member). It also leads to valgrind reporting the use of uninitialized memory when the SamplerState is used with the map. Just wanted to let you know in case you want to do something about it. On my end I'm changing the implementation of SamplerState::operator== to compare all the members explicitly, which should avoid comparing the content of padding bytes, but is much uglier to write.

[timo-oster]

The Hash_fn in samplers_vk.hpp calculates a hash based on the raw bytes of the SamplerState object and therefore could also suffer from similar problems.

[timo-oster]

Here's a hash function that avoids the problem (valgrind errors are gone for me if I use it):

  template<typename First, typename... Args>
  static std::size_t combineHash(std::size_t seed, First const& first, Args const&... rest)
  {
    return combineHash(combineHash(seed, first), rest...);
  }
  
  template<typename T>
  static std::size_t combineHash(std::size_t seed, T const& value)
  {
    std::hash<T> hasher;
    // https://www.boost.org/doc/libs/1_35_0/doc/html/boost/hash_combine_id241013.html
    return seed ^ (hasher(value) + 0x9e3779b9 + (seed << 6) + (seed >> 2));
  }

  struct Hash_fn
  {
    std::size_t operator()(const SamplerState& s) const
    {
      return combineHash(0, s.createInfo.flags, 
                         s.createInfo.magFilter, s.createInfo.minFilter,
                         s.createInfo.mipmapMode, s.createInfo.addressModeU, 
                         s.createInfo.addressModeV, s.createInfo.addressModeW, 
                         s.createInfo.mipLodBias, s.createInfo.anisotropyEnable, 
                         s.createInfo.maxAnisotropy, s.createInfo.compareEnable,
                         s.createInfo.compareOp, s.createInfo.minLod,
                         s.createInfo.maxLod, s.createInfo.borderColor, 
                         s.createInfo.unnormalizedCoordinates, 
                         s.reduction.reductionMode,
                         s.ycbr.format, s.ycbr.ycbcrModel,
                         s.ycbr.ycbcrRange, s.ycbr.components.r,
                         s.ycbr.components.g, s.ycbr.components.b, 
                         s.ycbr.components.a, s.ycbr.xChromaOffset,
                         s.ycbr.yChromaOffset, s.ycbr.chromaFilter,
                         s.ycbr.forceExplicitReconstruction);                        
    }
  };

[pixeljetstream]

thanks!