KhronosGroup / Vulkan-Samples

One stop solution for all Vulkan samples
Apache License 2.0
4.36k stars 648 forks source link

Ensure to initialize image sharing mode when implicit #1211

Closed zhangyiwei closed 3 weeks ago

zhangyiwei commented 3 weeks ago

Description

Fixed a VVL violation of uninitialized image sharing mode.

Related to #1207

asuessenbach commented 3 weeks ago

Following the discussion in #1207, it's not yet clear if this would not only resolve this issue but also hide some other potentially more fundamental one. That is, we should wait 'til the discussion in #1207 is done.

zhangyiwei commented 3 weeks ago

Following the discussion in #1207, it's not yet clear if this would not only resolve this issue but also hide some other potentially more fundamental one. That is, we should wait 'til the discussion in #1207 is done.

I can drop the Fixes tag to keep the discussion there alive. Meanwhile, it's generally not a good practice to skip assigning enums that happen to be zero. I'd prefer we take this pr still.

asuessenbach commented 3 weeks ago

Meanwhile, it's generally not a good practice to skip assigning enums that happen to be zero.

This enum is initialized with a value that happens to be zero. Perfectly valid. Your PR would, at best, be superfluous. And at worst (as we encountered here), it would hide the actual issue.

The right move would be to prevent some compiler optimizations that are identified to be dangerous in this case. That is, using -fno-strict-warning for all the samples would be better.

zhangyiwei commented 3 weeks ago

This enum is initialized with a value that happens to be zero. Perfectly valid.

I'm not convinced. VK_SHARING_MODE_EXCLUSIVE just happened to be 0 when we originally introduced it, but whatever : /

...And at worst (as we encountered here), it would hide the actual issue.

The actual issue being exposed by this uninitialized field is just a coincidence. There're tons of other struct members could potentially leading to unexpected render without being properly initialized. It's a big question mark to lean on this to gate proper zero-init.

The right move would be to add a unit test for the builder to guarantee zero-init under the default compiler optimization used in the project.

/cc @SaschaWillems

SaschaWillems commented 3 weeks ago

Gotta admit that my C++ isn't good enough to fully understand some of the framework parts. I took a look at the builder but wasn't able to find out why or if at all it does initialize things.

So why don't we just explicitly zero initialize all the structs? Would also make it easier to understand for people trying to learn from our samples.

zhangyiwei commented 3 weeks ago

Me either unfamiliar with the right expectations of the builder class. I’d vote for either explicitly initialize all fields after getting the alloc or ensure zero init within the builder class ; )