<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 10, 2016 at 6:03 PM, Nanley Chery <span dir="ltr"><<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>><br>
<br>
Add guards to prevent dereferencing NULL dynamic pipeline state. Asserts<br>
of pCreateInfo members are moved to the earliest points at which they<br>
should not be NULL.<br>
<span class=""><br>
This fixes a segfault seen in the McNopper demo, VKTS_Example09.<br>
<br>
</span>v2: Fix disabled rasterization check (Jason Ekstrand)<br>
<span class=""><br>
Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>><br>
Cc: "12.0" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>><br>
---<br>
<br>
</span>v2: Also guard scissor state<br>
Move pColorBlend assert to earliest point known to be non-NULL<br>
Update commit message to mention changes with asserts<br>
<br>
src/intel/vulkan/anv_pipeline.c | 65 +++++++++++++++++++++++++++--------------<br>
1 file changed, 43 insertions(+), 22 deletions(-)<br>
<br>
diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c<br>
index ae03787..b15296a 100644<br>
--- a/src/intel/vulkan/anv_pipeline.c<br>
+++ b/src/intel/vulkan/anv_pipeline.c<br>
@@ -979,18 +979,27 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,<br>
<span class=""><br>
struct anv_dynamic_state *dynamic = &pipeline->dynamic_state;<br>
<br>
- dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount;<br>
- if (states & (1 << VK_DYNAMIC_STATE_VIEWPORT)) {<br>
- typed_memcpy(dynamic->viewport.viewports,<br>
- pCreateInfo->pViewportState->pViewports,<br>
- pCreateInfo->pViewportState->viewportCount);<br>
</span>- }<br>
<span class="">+ /* Section 9.2 of the Vulkan 1.0.15 spec says:<br>
+ *<br>
+ * pViewportState is [...] NULL if the pipeline<br>
+ * has rasterization disabled.<br>
+ */<br>
</span>+ if (pCreateInfo->pRasterizationState->rasterizerDiscardEnable) {<br></blockquote><div><br></div><div>I think you have the condition backwards. Did you mean !rasterizerDiscardEnable?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ assert(pCreateInfo->pViewportState);<br>
+<br>
<span class="">+ dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount;<br>
+ if (states & (1 << VK_DYNAMIC_STATE_VIEWPORT)) {<br>
+ typed_memcpy(dynamic->viewport.viewports,<br>
+ pCreateInfo->pViewportState->pViewports,<br>
+ pCreateInfo->pViewportState->viewportCount);<br>
+ }<br>
<br>
</span>- dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount;<br>
- if (states & (1 << VK_DYNAMIC_STATE_SCISSOR)) {<br>
- typed_memcpy(dynamic->scissor.scissors,<br>
- pCreateInfo->pViewportState->pScissors,<br>
- pCreateInfo->pViewportState->scissorCount);<br>
<span class="">+ dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount;<br>
</span>+ if (states & (1 << VK_DYNAMIC_STATE_SCISSOR)) {<br>
+ typed_memcpy(dynamic->scissor.scissors,<br>
+ pCreateInfo->pViewportState->pScissors,<br>
+ pCreateInfo->pViewportState->scissorCount);<br>
+ }<br>
}<br></blockquote><div><br></div><div>We may want a sensible else case. Probably best to just set scissor and viewport count to zero.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
if (states & (1 << VK_DYNAMIC_STATE_LINE_WIDTH)) {<br>
@@ -1008,10 +1017,22 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,<br>
<span class=""> pCreateInfo->pRasterizationState->depthBiasSlopeFactor;<br>
}<br>
<br>
- if (states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) {<br>
+ /* Section 9.2 of the Vulkan 1.0.15 spec says:<br>
+ *<br>
+ * pColorBlendState is [...] NULL if the pipeline has rasterization<br>
+ * disabled or if the subpass of the render pass the pipeline is<br>
+ * created against does not use any color attachments.<br>
+ */<br>
</span>+ unsigned num_color_att_unused = 0;<br>
+ while (num_color_att_unused < subpass->color_count &&<br>
+ subpass->color_attachments[num_color_att_unused++] == VK_ATTACHMENT_UNUSED);<br></blockquote><div><br></div><div>While the while loop here certainly is clever, it took me a while to figure out what it's doing. How about something like<br><br></div><div>bool writes_color = false;<br></div><div>for (unsigned i = 0; i < subpass->color_count; i++) {<br></div><div> if (subpass->color_attachments[i] != VK_ATTACHMENT_UNUSED) {<br></div><div> writes_color = true;<br></div><div> break;<br> }<br></div><div>}<br><br></div><div>That's a good deal more obvious.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (num_color_att_unused < subpass->color_count &&<br>
+ pCreateInfo->pRasterizationState->rasterizerDiscardEnable) {<br>
assert(pCreateInfo->pColorBlendState);<br>
- typed_memcpy(dynamic->blend_constants,<br>
- pCreateInfo->pColorBlendState->blendConstants, 4);<br>
+<br>
+ if (states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS))<br>
+ typed_memcpy(dynamic->blend_constants,<br>
+ pCreateInfo->pColorBlendState->blendConstants, 4);<br>
}<br>
<br>
/* If there is no depthstencil attachment, then don't read<br>
@@ -1020,14 +1041,17 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,<br>
<span class=""> * no need to override the depthstencil defaults in<br>
* anv_pipeline::dynamic_state when there is no depthstencil attachment.<br>
*<br>
- * From the Vulkan spec (20 Oct 2015, git-aa308cb):<br>
+ * Section 9.2 of the Vulkan 1.0.15 spec says:<br>
*<br>
- * pDepthStencilState [...] may only be NULL if renderPass and subpass<br>
- * specify a subpass that has no depth/stencil attachment.<br>
+ * pDepthStencilState is [...] NULL if the pipeline has rasterization<br>
+ * disabled or if the subpass of the render pass the pipeline is created<br>
+ * against does not use a depth/stencil attachment.<br>
*/<br>
- if (subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {<br>
</span>+ if (pCreateInfo->pRasterizationState->rasterizerDiscardEnable &&<br></blockquote><div><br></div><div>Again, !rasterizerDiscardEnable<br><br></div><div>Other than the above comments, it seems reasonable. Did you run it through Jenkins? I'm kind-of surprised that the CTS didn't catch the flipped rasterizerDiscardEnable flag. It's possible that it only ever uses dynamic states but that would be silly.<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {<br>
<span class="">+ assert(pCreateInfo->pDepthStencilState);<br>
+<br>
if (states & (1 << VK_DYNAMIC_STATE_DEPTH_BOUNDS)) {<br>
- assert(pCreateInfo->pDepthStencilState);<br>
dynamic->depth_bounds.min =<br>
pCreateInfo->pDepthStencilState->minDepthBounds;<br>
dynamic->depth_bounds.max =<br>
</span>@@ -1035,7 +1059,6 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,<br>
<span class=""> }<br>
<br>
if (states & (1 << VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK)) {<br>
- assert(pCreateInfo->pDepthStencilState);<br>
dynamic->stencil_compare_mask.front =<br>
pCreateInfo->pDepthStencilState->front.compareMask;<br>
dynamic->stencil_compare_mask.back =<br>
</span>@@ -1043,7 +1066,6 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,<br>
<span class=""> }<br>
<br>
if (states & (1 << VK_DYNAMIC_STATE_STENCIL_WRITE_MASK)) {<br>
- assert(pCreateInfo->pDepthStencilState);<br>
dynamic->stencil_write_mask.front =<br>
pCreateInfo->pDepthStencilState->front.writeMask;<br>
dynamic->stencil_write_mask.back =<br>
</span>@@ -1051,7 +1073,6 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,<br>
<span class=""> }<br>
<br>
if (states & (1 << VK_DYNAMIC_STATE_STENCIL_REFERENCE)) {<br>
- assert(pCreateInfo->pDepthStencilState);<br>
dynamic->stencil_reference.front =<br>
pCreateInfo->pDepthStencilState->front.reference;<br>
dynamic->stencil_reference.back =<br>
--<br>
2.8.3<br>
<br>
</span>_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div><br></div></div>