KhronosGroup / Vulkan-ValidationLayers

Vulkan Validation Layers (VVL)
https://vulkan.lunarg.com/doc/sdk/latest/linux/khronos_validation_layer.html
Other
752 stars 403 forks source link

tests: PositiveShaderLimits::ComputeSharedMemoryWorkgroupMemoryExplicitLayoutSpec SIGFPE #6404

Closed HildarTheDorf closed 11 months ago

HildarTheDorf commented 1 year ago

Environment:

Describe the Issue

The test PositiveShaderLimits::ComputeSharedMemoryWorkgroupMemoryExplicitLayoutSpec causes a divide by zero fault on my machine deep in the driver. The underlying error is that the spir-v produced by glslang lays the structure out in such a way it will blow past maxComputeSharedMemorySize. Laying out a struct like this using a spec constant will not work as intended as the OpMemberDecorate in the spir-v will be fixed at glsl->spir-v time, and not updated at spirv->bytecode time. This causes the entire test run to end prematurely, not running any further tests.

Expected behavior Test passes, is skipped, or at least fails gracefully

spencer-lunarg commented 1 year ago

we use const uint32_t max_shared_memory_size = m_device->phy().properties().limits.maxComputeSharedMemorySize; to get the size and go from there, the glslang we use ProcessConfigFile(device_limits); to set these in the GLSL we generate

Is it possible to get me the maxComputeSharedMemorySize on your machine and the csSource we generate

HildarTheDorf commented 1 year ago

The issue is that the glsl will always specify X.x1 to be an array of max_shared_vec_4 + 16 vec4s.

On my machine this means the OpMemberDecorate in the generated spirv puts x2 at an offset of 65792 but maxComputeSharedMemorySize is 65536. I think this will always invoke UB the way it is written as sizeof(X::x1) will always be greater than maxComputeSharedMemorySize, the comment in the source even claims that was intended!

I don't believe the underlying bug is environment specific, it's just some drivers are more polite with their handling of the UB.

As requested though:

maxComputeSharedMemorySize = 65536


        #extension GL_EXT_shared_memory_block : enable

        // will be over the max if the spec constant uses default value
        layout(constant_id = 0) const uint value = 4112;

        // Both structs by themselves are 16 bytes less than the max
        shared X {
            vec4 x1[value];
            vec4 x2;
        };

        shared Y {
            int y1[16380];
            int y2;
        };

        void main() {
            x2.x = 0.0f; // prevent dead-code elimination
            y2 = 0;
        }```
spencer-lunarg commented 1 year ago

ok took a look, this is interesting (and thanks for digging into this a bit!)

so the idea of this test was that layout(constant_id = 0) const uint value = 4112; is too much, but when the spec constants are applied, it should be below... but as you pointed out, there is still a OpMemberDecorate %X 1 Offset 65792 in the SPIR-V .... fun

it's just some drivers are more polite with their handling of the UB.

this should have triggered a validation error, will need to understand why it isn't