Merge branch 'dev/ralston/flags_fix' into 'main'

Fix Flags Bugs

See merge request lightspeedrtx/dxvk-remix-nv!705

src/dxvk/dxvk_pipelayout.cpp

@@ -151,7 +151,9 @@ namespace dxvk {
       if (bindingInfos[i].type == VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC)
         m_dynamicSlots.push_back(i);
 
-      m_descriptorTypes.set(bindingInfos[i].type);
+      // NV-DXVK start: Replace unsuitable Flags set function with new boolean setting function
+      setDescriptorType(bindingInfos[i].type);
+      // NV-DXVK end
     }
 
     // Create descriptor set layout. We do not need to

src/dxvk/dxvk_pipelayout.h

@@ -247,8 +247,9 @@ namespace dxvk {
      * type.
      */
     bool hasStaticBufferBindings() const {
-      return m_descriptorTypes.test(
-        VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER);
+      // NV-DXVK start: Replace unsuitable Flags struct with per-type booleans
+      return m_hasUniformBufferDescriptorType;
+      // NV-DXVK end
     }
 
     /**
@@ -275,6 +276,22 @@ namespace dxvk {
     }
 
   private:
+    // NV-DXVK start
+    // Note: Replacement code for what was previously an incorrectly used Flags bitset.
+    // Using a Flags<VkDescriptorType> was invalid because some descriptor types set in it
+    // are far out of the valid range (e.g. VK_DESCRIPTOR_TYPE_ACCELERATION_STRUCTURE_KHR = 1000150000),
+    // which caused undefined behavior.
+    void setDescriptorType(VkDescriptorType descriptorType) {
+      switch (descriptorType) {
+      case VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER:
+        m_hasUniformBufferDescriptorType = true;
+
+        break;
+      default:
+        break;
+      }
+    }
+    // NV-DXVK end
 
     Rc<vk::DeviceFn> m_vkd;
 
@@ -286,7 +303,9 @@ namespace dxvk {
     std::vector<DxvkDescriptorSlot> m_bindingSlots;
     std::vector<uint32_t>           m_dynamicSlots;
 
-    Flags<VkDescriptorType>         m_descriptorTypes;
+    // NV-DXVK start: Replace unsuitable Flags struct with per-type booleans
+    bool                            m_hasUniformBufferDescriptorType = false;
+    // NV-DXVK end
 
     bool m_hasExtraLayouts = false;
   };

src/dxvk/rtx_render/rtx_lights_data.cpp

@@ -119,7 +119,7 @@ namespace dxvk {
 
   void LightData::merge(const D3DLIGHT9& light) {
     // Special case, dont do any merging if we know we dont need to
-    if (m_dirty != DirtyFlags::AllDirty) {
+    if (m_dirty != m_allDirty) {
       std::optional<LightData> input = tryCreate(light);
       if (input.has_value()) {
         merge(input.value()); // when converting from legacy lights, we always use the games transform
@@ -396,7 +396,7 @@ namespace dxvk {
 
     // If this light is fully defined (i.e. a child light) then we need to use all attributes
     if (prim.GetSpecifier() == pxr::SdfSpecifier::SdfSpecifierDef) {
-      m_dirty = DirtyFlags::AllDirty;
+      m_dirty = m_allDirty;
     }
   }
 

src/dxvk/rtx_render/rtx_lights_data.h

@@ -21,6 +21,8 @@
 */
 #pragma once
 
+#include <type_traits>
+
 #include "rtx_lights.h"
 
 #define LIST_LIGHT_CONSTANTS(X) \
@@ -103,17 +105,23 @@ namespace dxvk {
     enum class DirtyFlags : uint32_t {
       LIST_LIGHT_CONSTANTS(WRITE_DIRTY_FLAGS)
       k_Transform,
-      AllDirty = 0xFFFFffff
     };
-
+
+    // Note: Be very careful with what is passed to the Flags class's constructor. A bug previously existed where this all ones bit
+    // pattern was a DirtyFlags enum value itself (e.g. AllDirty = 0xFFFFFFFF), causing it to call the wrong conversion constructor and being
+    // interpreted as a bit index to set instead of a raw integer value to manually set the flags with. This caused great pain as shifting
+    // by such a large number in the internal set function caused undefined behavior (as C++ does not allow shifting greater than the number
+    // of bits in the type being shifted), which in turn caused all the flags to be cleared rather than all to be set.
+    constexpr static Flags<DirtyFlags> m_allDirty{ ~static_cast<std::underlying_type_t<DirtyFlags>>(0) };
+
     LIST_LIGHT_CONSTANTS(WRITE_PARAMETER_MEMBERS)
 
     enum TransformType {
       Absolute,
       Relative
     };
 
-    Flags<DirtyFlags> m_dirty { 0 };
+    Flags<DirtyFlags> m_dirty{};
     LightType m_lightType;
     XXH64_hash_t m_cachedHash = kEmptyHash;
     // NOTE: Just add params for these without USD deserializer

src/util/util_flags.h

@@ -1,11 +1,19 @@
 #pragma once
 
+#include <cassert>
+#include <climits>
 #include <type_traits>
 
 #include "util_bit.h"
 
 namespace dxvk {
 
+  // Takes an enum or enum class type to use as a set of bits, the value of each enum member representing
+  // the index of the bit they represent in the flags bitset.
+  // Warning: All values in the enum/enum class which are intended to be used in setting/testing/etc
+  // operations must have a value less than the number of bits in the underlying enum type. This is only
+  // a problem when manually setting enum values, e.g. Foo = 0xFFFFFFFF will certianly cause an issue if
+  // ever used (though an assertion will at least guard against this at runtime).
   template<typename T>
   class Flags {
 
@@ -15,7 +23,7 @@ namespace dxvk {
 
     Flags() { }
 
-    Flags(IntType t)
+    constexpr Flags(IntType t)
     : m_bits(t) { }
 
     template<typename... Tx>
@@ -93,6 +101,16 @@ namespace dxvk {
     IntType m_bits = 0;
 
     static IntType bit(T f) {
+      // NV-DXVK start: Flags safety improvements
+      // Note: This check exists to ensure that undefined behavior is not invoked when attempting to set a bit,
+      // as left shifts greater or equal to the number of bits in the type is undefined behavior in C++ and
+      // numerous bugs in the past due to invalid usage ofthe Flags class.
+      // Do note though that this is a conservative check as C++ does not give an easy way to get the exact
+      // number of bits in a type (numeric_limits<T>::digits has to be adjusted by sign). It will be at
+      // least less than this value though (and in practice cursed 29 bit integers are not a thing anyways).
+      assert(static_cast<IntType>(f) < (sizeof(IntType) * CHAR_BIT));
+      // NV-DXVK end
+
       return IntType(1) << static_cast<IntType>(f);
     }