-
Notifications
You must be signed in to change notification settings - Fork 569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
spirv-val: Validate zero product workgroup size #4828
Conversation
This fails the ASAN build. For example: [ RUN ] DecorationTest.WorkgroupSizeKernel==2725==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000e8f80 at pc 0x000001382022 bp 0x7ffd882c5c70 sp 0x7ffd882c5c68 0x6020000e8f80 is located 0 bytes to the right of 16-byte region [0x6020000e8f70,0x6020000e8f80) SUMMARY: AddressSanitizer: heap-buffer-overflow /tmpfs/src/github/SPIRV-Tools/build/../source/val/instruction.h:66:46 in spvtools::val::Instruction::word(unsigned long) const
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this forward, but the check has problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the value check should be moved to validate_builtins.cpp.
} | ||
if (_.HasCapability(SpvCapabilityShader) && | ||
inst->GetOperandAs<SpvBuiltIn>(2) == SpvBuiltInWorkgroupSize) { | ||
if (target->opcode() != SpvOpVariable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check could be greatly improved or just moved into the validate_builtins.cpp
as well but for sake of not overloading this PR too much will leave for a post-PR fix
@@ -3123,16 +3123,6 @@ spv_result_t BuiltInsValidator::ValidateI32Vec4InputAtDefinition( | |||
|
|||
spv_result_t BuiltInsValidator::ValidateWorkgroupSizeAtDefinition( | |||
const Decoration& decoration, const Instruction& inst) { | |||
if (spvIsVulkanEnv(_.context()->target_env)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type of the constant is not checked. So someone could feed a bad module and still get a buffer overrund, I think.
In Vulkan we have the VUID
The variable decorated with WorkgroupSize must be declared as a three-component vector of 32-bit integer values
I tried to write a Kernel shader but I can't think of how it could use a Builtin WorkgroupSize
without an ivec3
so I changed this to a global SPIR-V check
This PR has gone stale. I will close it. Open a new PR if needed. |
closes #4801
Created a
EntryPointData
reduce number of hash maps with theentry_point_id
as the key