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

Vulkan: Fix sanitizers error with empty shader name #80288

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

pkpro
Copy link
Contributor

@pkpro pkpro commented Aug 5, 2023

In drivers/vulkan/rendering_device_vulkan.cpp:4815 on editor start compiled with following options:
dev_build=yes debug_symbols=yes optimize=debug use_ubsan=yes use_asan=yes use_lsan=yes use_msan=yes

Whenever you use a low level functionality for compute shaders the shader name is nullptr on editor start.
This is non critical, as the length of the shader name is 0, no actual copy happens, though the sanitizers report this as a runtime error.

@pkpro pkpro requested a review from a team as a code owner August 5, 2023 06:53
@AThousandShips
Copy link
Member

Please squash your commits into a single one, see here

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Seems fine

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Aug 7, 2023
@clayjohn clayjohn added enhancement and removed bug labels Aug 7, 2023
@akien-mga
Copy link
Member

Thanks for contributing. The fix looks correct, though check bruvzg's suggestion to make it better.

Aside from this, there are a number of Git issues that need to be handled:

  • Your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.
  • The commits should be squashed (see PR workflow).
  • The resulting commit message should be improved. "memcpy into nullptr" isn't explicit enough about what is being fixed and where. This could apply to thousands of places in the codebase. Something like "Vulkan: Fix sanitizers error with empty shader name" would be good for example.

@akien-mga akien-mga changed the title memcpy into nullptr. non critical Vulkan: Fix sanitizers error with empty shader name Aug 7, 2023
@akien-mga
Copy link
Member

akien-mga commented Aug 8, 2023

Note that you merged the remote into your branch, instead of rebasing, squashing, and amending the commit as I requested. So the PR now has 3 commits when it should be 1.

@pkpro pkpro force-pushed the memcpy_into_nullptr branch from 2df9694 to c83280d Compare August 8, 2023 13:13
@akien-mga akien-mga force-pushed the memcpy_into_nullptr branch from c83280d to 9b3b891 Compare August 9, 2023 09:59
@akien-mga akien-mga requested a review from clayjohn August 9, 2023 10:00
Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@akien-mga akien-mga force-pushed the memcpy_into_nullptr branch from 9b3b891 to 77b0235 Compare August 9, 2023 10:13
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good!

@akien-mga akien-mga merged commit c1dbc3d into godotengine:master Aug 17, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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

Successfully merging this pull request may close these issues.

5 participants