Swapchain: Fixes and improvements validating vkCreateSwapchainKHR().

3 files changed, 119 insertions, 40 deletions

diff --git a/layers/swapchain.cpp b/layers/swapchain.cpp
index 3bebd733..25cec1ad 100644
--- a/layers/swapchain.cpp
+++ b/layers/swapchain.cpp
@@ -1159,7 +1159,10 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkGetPhysicalDeviceSurfacePresent
 // This function does the up-front validation work for vkCreateSwapchainKHR(),
 // and returns VK_TRUE if a logging callback indicates that the call down the
 // chain should be skipped:
-static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCreateInfoKHR* pCreateInfo, VkSwapchainKHR* pSwapchain)
+static VkBool32 validateCreateSwapchainKHR(
+    VkDevice device,
+    const VkSwapchainCreateInfoKHR* pCreateInfo,
+    VkSwapchainKHR* pSwapchain)
 {
 // TODO: Validate cases of re-creating a swapchain (the current code
 // assumes a new swapchain is being created).
@@ -1183,27 +1186,54 @@ static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCre
                          "%s() called even though the %s extension was not enabled for this VkDevice.",
                          fn, VK_KHR_SWAPCHAIN_EXTENSION_NAME );
     }
+    if (!pCreateInfo) {
+        skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                           device,
+                                           "pCreateInfo");
+    } else if (pCreateInfo->sType != VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR) {
+        skipCall |= LOG_ERROR_WRONG_STYPE(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                          device,
+                                          "pCreateInfo",
+                                          "VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR");
+    } else if (pCreateInfo->pNext != NULL) {
+        skipCall |= LOG_ERROR_WRONG_NEXT(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                         device,
+                                         "pCreateInfo");
+    }
+    if (!pSwapchain) {
+        skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                           device,
+                                           "pSwapchain");
+    }
+
+    // Keep around a useful pointer to pPhysicalDevice:
+    SwpPhysicalDevice *pPhysicalDevice = pDevice->pPhysicalDevice;
 
-    // Validate pCreateInfo with the results for previous queries:
-    if (!pDevice->pPhysicalDevice && !pDevice->pPhysicalDevice->gotSurfaceCapabilities) {
+    // Validate pCreateInfo->surface:
+    if (pPhysicalDevice) {
+        // Note: in order to validate, we must lookup layer_data based on the
+        // VkInstance associated with this VkDevice:
+        SwpInstance *pInstance =
+            (pPhysicalDevice) ? pPhysicalDevice->pInstance : NULL;
+        layer_data *my_instance_data =
+            (pInstance) ? get_my_data_ptr(get_dispatch_key(pInstance->instance), layer_data_map) : NULL;
+        skipCall |= validateSurface(my_instance_data,
+                                    pCreateInfo->surface,
+                                    (char *) "vkCreateSwapchainKHR");
+    }
+
+    // Validate pCreateInfo values with the results of
+    // vkGetPhysicalDeviceSurfaceCapabilitiesKHR():
+    if (!pPhysicalDevice || !pPhysicalDevice->gotSurfaceCapabilities) {
         skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "VkDevice",
                               SWAPCHAIN_CREATE_SWAP_WITHOUT_QUERY,
                               "%s() called before calling "
                               "vkGetPhysicalDeviceSurfaceCapabilitiesKHR().",
                               fn);
-    } else {
-        // Validate pCreateInfo->surface, to ensure it is valid (Note: in order
-        // to validate, we must lookup layer_data based on the VkInstance
-        // associated with this VkDevice):
-        SwpPhysicalDevice *pPhysicalDevice = pDevice->pPhysicalDevice;
-        SwpInstance *pInstance = pPhysicalDevice->pInstance;
-        layer_data *my_instance_data = get_my_data_ptr(get_dispatch_key(pInstance->instance), layer_data_map);
-        skipCall |= validateSurface(my_instance_data,
-                                    pCreateInfo->surface,
-                                    (char *) "vkCreateSwapchainKHR");
+    } else if (pCreateInfo) {
         // Validate pCreateInfo->minImageCount against
         // VkSurfaceCapabilitiesKHR::{min|max}ImageCount:
-        VkSurfaceCapabilitiesKHR *pCapabilities = &pDevice->pPhysicalDevice->surfaceCapabilities;
+        VkSurfaceCapabilitiesKHR *pCapabilities = &pPhysicalDevice->surfaceCapabilities;
         if ((pCreateInfo->minImageCount < pCapabilities->minImageCount) ||
             ((pCapabilities->maxImageCount > 0) &&
              (pCreateInfo->minImageCount > pCapabilities->maxImageCount))) {
@@ -1325,7 +1355,8 @@ static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCre
         }
         // Validate pCreateInfo->imageArraySize against
         // VkSurfaceCapabilitiesKHR::maxImageArraySize:
-        if (pCreateInfo->imageArrayLayers > pCapabilities->maxImageArrayLayers) {
+        if ((pCreateInfo->imageArrayLayers > 0) &&
+            (pCreateInfo->imageArrayLayers > pCapabilities->maxImageArrayLayers)) {
             skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "VkDevice",
                                   SWAPCHAIN_CREATE_SWAP_BAD_IMG_ARRAY_SIZE,
                                   "%s() called with a non-supported "
@@ -1350,29 +1381,32 @@ static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCre
                                   pCapabilities->supportedUsageFlags);
         }
     }
