Don't access sampler handles for sampled image descriptors
The Vulkan 1.1 spec states that:
"For VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, VK_DESCRIPTOR_TYPE_STORAGE_IMAGE,
or VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT, only the imageView and
imageLayout members of each element of VkWriteDescriptorSet::pImageInfo
are accessed."
Also remove the zeroing of sampler descriptor data in
SampledImageDescriptor when no sampler handle is provided. This was
intended to detect bugs but we didn't do it consistently for all
descriptor types (VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER also uses
SampledImageDescriptor and has no sampler data). We now pass a null
pointer to the sampling routine when no sampler descriptor is available
(specifically for OpImageFetch) so it is less likely to still access it
unintentionally.
Bug: b/139401791
Change-Id: Id1394493e88b7465fe3628c4c7d410cd3eaa3ccf
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/35388
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Chris Forbes <chrisforbes@google.com>
Kokoro-Presubmit: kokoro <noreply+kokoro@google.com>
diff --git a/src/Vulkan/VkDescriptorSetLayout.cpp b/src/Vulkan/VkDescriptorSetLayout.cpp
index 9f6eb96..6fe509b 100644
--- a/src/Vulkan/VkDescriptorSetLayout.cpp
+++ b/src/Vulkan/VkDescriptorSetLayout.cpp
@@ -158,7 +158,7 @@
for(uint32_t j = 0; j < bindings[i].descriptorCount; j++)
{
SampledImageDescriptor* imageSamplerDescriptor = reinterpret_cast<SampledImageDescriptor*>(mem);
- imageSamplerDescriptor->updateSampler(vk::Cast(bindings[i].pImmutableSamplers[j]));
+ imageSamplerDescriptor->updateSampler(bindings[i].pImmutableSamplers[j]);
mem += typeSize;
}
}
@@ -255,18 +255,9 @@
return &descriptorSet->data[byteOffset];
}
-void SampledImageDescriptor::updateSampler(const vk::Sampler *newSampler)
+void SampledImageDescriptor::updateSampler(const VkSampler newSampler)
{
- if(newSampler)
- {
- memcpy(reinterpret_cast<void*>(&sampler), newSampler, sizeof(sampler));
- }
- else
- {
- // Descriptor ID's start at 1, allowing to detect descriptor update
- // bugs by checking for 0. Also avoid reading random values.
- memset(reinterpret_cast<void*>(&sampler), 0, sizeof(sampler));
- }
+ memcpy(reinterpret_cast<void*>(&sampler), vk::Cast(newSampler), sizeof(sampler));
}
void DescriptorSetLayout::WriteDescriptorSet(Device* device, DescriptorSet *dstSet, VkDescriptorUpdateTemplateEntry const &entry, char const *src)
@@ -292,7 +283,7 @@
// descriptorCount of zero, must all either use immutable samplers or must all not use immutable samplers."
if (!binding.pImmutableSamplers)
{
- imageSampler[i].updateSampler(vk::Cast(update->sampler));
+ imageSampler[i].updateSampler(update->sampler);
}
imageSampler[i].device = device;
}
@@ -347,11 +338,14 @@
sw::Texture *texture = &imageSampler[i].texture;
- // "All consecutive bindings updated via a single VkWriteDescriptorSet structure, except those with a
- // descriptorCount of zero, must all either use immutable samplers or must all not use immutable samplers."
- if(!binding.pImmutableSamplers)
+ if(entry.descriptorType == VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER)
{
- imageSampler[i].updateSampler(vk::Cast(update->sampler));
+ // "All consecutive bindings updated via a single VkWriteDescriptorSet structure, except those with a
+ // descriptorCount of zero, must all either use immutable samplers or must all not use immutable samplers."
+ if(!binding.pImmutableSamplers)
+ {
+ imageSampler[i].updateSampler(update->sampler);
+ }
}
imageSampler[i].imageViewId = imageView->id;
diff --git a/src/Vulkan/VkDescriptorSetLayout.hpp b/src/Vulkan/VkDescriptorSetLayout.hpp
index 73535e8..3f52f3b 100644
--- a/src/Vulkan/VkDescriptorSetLayout.hpp
+++ b/src/Vulkan/VkDescriptorSetLayout.hpp
@@ -32,7 +32,7 @@
{
~SampledImageDescriptor() = delete;
- void updateSampler(const vk::Sampler *sampler);
+ void updateSampler(VkSampler sampler);
// TODO(b/129523279): Minimize to the data actually needed.
vk::Sampler sampler;
