Associate bug IDs with unimplemented functionality
Also correct uses of UNIMPLEMENTED() for functionality that is not
advertised as supported, and add spec quotes to justify and document
them.
This change also enforces requiring that the UNIMPLEMENTED() macro's
format string starts with a bug reference of the form b/###. Note we
still have to mention the bug ID in a FIXME() comment as well for
'go/todo' issue tracking (until b/148221114 gets fixed).
Bug: b/131243109
Change-Id: Ieb28e31f2c6c80a27b833626538e7e223b5fda08
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/40509
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Presubmit: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Device/Blitter.cpp b/src/Device/Blitter.cpp
index 946eafc..611df31 100644
--- a/src/Device/Blitter.cpp
+++ b/src/Device/Blitter.cpp
@@ -1747,11 +1747,11 @@
return;
}
- if((region.srcSubresource.layerCount != region.dstSubresource.layerCount) ||
- (region.srcSubresource.aspectMask != region.dstSubresource.aspectMask))
- {
- UNIMPLEMENTED("region");
- }
+ // Vulkan 1.2 section 18.5. Image Copies with Scaling:
+ // "The layerCount member of srcSubresource and dstSubresource must match"
+ // "The aspectMask member of srcSubresource and dstSubresource must match"
+ ASSERT(region.srcSubresource.layerCount == region.dstSubresource.layerCount);
+ ASSERT(region.srcSubresource.aspectMask == region.dstSubresource.aspectMask);
if(region.dstOffsets[0].x > region.dstOffsets[1].x)
{
@@ -1890,10 +1890,9 @@
ASSERT(state.sourceFormat == state.destFormat);
ASSERT(state.srcSamples == state.destSamples);
- if(state.srcSamples != 1)
- {
- UNIMPLEMENTED("state.srcSamples %d", state.srcSamples);
- }
+ // Vulkan 1.2: "If samples is not VK_SAMPLE_COUNT_1_BIT, then imageType must be
+ // VK_IMAGE_TYPE_2D, flags must not contain VK_IMAGE_CREATE_CUBE_COMPATIBLE_BIT"
+ ASSERT(state.srcSamples == 1);
CornerUpdateFunction function;
{
@@ -1922,11 +1921,7 @@
void Blitter::updateBorders(vk::Image *image, const VkImageSubresourceLayers &subresourceLayers)
{
- if(image->getArrayLayers() < (subresourceLayers.baseArrayLayer + 6))
- {
- UNIMPLEMENTED("image->getArrayLayers() %d, baseArrayLayer %d",
- image->getArrayLayers(), subresourceLayers.baseArrayLayer);
- }
+ ASSERT(image->getArrayLayers() >= (subresourceLayers.baseArrayLayer + 6));
// From Vulkan 1.1 spec, section 11.5. Image Views:
// "For cube and cube array image views, the layers of the image view starting
@@ -1980,10 +1975,9 @@
VkSampleCountFlagBits samples = image->getSampleCountFlagBits();
State state(format, format, samples, samples, Options{ 0xF });
- if(samples != VK_SAMPLE_COUNT_1_BIT)
- {
- UNIMPLEMENTED("Multi-sampled cube: %d samples", static_cast<int>(samples));
- }
+ // Vulkan 1.2: "If samples is not VK_SAMPLE_COUNT_1_BIT, then imageType must be
+ // VK_IMAGE_TYPE_2D, flags must not contain VK_IMAGE_CREATE_CUBE_COMPATIBLE_BIT"
+ ASSERT(samples == VK_SAMPLE_COUNT_1_BIT);
auto cornerUpdateRoutine = getCornerUpdateRoutine(state);
if(!cornerUpdateRoutine)
diff --git a/src/Pipeline/SpirvShaderControlFlow.cpp b/src/Pipeline/SpirvShaderControlFlow.cpp
index f70a29c..6139002 100644
--- a/src/Pipeline/SpirvShaderControlFlow.cpp
+++ b/src/Pipeline/SpirvShaderControlFlow.cpp
@@ -581,13 +581,13 @@
{
if(insnNumber > 1)
{
- UNIMPLEMENTED("Function block number of instructions: %d", insnNumber);
+ UNIMPLEMENTED("b/141246700: Function block number of instructions: %d", insnNumber); // FIXME(b/141246700)
return EmitResult::Continue;
}
if(blockInsn.opcode() != wrapOpKill[insnNumber++])
{
- UNIMPLEMENTED("Function block instruction %d : %s", insnNumber - 1, OpcodeName(blockInsn.opcode()).c_str());
+ UNIMPLEMENTED("b/141246700: Function block instruction %d : %s", insnNumber - 1, OpcodeName(blockInsn.opcode()).c_str()); // FIXME(b/141246700)
return EmitResult::Continue;
}
diff --git a/src/Pipeline/SpirvShaderMemory.cpp b/src/Pipeline/SpirvShaderMemory.cpp
index 8fcd939..c656216 100644
--- a/src/Pipeline/SpirvShaderMemory.cpp
+++ b/src/Pipeline/SpirvShaderMemory.cpp
@@ -225,7 +225,7 @@
Object::ID initializerId = insn.word(4);
if(getObject(initializerId).kind != Object::Kind::Constant)
{
- UNIMPLEMENTED("Non-constant initializers not yet implemented");
+ UNIMPLEMENTED("b/148241854: Non-constant initializers not yet implemented"); // FIXME(b/148241854)
}
switch(objectTy.storageClass)
diff --git a/src/Vulkan/VkDebug.hpp b/src/Vulkan/VkDebug.hpp
index b0287f8..801a9ad 100644
--- a/src/Vulkan/VkDebug.hpp
+++ b/src/Vulkan/VkDebug.hpp
@@ -20,6 +20,8 @@
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
+
+#include <cctype>
#include <string>
#if !defined(TRACE_OUTPUT_FILE)
@@ -110,11 +112,13 @@
} \
} while(0)
-// A macro to indicate functionality currently unimplemented for a feature
-// advertised as supported. For unsupported features not advertised as supported
-// use UNSUPPORTED().
+// A macro to indicate functionality currently unimplemented, for a feature advertised
+// as supported. Since this is a bug, a bug ID must be provided, in b/### format.
+// For unimplemented functionality not advertised as supported, use UNSUPPORTED() instead.
#undef UNIMPLEMENTED
-#define UNIMPLEMENTED(format, ...) DABORT("UNIMPLEMENTED: " format, ##__VA_ARGS__)
+#define UNIMPLEMENTED(format, ...) \
+ DABORT("UNIMPLEMENTED: " format, ##__VA_ARGS__); \
+ static_assert(format[0] == 'b' && format[1] == '/' && format[2] >= '0' && format[2] <= '9', "explanation must start with bug reference in b/### format")
// A macro to indicate unsupported functionality.
// This should be called when a Vulkan / SPIR-V feature is attempted to be used,
diff --git a/src/Vulkan/VkDeviceMemoryExternalAndroid.hpp b/src/Vulkan/VkDeviceMemoryExternalAndroid.hpp
index 6cd2c93..853d144 100644
--- a/src/Vulkan/VkDeviceMemoryExternalAndroid.hpp
+++ b/src/Vulkan/VkDeviceMemoryExternalAndroid.hpp
@@ -13,6 +13,8 @@
// limitations under the License.
#include "VkDebug.hpp"
+#include "VkStringify.hpp"
+
#include <android/hardware_buffer.h>
#include <errno.h>
@@ -55,7 +57,7 @@
if(exportInfo->handleTypes != VK_EXTERNAL_MEMORY_HANDLE_TYPE_ANDROID_HARDWARE_BUFFER_BIT_ANDROID)
{
- UNIMPLEMENTED("exportInfo->handleTypes");
+ UNSUPPORTED("VkExportMemoryAllocateInfo::handleTypes %d", int(exportInfo->handleTypes));
}
exportAhb = true;
}
@@ -70,7 +72,8 @@
}
break;
- default:;
+ default:
+ WARN("VkMemoryAllocateInfo->pNext sType = %s", vk::Stringify(createInfo->sType).c_str());
}
createInfo = createInfo->pNext;
}
@@ -146,7 +149,7 @@
static VkResult getAhbProperties(const struct AHardwareBuffer *buffer, VkAndroidHardwareBufferPropertiesANDROID *pProperties)
{
- UNIMPLEMENTED("getAhbProperties");
+ UNIMPLEMENTED("b/141698760: getAhbProperties"); // FIXME(b/141698760)
return VK_SUCCESS;
}
diff --git a/src/Vulkan/VkDeviceMemoryExternalLinux.hpp b/src/Vulkan/VkDeviceMemoryExternalLinux.hpp
index 89dd4f9..3a9a768 100644
--- a/src/Vulkan/VkDeviceMemoryExternalLinux.hpp
+++ b/src/Vulkan/VkDeviceMemoryExternalLinux.hpp
@@ -13,6 +13,7 @@
// limitations under the License.
#include "VkDebug.hpp"
+#include "VkStringify.hpp"
#include "System/Linux/MemFd.hpp"
#include <errno.h>
@@ -47,7 +48,7 @@
if(importInfo->handleType != VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT)
{
- UNIMPLEMENTED("importInfo->handleType");
+ UNSUPPORTED("VkImportMemoryFdInfoKHR::handleType %d", int(importInfo->handleType));
}
importFd = true;
fd = importInfo->fd;
@@ -59,13 +60,14 @@
if(exportInfo->handleTypes != VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT)
{
- UNIMPLEMENTED("exportInfo->handleTypes");
+ UNSUPPORTED("VkExportMemoryAllocateInfo::handleTypes %d", int(exportInfo->handleTypes));
}
exportFd = true;
}
break;
- default:;
+ default:
+ WARN("VkMemoryAllocateInfo->pNext sType = %s", vk::Stringify(createInfo->sType).c_str());
}
createInfo = createInfo->pNext;
}
diff --git a/src/Vulkan/VkImage.cpp b/src/Vulkan/VkImage.cpp
index 2a462ca..d538733 100644
--- a/src/Vulkan/VkImage.cpp
+++ b/src/Vulkan/VkImage.cpp
@@ -892,21 +892,15 @@
void Image::clear(const VkClearColorValue &color, const VkImageSubresourceRange &subresourceRange)
{
- if(!(subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT))
- {
- UNIMPLEMENTED("aspectMask");
- }
+ ASSERT(subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT);
device->getBlitter()->clear((void *)color.float32, getClearFormat(), this, format, subresourceRange);
}
void Image::clear(const VkClearDepthStencilValue &color, const VkImageSubresourceRange &subresourceRange)
{
- if((subresourceRange.aspectMask & ~(VK_IMAGE_ASPECT_DEPTH_BIT |
- VK_IMAGE_ASPECT_STENCIL_BIT)) != 0)
- {
- UNIMPLEMENTED("aspectMask");
- }
+ ASSERT((subresourceRange.aspectMask & ~(VK_IMAGE_ASPECT_DEPTH_BIT |
+ VK_IMAGE_ASPECT_STENCIL_BIT)) == 0);
if(subresourceRange.aspectMask & VK_IMAGE_ASPECT_DEPTH_BIT)
{
@@ -925,12 +919,9 @@
void Image::clear(const VkClearValue &clearValue, const vk::Format &viewFormat, const VkRect2D &renderArea, const VkImageSubresourceRange &subresourceRange)
{
- if(!((subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT) ||
- (subresourceRange.aspectMask & (VK_IMAGE_ASPECT_DEPTH_BIT |
- VK_IMAGE_ASPECT_STENCIL_BIT))))
- {
- UNIMPLEMENTED("subresourceRange");
- }
+ ASSERT((subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT) ||
+ (subresourceRange.aspectMask & (VK_IMAGE_ASPECT_DEPTH_BIT |
+ VK_IMAGE_ASPECT_STENCIL_BIT)));
if(subresourceRange.aspectMask == VK_IMAGE_ASPECT_COLOR_BIT)
{
diff --git a/src/Vulkan/VkImageView.cpp b/src/Vulkan/VkImageView.cpp
index 3142cfb..fc0c8bd 100644
--- a/src/Vulkan/VkImageView.cpp
+++ b/src/Vulkan/VkImageView.cpp
@@ -75,6 +75,7 @@
{
}
+// Vulkan 1.2 Table 8. Image and image view parameter compatibility requirements
bool ImageView::imageTypesMatch(VkImageType imageType) const
{
uint32_t imageArrayLayers = image->getArrayLayers();
@@ -118,15 +119,8 @@
{
// Note: clearing ignores swizzling, so components is ignored.
- if(!imageTypesMatch(image->getImageType()))
- {
- UNIMPLEMENTED("imageTypesMatch");
- }
-
- if(!format.isCompatible(image->getFormat()))
- {
- UNIMPLEMENTED("incompatible formats");
- }
+ ASSERT(imageTypesMatch(image->getImageType()));
+ ASSERT(format.isCompatible(image->getFormat()));
VkImageSubresourceRange sr = subresourceRange;
sr.aspectMask = aspectMask;
@@ -137,15 +131,8 @@
{
// Note: clearing ignores swizzling, so components is ignored.
- if(!imageTypesMatch(image->getImageType()))
- {
- UNIMPLEMENTED("imageTypesMatch");
- }
-
- if(!format.isCompatible(image->getFormat()))
- {
- UNIMPLEMENTED("incompatible formats");
- }
+ ASSERT(imageTypesMatch(image->getImageType()));
+ ASSERT(format.isCompatible(image->getFormat()));
VkImageSubresourceRange sr;
sr.aspectMask = aspectMask;
@@ -173,7 +160,7 @@
{
if((subresourceRange.levelCount != 1) || (resolveAttachment->subresourceRange.levelCount != 1))
{
- UNIMPLEMENTED("levelCount");
+ UNIMPLEMENTED("b/148242443: levelCount != 1"); // FIXME(b/148242443)
}
VkImageCopy region;
@@ -201,7 +188,7 @@
{
if((subresourceRange.levelCount != 1) || (resolveAttachment->subresourceRange.levelCount != 1))
{
- UNIMPLEMENTED("levelCount");
+ UNIMPLEMENTED("b/148242443: levelCount != 1"); // FIXME(b/148242443)
}
VkImageCopy region;
diff --git a/src/Vulkan/VkQueryPool.cpp b/src/Vulkan/VkQueryPool.cpp
index 07b29d7..30000f7 100644
--- a/src/Vulkan/VkQueryPool.cpp
+++ b/src/Vulkan/VkQueryPool.cpp
@@ -189,7 +189,7 @@
if(flags != 0)
{
- UNIMPLEMENTED("vkCmdBeginQuery::flags %d", int(flags));
+ UNSUPPORTED("vkCmdBeginQuery::flags %d", int(flags));
}
pool[query].prepare(type);
diff --git a/src/Vulkan/VkSemaphore.cpp b/src/Vulkan/VkSemaphore.cpp
index 31eb5fd..72bee8b 100644
--- a/src/Vulkan/VkSemaphore.cpp
+++ b/src/Vulkan/VkSemaphore.cpp
@@ -15,6 +15,7 @@
#include "VkSemaphore.hpp"
#include "VkConfig.h"
+#include "VkStringify.hpp"
#if SWIFTSHADER_EXTERNAL_SEMAPHORE_OPAQUE_FD
# if defined(__linux__) || defined(__ANDROID__)
@@ -50,15 +51,20 @@
for(const auto *nextInfo = reinterpret_cast<const VkBaseInStructure *>(pCreateInfo->pNext);
nextInfo != nullptr; nextInfo = nextInfo->pNext)
{
- if(nextInfo->sType == VK_STRUCTURE_TYPE_EXPORT_SEMAPHORE_CREATE_INFO)
+ switch(nextInfo->sType)
{
- const auto *exportInfo = reinterpret_cast<const VkExportSemaphoreCreateInfo *>(nextInfo);
- exportSemaphore = true;
- if(exportInfo->handleTypes != Semaphore::External::kExternalSemaphoreHandleType)
+ case VK_STRUCTURE_TYPE_EXPORT_SEMAPHORE_CREATE_INFO:
{
- UNIMPLEMENTED("exportInfo->handleTypes");
+ const auto *exportInfo = reinterpret_cast<const VkExportSemaphoreCreateInfo *>(nextInfo);
+ exportSemaphore = true;
+ if(exportInfo->handleTypes != Semaphore::External::kExternalSemaphoreHandleType)
+ {
+ UNSUPPORTED("exportInfo->handleTypes %d", int(exportInfo->handleTypes));
+ }
}
break;
+ default:
+ WARN("nextInfo->sType = %s", vk::Stringify(nextInfo->sType).c_str());
}
}
}
diff --git a/src/Vulkan/libVulkan.cpp b/src/Vulkan/libVulkan.cpp
index 6478702..f2146b1 100644
--- a/src/Vulkan/libVulkan.cpp
+++ b/src/Vulkan/libVulkan.cpp
@@ -273,7 +273,7 @@
if(pCreateInfo->enabledLayerCount != 0)
{
- UNIMPLEMENTED("pCreateInfo->enabledLayerCount != 0");
+ UNIMPLEMENTED("b/148240133: pCreateInfo->enabledLayerCount != 0"); // FIXME(b/148240133)
}
uint32_t extensionPropertiesCount = sizeof(instanceExtensionProperties) / sizeof(instanceExtensionProperties[0]);