-    if (!pDevice->pPhysicalDevice && !pDevice->pPhysicalDevice->surfaceFormatCount) {
+
+    // Validate pCreateInfo values with the results of
+    // vkGetPhysicalDeviceSurfaceFormatsKHR():
+    if (!pPhysicalDevice || !pPhysicalDevice->surfaceFormatCount) {
         skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "VkDevice",
                               SWAPCHAIN_CREATE_SWAP_WITHOUT_QUERY,
                               "%s() called before calling "
                               "vkGetPhysicalDeviceSurfaceFormatsKHR().",
                               fn);
-    } else {
+    } else if (pCreateInfo) {
         // Validate pCreateInfo->imageFormat against
         // VkSurfaceFormatKHR::format:
         bool foundFormat = false;
         bool foundColorSpace = false;
         bool foundMatch = false;
-        for (uint32_t i = 0 ; i < pDevice->pPhysicalDevice->surfaceFormatCount ; i++) {
-            if (pCreateInfo->imageFormat == pDevice->pPhysicalDevice->pSurfaceFormats[i].format) {
+        for (uint32_t i = 0 ; i < pPhysicalDevice->surfaceFormatCount ; i++) {
+            if (pCreateInfo->imageFormat == pPhysicalDevice->pSurfaceFormats[i].format) {
                 // Validate pCreateInfo->imageColorSpace against
                 // VkSurfaceFormatKHR::colorSpace:
                 foundFormat = true;
-                if (pCreateInfo->imageColorSpace == pDevice->pPhysicalDevice->pSurfaceFormats[i].colorSpace) {
+                if (pCreateInfo->imageColorSpace == pPhysicalDevice->pSurfaceFormats[i].colorSpace) {
                     foundMatch = true;
                     break;
                 }
             } else {
-                if (pCreateInfo->imageColorSpace == pDevice->pPhysicalDevice->pSurfaceFormats[i].colorSpace) {
+                if (pCreateInfo->imageColorSpace == pPhysicalDevice->pSurfaceFormats[i].colorSpace) {
                     foundColorSpace = true;
                 }
             }
@@ -1408,18 +1442,21 @@ static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCre
             }
         }
     }
-    if (!pDevice->pPhysicalDevice && !pDevice->pPhysicalDevice->presentModeCount) {
+
+    // Validate pCreateInfo values with the results of
+    // vkGetPhysicalDeviceSurfacePresentModesKHR():
+    if (!pPhysicalDevice || !pPhysicalDevice->presentModeCount) {
         skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "VkDevice",
                               SWAPCHAIN_CREATE_SWAP_WITHOUT_QUERY,
                               "%s() called before calling "
                               "vkGetPhysicalDeviceSurfacePresentModesKHR().",
                               fn);
-    } else {
+    } else if (pCreateInfo) {
         // Validate pCreateInfo->presentMode against
         // vkGetPhysicalDeviceSurfacePresentModesKHR():
         bool foundMatch = false;
-        for (uint32_t i = 0 ; i < pDevice->pPhysicalDevice->presentModeCount ; i++) {
-            if (pDevice->pPhysicalDevice->pPresentModes[i] == pCreateInfo->presentMode) {
+        for (uint32_t i = 0 ; i < pPhysicalDevice->presentModeCount ; i++) {
+            if (pPhysicalDevice->pPresentModes[i] == pCreateInfo->presentMode) {
                 foundMatch = true;
                 break;
             }
@@ -1434,6 +1471,7 @@ static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCre
         }
     }
 
+    // Validate pCreateInfo->imageSharingMode and related values:
     if (pCreateInfo->imageSharingMode == VK_SHARING_MODE_CONCURRENT) {
         if ((pCreateInfo->queueFamilyIndexCount <= 1) ||
             !pCreateInfo->pQueueFamilyIndices) {
@@ -1456,25 +1494,40 @@ static VkBool32 validateCreateSwapchainKHR(VkDevice device, const VkSwapchainCre
                               sharingModeStr(pCreateInfo->imageSharingMode));
     }
 
-    if ((pCreateInfo->clipped != VK_FALSE) &&
+    // Validate pCreateInfo->clipped:
+    if (pCreateInfo &&
+        (pCreateInfo->clipped != VK_FALSE) &&
         (pCreateInfo->clipped != VK_TRUE)) {
-        skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "VkDevice",
+        skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                              device, "VkDevice",
                               SWAPCHAIN_BAD_BOOL,
-                              "%s() called with a VkBool32 value that is neither "
-                              "VK_TRUE nor VK_FALSE, but has the numeric value of %d.",
+                              "%s() called with a VkBool32 value that is "
+                              "neither VK_TRUE nor VK_FALSE, but has the "
+                              "numeric value of %d.",
                               fn,
                               pCreateInfo->clipped);
     }
 
-    if (pCreateInfo->oldSwapchain) {
-        SwpSwapchain *pSwapchain = &my_data->swapchainMap[pCreateInfo->oldSwapchain];
-        if (pSwapchain) {
-            if (device != pSwapchain->pDevice->device) {
-                LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "VkDevice",
-                          SWAPCHAIN_DESTROY_SWAP_DIFF_DEVICE,
-                          "%s() called with a different VkDevice than the "
-                          "VkSwapchainKHR was created with.",
-                          __FUNCTION__);
+    // Validate pCreateInfo->oldSwapchain:
+    if (pCreateInfo && pCreateInfo->oldSwapchain) {
+        SwpSwapchain *pOldSwapchain = &my_data->swapchainMap[pCreateInfo->oldSwapchain];
+        if (pOldSwapchain) {
+            if (device != pOldSwapchain->pDevice->device) {
+                skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                      device, "VkDevice",
+                                      SWAPCHAIN_DESTROY_SWAP_DIFF_DEVICE,
+                                      "%s() called with a different VkDevice "
+                                      "than the VkSwapchainKHR was created with.",
+                                      __FUNCTION__);
+            }
+            if (pCreateInfo->surface != pOldSwapchain->surface) {
+                skipCall |= LOG_ERROR(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                      device, "VkDevice",
+                                      SWAPCHAIN_CREATE_SWAP_DIFF_SURFACE,
+                                      "%s() called with pCreateInfo->oldSwapchain "
+                                      "that has a different VkSurfaceKHR than "
+                                      "pCreateInfo->surface.",
+                                      fn);
             }
         } else {
             skipCall |= LOG_ERROR_NON_VALID_OBJ(VK_DEBUG_REPORT_OBJECT_TYPE_SWAPCHAIN_KHR_EXT,
@@ -1510,6 +1563,8 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateSwapchainKHR(
             pDevice->swapchains[*pSwapchain] =
                 &my_data->swapchainMap[*pSwapchain];
             my_data->swapchainMap[*pSwapchain].pDevice = pDevice;
+            my_data->swapchainMap[*pSwapchain].surface =
+                (pCreateInfo) ? pCreateInfo->surface : 0;
             my_data->swapchainMap[*pSwapchain].imageCount = 0;
         }
 
@@ -1600,8 +1655,8 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkGetSwapchainImagesKHR(
                                             "VkSwapchainKHR");
     }
     if (!pSwapchainImageCount) {
-        skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT,
-                                           physicalDevice,
+        skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT,
+                                           device,
                                            "pSwapchainImageCount");
     }
 
diff --git a/layers/swapchain.h b/layers/swapchain.h
index 4df160ec..55172d9a 100644
--- a/layers/swapchain.h
+++ b/layers/swapchain.h
@@ -78,12 +78,15 @@ typedef enum _SWAPCHAIN_ERROR
     SWAPCHAIN_CREATE_SWAP_BAD_PRESENT_MODE,     // Called vkCreateSwapchainKHR() with a non-supported presentMode
     SWAPCHAIN_CREATE_SWAP_BAD_SHARING_MODE,     // Called vkCreateSwapchainKHR() with a non-supported imageSharingMode
     SWAPCHAIN_CREATE_SWAP_BAD_SHARING_VALUES,   // Called vkCreateSwapchainKHR() with bad values when imageSharingMode is VK_SHARING_MODE_CONCURRENT
+    SWAPCHAIN_CREATE_SWAP_DIFF_SURFACE,         // Called vkCreateSwapchainKHR() with pCreateInfo->oldSwapchain that has a different surface than pCreateInfo->surface
     SWAPCHAIN_DESTROY_SWAP_DIFF_DEVICE,         // Called vkDestroySwapchainKHR() with a different VkDevice than vkCreateSwapchainKHR()
     SWAPCHAIN_APP_OWNS_TOO_MANY_IMAGES,         // vkAcquireNextImageKHR() asked for more images than are available
     SWAPCHAIN_INDEX_TOO_LARGE,                  // Index is too large for swapchain
     SWAPCHAIN_INDEX_NOT_IN_USE,                 // vkQueuePresentKHR() given index that is not owned by app
     SWAPCHAIN_BAD_BOOL,                         // VkBool32 that doesn't have value of VK_TRUE or VK_FALSE (e.g. is a non-zero form of true)
     SWAPCHAIN_INVALID_COUNT,                    // Second time a query called, the pCount value didn't match first time
+    SWAPCHAIN_WRONG_STYPE,                      // The sType for a struct has the wrong value
+    SWAPCHAIN_WRONG_NEXT,                       // The pNext for a struct is not NULL
 } SWAPCHAIN_ERROR;
 
 
@@ -109,6 +112,21 @@ typedef enum _SWAPCHAIN_ERROR
                 "the value (%d) that was returned when %s was NULL.",   \
                 __FUNCTION__, (obj2), (obj), (val), (obj2))             \
     : VK_FALSE
+#define LOG_ERROR_WRONG_STYPE(objType, type, obj, val)          \
+    (my_data) ?                                                         \
+        log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (objType), \
+                (uint64_t) (obj), 0, SWAPCHAIN_WRONG_STYPE, LAYER_NAME, \
+                "%s() called with the wrong value for %s->sType "       \
+                "(expected %s).",                                       \
+                __FUNCTION__, (obj), (val))                             \
+    : VK_FALSE
+#define LOG_ERROR_WRONG_NEXT(objType, type, obj)          \
+    (my_data) ?                                                         \
+        log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (objType), \
+                (uint64_t) (obj), 0, SWAPCHAIN_WRONG_NEXT, LAYER_NAME, \
+                "%s() called with non-NULL value for %s->pNext.",       \
+                __FUNCTION__, (obj))                                    \
+    : VK_FALSE
 #define LOG_ERROR(objType, type, obj, enm, fmt, ...)                    \
     (my_data) ?                                                         \
         log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (objType), \
@@ -241,6 +259,9 @@ struct _SwpSwapchain {
     // Corresponding VkDevice (and info) to this VkSwapchainKHR:
     SwpDevice *pDevice;
 
+    // Corresponding VkSurfaceKHR to this VkSwapchainKHR:
+    VkSurfaceKHR surface;
+
     // When vkGetSwapchainImagesKHR is called, the VkImage's are
     // remembered:
     uint32_t imageCount;
diff --git a/layers/vk_validation_layer_details.md b/layers/vk_validation_layer_details.md
index 38b43d33..a5977de9 100644
--- a/layers/vk_validation_layer_details.md
+++ b/layers/vk_validation_layer_details.md
@@ -350,12 +350,15 @@ This layer is a work in progress. VK_LAYER_LUNARG_swapchain layer is intended to
 | vkCreateSwapchainKHR(pCreateInfo->presentMode) | Validates vkCreateSwapchainKHR(pCreateInfo->presentMode) | CREATE_SWAP_BAD_PRESENT_MODE | vkCreateSwapchainKHR | NA | None |
 | vkCreateSwapchainKHR(pCreateInfo->imageSharingMode) | Validates vkCreateSwapchainKHR(pCreateInfo->imageSharingMode) | CREATE_SWAP_BAD_SHARING_MODE | vkCreateSwapchainKHR | NA | None |
 | vkCreateSwapchainKHR(pCreateInfo->imageSharingMode) | Validates vkCreateSwapchainKHR(pCreateInfo->imageSharingMode) | CREATE_SWAP_BAD_SHARING_VALUES | vkCreateSwapchainKHR | NA | None |
+| vkCreateSwapchainKHR(pCreateInfo->oldSwapchain and pCreateInfo->surface) | pCreateInfo->surface must match pCreateInfo->oldSwapchain's surface | CREATE_SWAP_DIFF_SURFACE | vkCreateSwapchainKHR | NA | None |
 | Use same device for swapchain | Validates that vkDestroySwapchainKHR() called with the same VkDevice as vkCreateSwapchainKHR() | DESTROY_SWAP_DIFF_DEVICE | vkDestroySwapchainKHR | NA | None |
 | Don't use too many images | Validates that app never tries to own too many swapchain images at a time | APP_OWNS_TOO_MANY_IMAGES | vkAcquireNextImageKHR | NA | None |
 | Index too large | Validates that an image index is within the number of images in a swapchain | INDEX_TOO_LARGE | vkQueuePresentKHR | NA | None |
 | Can't present a non-owned image | Validates that application only presents images that it owns | INDEX_NOT_IN_USE | vkQueuePresentKHR | NA | None |
 | A VkBool32 must have values of VK_TRUE or VK_FALSE | Validates that a VkBool32 must have values of VK_TRUE or VK_FALSE | BAD_BOOL | vkCreateSwapchainKHR | NA | None |
 | pCount must point to same value regardless of whether other pointer is NULL | Validates that app doesn't change value of pCount returned by a query | INVALID_COUNT | vkGetPhysicalDeviceSurfaceFormatsKHR vkGetPhysicalDeviceSurfacePresentModesKHR vkGetSwapchainImagesKHR | NA | None |
+| Valid sType | Validates that a struct has correct value for sType | WRONG_STYPE | vkCreateSwapchainKHR | NA | None |
+| Valid pNext | Validates that a struct has NULL for the value of pNext | WRONG_NEXT | vkCreateSwapchainKHR | NA | None |
 
 ### VK_LAYER_LUNARG_swapchain Pending Work
 Additional checks to be added to VK_LAYER_LUNARG_swapchain