Skip to content
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

layers: Fix PIPELINE_STATE::GetStageStates bug #4690

Closed

Conversation

spencer-lunarg
Copy link
Contributor

closes #4689

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 18082.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 9125 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 9125 passed.

Copy link
Contributor

@ncesario-lunarg ncesario-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fixes the original issue, then LGTM, but I'm missing what actually changed.

Also, what is the rationale behind the rename from Vec -> List? Not necessarily arguing for or against it, just curious :)

Comment on lines +82 to +83
// (1 << 31) is greater than VK_SHADER_STAGE_FLAG_BITS_MAX_ENUM
for (uint32_t stage_idx = 0; stage_idx < 31; ++stage_idx) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually fix the original issue? I'm not sure I see what changed here. I would think we would need something like stage_idx = highest_bit(VK_SHADER_STAGE_FLAG_BITS_MAX_ENUM); to make UBSAN happy here, but I'm probably missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this code logic needs to loop through all the VkShaderStageFlagBits values

VK_SHADER_STAGE_FLAG_BITS_MAX_ENUM == 0x7FFFFFFF

so I assumed the 1 << 30 is 0x40000000 which is the max any valid Vulkan FlagBits can be. I assume this change would correctly fix UBSAN, but honestly I would rather have a way to loop through all the VkShaderStageFlagBits, I was thinking how we have these vectors in paramater_validation.cpp such as const std::vector<VkFormat> AllVkFormatEnums and that maybe it would be smarter to just generate something similar for this code instead

rationale behind the rename

I didn't know what Vec stood for, wasn't an intuitive abbreviation for me, but now realizing we use *Vec else where, rather not inconsistently change it here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I completely missed the 32 -> 31. I'm good with this if it fixes the original issue. The comment wasn't intuitive to me the first time I read it, but I probably just read it too quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact you can't at a glance know if it is correct or not is the problem IMO, going to just generate an array to loop through stages flags

@spencer-lunarg
Copy link
Contributor Author

spencer-lunarg commented Oct 22, 2022

closing for a better solution I will create

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UBSAN error in PIPELINE_STATE::GetStageStates
3 participants