Fix uniform type enum UB regarding extra fragment-bit.

[mcourteaux]

Caught with UBSan. It apparently is UB if you start passing Enum values that do not exist in the enumerated enum values.

[bkaradzic]

I don't like this... Where exactly is UB?

[mcourteaux]

I don't like this... Where exactly is UB?

See here. Copying the excerpt here:

C++ standard 5.2.9 Static cast [expr.static.cast] paragraph 7

A value of integral or enumeration type can be explicitly converted to an enumeration type. The value is unchanged if the original value is within the range of the enumeration values (7.2). Otherwise, the resulting enumeration value is unspecified / undefined (since C++17).

So this would mean that the casts in lines such as:

bgfx/src/renderer_vk.cpp

Line 5093 in de56398

m_constantBuffer->writeUniformHandle( (UniformType::Enum)(type|fragmentBit), regIndex, info->m_handle, regCount);

are not allowed (since C++17 🤷?), since the cast from the integral type to the enum type is not a value within the range of the enumeration values.

[mcourteaux]

I must admit that I also don't like this PR. This is just annoying language semantics/rules sitting in the way of doing something efficient AND readable. You may propose an alternative that would also get rid of the UB, which is more to your liking, and I'll take care of it!

I like clang's ubsan. I found many bugs in my software already. I think this is good practice to have this turned on for debug builds.

[bkaradzic]

Alternative is to add annotation around, or special inline function that would do conversion that would turn off asan checks for that particular thing.

[mcourteaux]

What about something along the lines of:

template<typename E, int EnumBits>
struct EnumWithBits {
  static constexpr std::underlying_t<E> mask = (1 << EnumBits) - 1;

  std::underlying_t<E> value;

  inline void set(E eval, int bits) {
    assert(eval & mask == eval);
    value = static_cast<std::underlying_t<E>>(eval) | (bits << EnumBits);
  }

  E get_enum() { return static_cast<E>(value & mask) }
  int get_extra_bits() { return value >> EnumBits; }
};

The function that cheats and doesn't take a pure enum, but an enum with the extra bit, could look like:

void UniformBuffer::writeUniformHandle(EnumWithBits<UniformType::Enum, 3> _type, uint16_t _loc, UniformHandle _handle, uint16_t _num)

or potentially with an extra using UniformTypeWithBits = EnumWithBits<UniformType::Enum, 3> somewhere...

[bkaradzic]

🤮

[bkaradzic]

Change your PR to use uint8_t instead of uint16_t since it's enough...

void UniformBuffer::writeUniform(uint8_t _type, ...

And I'll accept it.

[mcourteaux]

Change your PR to use uint8_t instead of uint16_t since it's enough...

void UniformBuffer::writeUniform(uint8_t _type, ...

And I'll accept it.

Done. I have an alternative PR which tries to preserve the use of the actual enum by separating out those extra type bits, in #3402. Pick whichever you like best.