KhronosGroup / SPIRV-Reflect

SPIRV-Reflect is a lightweight library that provides a C/C++ reflection API for SPIR-V shader bytecode in Vulkan applications.
Apache License 2.0
672 stars 147 forks source link

Fix asserting from uvec2 buffer reference bitcast #259

Closed spencer-lunarg closed 5 months ago

spencer-lunarg commented 5 months ago

closes https://github.com/KhronosGroup/SPIRV-Reflect/issues/257

confirmed doing more research that we are safe to return 0 for a push constant that tries to use uvec2 to pass in a buffer reference handle

corporateshark commented 5 months ago

confirmed doing more research that we are safe to return 0 for a push constant that tries to use uvec2 to pass in a buffer reference handle

Based on this logic, will we obtain the correct byte size for push constants?

spencer-lunarg commented 5 months ago

will we obtain the correct byte size for push constants?

Yes, as shown from the shader in the issue, the bufferId is still properly being accounted for (and properly marked as used)

          // size = 128, padded size = 128
          struct PerFrameData {               
              column_major float4x4 MVP;       // abs offset =   0, rel offset =   0, size = 64, padded size = 64 UNUSED
              uint2                 bufferId;  // abs offset =  64, rel offset =  64, size =  8, padded size =  8
              uint                  textureId; // abs offset =  72, rel offset =  72, size =  4, padded size =  4 UNUSED
              float                 time;      // abs offset =  76, rel offset =  76, size =  4, padded size =  4
              uint                  numU;      // abs offset =  80, rel offset =  80, size =  4, padded size =  4
              uint                  numV;      // abs offset =  84, rel offset =  84, size =  4, padded size =  4
              float                 minU;      // abs offset =  88, rel offset =  88, size =  4, padded size =  4 UNUSED
              float                 maxU;      // abs offset =  92, rel offset =  92, size =  4, padded size =  4 UNUSED
              float                 minV;      // abs offset =  96, rel offset =  96, size =  4, padded size =  4 UNUSED
              float                 maxV;      // abs offset = 100, rel offset = 100, size =  4, padded size =  4 UNUSED
              uint                  P1;        // abs offset = 104, rel offset = 104, size =  4, padded size =  4
              uint                  P2;        // abs offset = 108, rel offset = 108, size =  4, padded size =  4
              uint                  Q1;        // abs offset = 112, rel offset = 112, size =  4, padded size =  4
              uint                  Q2;        // abs offset = 116, rel offset = 116, size =  4, padded size =  4
              float                 morph;     // abs offset = 120, rel offset = 120, size =  4, padded size =  8
          } pc;  

The issue going on here was because inside a buffer reference struct, you can have things pointing to itself (ex, linked list)

layout(buffer_reference) buffer VertexBuffer {
    VertexBuffer next_pointer;
};

and we have logic to handle this recursion, but a Push Constants block, it can't be a buffer_reference block by definition, so there is no way to have it point to itself, so we don't need to chase down the buffer reference chain and can just return early

corporateshark commented 5 months ago

Thanks @spencer-lunarg Looking forward to seeing it merged!