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

Force RDD::id to be always uint64_t #98910

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Nov 6, 2024

On Vulkan, some handles are meant to always be u64, even on 32-bit architectures such as arm32.

Fixes #98654

Testing methodology

Testing this is hard, because most platforms Godot is meant to be run on are already 64-bit.
Effectively this only affects arm32.

My particular worry is what if at some point Godot does the following? (or similar)

void * ptr = (void *)buffer.id; // Ooops! This is downgraded from 64 to 32 bit
buffer.id = (size_t)ptr;

More bizarre stuff such as memcpy could be happening.

To check for this very particular scenario I was afraid of, I devised a much larger patch I'm including here:

arch32_test.patch.zip

Which changes RenderingDeviceDriver::id to private and is a RenderingDeviceDriver::id[3]; where I store the value in id[1].

This way, if Godot were to make such horrible type punning (or other dangerous raw manipulation), the value would be lost even on an x64 machine.

I tested this patch, and Godot ran fine. I tested 3 different projects, but not thoroughly.

So far this change appears to be harmless. But I can never be 100% certain. Type punning Vulkan & D3D12 handles as RenderingDeviceDriver::id was not the best idea to begin with. But it is what we have.

Hopefully CI will agree.

On Vulkan, some handles are meant to always be u64, even on 32-bit
architectures such as arm32.

Fixes godotengine#98654
@darksylinc darksylinc requested a review from a team as a code owner November 6, 2024 23:10
@Calinou Calinou added this to the 4.4 milestone Nov 6, 2024
@clayjohn clayjohn changed the title Force RID::id to be always uint64_t Force RDD::id to be always uint64_t Nov 7, 2024
@akien-mga akien-mga changed the title Force RDD::id to be always uint64_t Force RDD::id to be always uint64_t Nov 7, 2024
@Repiteo Repiteo merged commit 3cff32a into godotengine:master Nov 10, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 10, 2024

Thanks!

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.

Godot's Vulkan is non-compliant (broken) on arm32
4 participants