Unsound push_constants command implementation

[Amjad50]

The current implementation of push_constants in auto.rs has many problems:

1- Serious buffer overflow bug (increment offset with same size):

vulkano/vulkano/src/command_buffer/auto.rs

Lines 1874 to 1877 in 4f09439

	let data = slice::from_raw_parts(
	(&push_constants as *const Pc as *const u8).offset(offset as isize),
	size as usize,
	);

2- When calling push_constants we don't know which stages will be pushed to:
If you have multiple stages, it will try to include as many as it can.
For example if you have these push_constant ranges:

fragment (0..4)
vertex (0..4)

It will push to the two of them, even if the content is different, for example:

fragment:
layout(push_constant) uniform PushConstantData1 {
    uint a;
} pc;

vertex:
layout(push_constant) uniform PushConstantData2 {
    uint b;
} pc;

3- Also there is a bug in selecting the stages, if you have two stages

fragment (0..4)
vertex (0..8)

And you pushed struct of size 8, it will push to the two of them, which invalidates

VUID-vkCmdPushConstants-offset-01795
For each byte in the range specified by offset and size and for each shader stage in stageFlags, there must be a push constant range in layout that includes that byte and that stage

I'll create a PR on making it mandatory to supply the stage that is needed, and also fix the buffer overflow bug.

Any other suggestions to improve push_constants handling?

[Rua]

I'm not sure if you understood it that way, but there isn't different push constant data for different stages. Rather, there is one set of data for all stages, and each stage declares (in the pipeline layout) which part of that data it's going to access. So in the example you gave for point 2, a and b will always map to the same data, the data can't be set for each one separately.

To actually make them map to different ranges, you'd do it like this:

layout(push_constant) uniform PushConstantData1 {
    layout(offset = 0) uint a;
} pc;

layout(push_constant) uniform PushConstantData2 {
    layout(offset = 4) uint b;
} pc;

Now, one stage has the range 0..4 and the other has 4..8.

[Amjad50]

Yes, your example is correct if we want to push two data using one push_constant command.
But Vulkan allows to push two separate data and changing the ShaderStageFlags bits, in our current implementation, we can't do that.

Push constant values can be updated incrementally, using shader stages in stageFlags

[Rua]

"Incrementally" means between one draw/dispatch command and the next. But there is only one set of data per single draw/dispatch command, one command will always use the current push constant data for all stages. The stageFlags value doesn't indicate that the new data only affects those stages, but rather it's simply a value that you must set to indicate all stages whose range overlaps the data you're setting. That's what VUID-vkCmdPushConstants-offset-01796 indicates. You can experiment a bit with it and see.

[Amjad50]

I think I might have misunderstood some parts of it, I'll experiment it.

[Amjad50]

If push_constants was called two times:

• offset=0, size=4, data=123u32
• offset=4, size=4, data=456u32

Does that mean that all shaders will get the same data: [123u32, 456u32]?

[Rua]

Yes, assuming that they access the whole 0..8 range.

[Amjad50]

Then, it is be possible to handle push_constants without having to pass the stages flags. Only need to fix the bugs.

[Rua]

The stage flags must be set in order to do what VUID-vkCmdPushConstants-offset-01796 requires. The code here figures out the stages automatically:

vulkano/vulkano/src/command_buffer/auto.rs

Lines 1843 to 1871 in 4f09439

	// Figure out which shader stages in the pipeline layout overlap this byte range.
	// Also check that none of the bytes being set are outside all push constant ranges.
	let shader_stages = pipeline_layout
	.push_constant_ranges()
	.iter()
	.filter(|range| range.offset < offset + size && offset < range.offset + range.size)
	.try_fold(
	(ShaderStages::none(), offset),
	|(shader_stages, last_bound), range| {
	if range.offset > last_bound {
	Err(())
	} else {
	Ok((
	shader_stages.union(&range.stages),
	last_bound.max(range.offset + range.size),
	))
	}
	},
	)
	.and_then(|(shader_stages, last_bound)| {
	if shader_stages == ShaderStages::none() || last_bound < offset + size {
	Err(())
	} else {
	Ok(shader_stages)
	}
	})
	.expect(
	"not all bytes in push_constants fall within the pipeline layout's push constant ranges",
	);

