Commit 88602e6e by Jamie Madill Committed by Commit Bot

Fix vertex array element limit condition.

Certain overflows wouldn't be detected when the attribute size was less than the attribute size and we were drawing a small number of vertices. Fix this and also set the sentinel value for an integer overflow to be negative maxint instead of -1 to more effectily distinguish the cases. Bug: angleproject:1391 Change-Id: I970d5e2a630c0a84c2c02ac0ac41ab1a395819fe Reviewed-on: https://chromium-review.googlesource.com/1162264 Commit-Queue: Jamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarGeoff Lang <geofflang@chromium.org>
parent 7cc1e556
...@@ -123,22 +123,30 @@ void VertexAttribute::updateCachedElementLimit(const VertexBinding &binding) ...@@ -123,22 +123,30 @@ void VertexAttribute::updateCachedElementLimit(const VertexBinding &binding)
angle::CheckedNumeric<GLint64> elementLimit = angle::CheckedNumeric<GLint64> elementLimit =
(bufferSize - bufferOffset - attribOffset - attribSize); (bufferSize - bufferOffset - attribOffset - attribSize);
if (binding.getStride() > 0) // Use the special integer overflow value if there was a math error.
if (!elementLimit.IsValid())
{ {
angle::CheckedNumeric<GLint64> bindingStride(binding.getStride()); static_assert(kIntegerOverflow < 0, "Unexpected value");
elementLimit /= bindingStride; mCachedElementLimit = kIntegerOverflow;
return;
} }
else
mCachedElementLimit = elementLimit.ValueOrDie();
if (mCachedElementLimit < 0)
{
return;
}
if (binding.getStride() == 0)
{ {
// Special case for a zero stride. If we can fit one vertex we can fit infinite vertices. // Special case for a zero stride. If we can fit one vertex we can fit infinite vertices.
mCachedElementLimit = elementLimit.ValueOrDefault(-1); mCachedElementLimit = std::numeric_limits<GLint64>::max();
if (mCachedElementLimit >= 0)
{
mCachedElementLimit = std::numeric_limits<GLint64>::max();
}
return; return;
} }
angle::CheckedNumeric<GLint64> bindingStride(binding.getStride());
elementLimit /= bindingStride;
if (binding.getDivisor() > 0) if (binding.getDivisor() > 0)
{ {
// For instanced draws, the element count is floor(instanceCount - 1) / binding.divisor. // For instanced draws, the element count is floor(instanceCount - 1) / binding.divisor.
...@@ -149,7 +157,7 @@ void VertexAttribute::updateCachedElementLimit(const VertexBinding &binding) ...@@ -149,7 +157,7 @@ void VertexAttribute::updateCachedElementLimit(const VertexBinding &binding)
elementLimit += bindingDivisor - 1; elementLimit += bindingDivisor - 1;
} }
mCachedElementLimit = elementLimit.ValueOrDefault(-1); mCachedElementLimit = elementLimit.ValueOrDefault(kIntegerOverflow);
} }
size_t ComputeVertexAttributeTypeSize(const VertexAttribute& attrib) size_t ComputeVertexAttributeTypeSize(const VertexAttribute& attrib)
......
...@@ -85,6 +85,9 @@ struct VertexAttribute final : private angle::NonCopyable ...@@ -85,6 +85,9 @@ struct VertexAttribute final : private angle::NonCopyable
GLuint vertexAttribArrayStride; // ONLY for queries of VERTEX_ATTRIB_ARRAY_STRIDE GLuint vertexAttribArrayStride; // ONLY for queries of VERTEX_ATTRIB_ARRAY_STRIDE
GLuint bindingIndex; GLuint bindingIndex;
// Special value for the cached element limit on the integer overflow case.
static constexpr GLint64 kIntegerOverflow = std::numeric_limits<GLint64>::min();
private: private:
// This is kept in sync by the VertexArray. It is used to optimize draw call validation. // This is kept in sync by the VertexArray. It is used to optimize draw call validation.
GLint64 mCachedElementLimit; GLint64 mCachedElementLimit;
......
...@@ -117,9 +117,11 @@ bool ValidateDrawAttribs(Context *context, GLint primcount, GLint maxVertex) ...@@ -117,9 +117,11 @@ bool ValidateDrawAttribs(Context *context, GLint primcount, GLint maxVertex)
return true; return true;
} }
// An overflow can happen when adding the offset. Negative indicates overflow. // An overflow can happen when adding the offset. Check against a special constant.
if (context->getStateCache().getNonInstancedVertexElementLimit() < 0 || if (context->getStateCache().getNonInstancedVertexElementLimit() ==
context->getStateCache().getInstancedVertexElementLimit() < 0) VertexAttribute::kIntegerOverflow ||
context->getStateCache().getInstancedVertexElementLimit() ==
VertexAttribute::kIntegerOverflow)
{ {
ANGLE_VALIDATION_ERR(context, InvalidOperation(), IntegerOverflow); ANGLE_VALIDATION_ERR(context, InvalidOperation(), IntegerOverflow);
return false; return false;
......
...@@ -749,8 +749,12 @@ TEST_P(VertexAttributeTest, SimpleBindAttribLocation) ...@@ -749,8 +749,12 @@ TEST_P(VertexAttributeTest, SimpleBindAttribLocation)
} }
// Verify that drawing with a large out-of-range offset generates INVALID_OPERATION. // Verify that drawing with a large out-of-range offset generates INVALID_OPERATION.
TEST_P(VertexAttributeTest, DrawArraysBufferTooSmall) // Requires an ANGLE or similar implementation that checks buffer bounds.
TEST_P(VertexAttributeTest, ANGLEDrawArraysBufferTooSmall)
{ {
// Test skipped due to supporting GL_KHR_robust_buffer_access_behavior
ANGLE_SKIP_TEST_IF(extensionEnabled("GL_KHR_robust_buffer_access_behavior"));
std::array<GLfloat, kVertexCount> inputData; std::array<GLfloat, kVertexCount> inputData;
std::array<GLfloat, kVertexCount> expectedData; std::array<GLfloat, kVertexCount> expectedData;
InitTestData(inputData, expectedData); InitTestData(inputData, expectedData);
...@@ -764,7 +768,8 @@ TEST_P(VertexAttributeTest, DrawArraysBufferTooSmall) ...@@ -764,7 +768,8 @@ TEST_P(VertexAttributeTest, DrawArraysBufferTooSmall)
} }
// Verify that index draw with an out-of-range offset generates INVALID_OPERATION. // Verify that index draw with an out-of-range offset generates INVALID_OPERATION.
TEST_P(VertexAttributeTest, DrawElementsBufferTooSmall) // Requires an ANGLE or similar implementation that checks buffer bounds.
TEST_P(VertexAttributeTest, ANGLEDrawElementsBufferTooSmall)
{ {
// Test skipped due to supporting GL_KHR_robust_buffer_access_behavior // Test skipped due to supporting GL_KHR_robust_buffer_access_behavior
ANGLE_SKIP_TEST_IF(extensionEnabled("GL_KHR_robust_buffer_access_behavior")); ANGLE_SKIP_TEST_IF(extensionEnabled("GL_KHR_robust_buffer_access_behavior"));
...@@ -781,6 +786,26 @@ TEST_P(VertexAttributeTest, DrawElementsBufferTooSmall) ...@@ -781,6 +786,26 @@ TEST_P(VertexAttributeTest, DrawElementsBufferTooSmall)
EXPECT_GL_ERROR(GL_INVALID_OPERATION); EXPECT_GL_ERROR(GL_INVALID_OPERATION);
} }
TEST_P(VertexAttributeTest, ANGLEDrawArraysOutOfBoundsCases)
{
// Test skipped due to supporting GL_KHR_robust_buffer_access_behavior
ANGLE_SKIP_TEST_IF(extensionEnabled("GL_KHR_robust_buffer_access_behavior"));
initBasicProgram();
GLfloat singleFloat = 1.0f;
GLsizei dataSize = TypeStride(GL_FLOAT);
glBindBuffer(GL_ARRAY_BUFFER, mBuffer);
glBufferData(GL_ARRAY_BUFFER, dataSize, &singleFloat, GL_STATIC_DRAW);
glVertexAttribPointer(mTestAttrib, 2, GL_FLOAT, GL_FALSE, 8, 0);
glEnableVertexAttribArray(mTestAttrib);
glBindBuffer(GL_ARRAY_BUFFER, 0);
drawIndexedQuad(mProgram, "position", 0.5f);
EXPECT_GL_ERROR(GL_INVALID_OPERATION);
}
// Verify that using a different start vertex doesn't mess up the draw. // Verify that using a different start vertex doesn't mess up the draw.
TEST_P(VertexAttributeTest, DrawArraysWithBufferOffset) TEST_P(VertexAttributeTest, DrawArraysWithBufferOffset)
{ {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment