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

Change bit field enums to use uint64_t as underlying type #1320

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Nov 27, 2023

Fixes godotengine/godot#85444.

As discussed in the issue above, this would change the underlying integer type for enums declared with VARIANT_BITFIELD_CAST to be uint64_t instead of the default int.

This helps resolve an issue where MSVC will currently truncate any enum values larger than 2^31 for enums that don't have an explicit underlying type, which is affecting RenderingServer::ArrayFormat, as seen in the issue above.

Note that this technically opens up for other potential future problems, where someone on the Godot side of things could define a regular int-backed enum, bind it with VARIANT_BITFIELD_CAST and then include it in something like a GDREGISTER_NATIVE_STRUCT, which would lead to an ABI difference and potentially memory corruption.

@mihe mihe requested a review from a team as a code owner November 27, 2023 23:33
@mihe mihe marked this pull request as draft November 27, 2023 23:46
@mihe mihe marked this pull request as ready for review November 27, 2023 23:48
@mihe mihe force-pushed the bit-field-size branch 2 times, most recently from b1f6756 to 1127a55 Compare November 28, 2023 00:00
@AThousandShips AThousandShips added bug This has been identified as a bug topic:buildsystem Related to the buildsystem or CI setup labels Nov 28, 2023
@AThousandShips AThousandShips added this to the 4.2 milestone Nov 28, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

This looks good to me as a short term downstream solution.

I agree we should aim to get this information in the API json itself, or document clearly that is_bitfield always means uint64_t and should be interpreted as such by bindings.

@akien-mga akien-mga requested a review from dsnopek November 28, 2023 11:34
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 28, 2023

Personally, I feel like we should be saying that all enums are backed by a 64-bit integer, since we are already saying that they are always encoded (when passing as an argument or return value) as a 64-bit integer.

Unfortunately, there is the case with GDREGISTER_NATIVE_STRUCT() that @mihe points out, where enums can be used without passing through our usual encoding mechanism, and the existing CaretInfo struct that uses an enum, and so saying all enums are 64bit would break the ABI on 32-bit builds. Also, GDREGISTER_NATIVE_STRUCT() is technically using C structs (not C++) and (as best I can tell via Google) the C standard defines enums as being int-sized and we can't explicitly set the backing type, so we'd probably also need a rule that says we shouldn't use enums in GDREGISTER_NATIVE_STRUCT()s. This definitely needs more thought and discussion in general!

In any case, I also think this fix is fine for now. :-)

@akien-mga akien-mga merged commit f3143c7 into godotengine:master Nov 28, 2023
12 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension bindings for new RenderingServer::ArrayFormat are incorrect
4 participants