[Amjad50]

My plan to fix point 3. is to call cmdPushConstants multiple times, each with correct offset and size

[Rua]

I don't think 3 is an error at all. The VUID just means "every byte must be in at least one of the push constant ranges". And in this case it is, since all 8 bytes are within the vertex stage's range.

[Amjad50]

Actually it is, the validator reports it, that's what got me to investigate it, but not sure if it's a bug in the validator. I got also confused by it.

[Rua]

Hmm that may mean the code above has a bug in it then. What error did the validator give?

[Amjad50]

Thread 1, Frame 0:
vkCreatePipelineLayout(device, pCreateInfo, pAllocator, pPipelineLayout) returns VkResult VK_SUCCESS (0):
    device:                         VkDevice                         = 0x56055c03a980
    pCreateInfo:                    const VkPipelineLayoutCreateInfo* = 0x7faf20e8eeb8:
        sType:                          VkStructureType                  = VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO (30)
        pNext:                          const void*                      = NULL
        flags:                          VkPipelineLayoutCreateFlags      = 0
        setLayoutCount:                 uint32_t                         = 1
        pSetLayouts:                    const VkDescriptorSetLayout*     = 0x7faf20e8f100
            pSetLayouts[0]:                 const VkDescriptorSetLayout      = 0x7faf1c01b8a0
        pushConstantRangeCount:         uint32_t                         = 2
        pPushConstantRanges:            const VkPushConstantRange*       = 0x7faf20e8efdc
            pPushConstantRanges[0]:         const VkPushConstantRange        = 0x7faf20e8efdc:
                stageFlags:                     VkShaderStageFlags               = 1 (VK_SHADER_STAGE_VERTEX_BIT)
                offset:                         uint32_t                         = 0
                size:                           uint32_t                         = 24
            pPushConstantRanges[1]:         const VkPushConstantRange        = 0x7faf20e8efe8:
                stageFlags:                     VkShaderStageFlags               = 16 (VK_SHADER_STAGE_FRAGMENT_BIT)
                offset:                         uint32_t                         = 0
                size:                           uint32_t                         = 72
    pAllocator:                     const VkAllocationCallbacks*     = NULL
    pPipelineLayout:                VkPipelineLayout*                = 0x7faf1c0201e0

.....

VUID-vkCmdPushConstants-offset-01795(ERROR / SPEC): msgNum: 666667206 - Validation Error: [ VUID-vkCmdPushConstants-offset-01795 ] Object 0: handle = 0x7faf1c06ec10, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x27bc88c6 | vkCmdPushConstants(): VK_SHADER_STAGE_VERTEX_BIT|VK_SHADER_STAGE_FRAGMENT_BIT, VkPushConstantRange in VkPipelineLayout 0x7faf1c0201e0[] overlapping offset = 0 and size = 72, do not contain VK_SHADER_STAGE_VERTEX_BIT. The Vulkan spec states: For each byte in the range specified by offset and size and for each shader stage in stageFlags, there must be a push constant range in layout that includes that byte and that stage (https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#VUID-vkCmdPushConstants-offset-01795)
    Objects: 1
        [0] 0x7faf1c06ec10, type: 6, name: NULL
Thread 1, Frame 3:
vkCmdPushConstants(commandBuffer, layout, stageFlags, offset, size, pValues) returns void:
    commandBuffer:                  VkCommandBuffer                  = 0x7faf1c06ec10
    layout:                         VkPipelineLayout                 = 0x7faf1c0201e0
    stageFlags:                     VkShaderStageFlags               = 17 (VK_SHADER_STAGE_VERTEX_BIT | VK_SHADER_STAGE_FRAGMENT_BIT)
    offset:                         uint32_t                         = 0
    size:                           uint32_t                         = 72
    pValues:                        const void*                      = 0x7faf1c038eb0