Fix: changed bitfields to be i64 instead of u64

[TCROC]

[Edit Bromeon]: Direct links to issue in other repos:

• Should bitfields types be unsigned integers instead of signed integers? godotengine/godot#88962
• https://github.com/GodotSteam/GodotSteam/issues/424

---

I found that gdext and godot-cpp are incorrectly generating bitfield bindings as u64 when they should be i64.

This is what gdext is currently referencing as its reasoning for using u64: https://github.com/godotengine/godot-cpp/pull/1320/files

However, upon further inspecting the godot source code, it can actually be seen that the godot engine is mapping all bitfields to i64 instead of u64:

https://github.com/godotengine/godot/blob/81f3d43cc1ba01136795fb2059bbaa55bc514a16/core/object/class_db.cpp#L949

So it appears that godot-cpp is incorrectly binding to u64 and that mistake bubbled into gdext as well.

I noticed this because the gdext bindings started erroring on our windows machines where windows interpretes 0xffffffff to be -1 and Linux interprets it to 4294967295 implicitly. The -1 was causing issues on the godot gen bindings for windows when using godotsteam.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-627

[lilizoey]

This would be a simple solution to the main example in #327 at least lol. I can't look into this very much rn but figured i should mention that.

[Bromeon]

I noticed this because the gdext bindings started erroring on our windows machines where windows interpretes 0xffffffff to be -1 and Linux interprets it to 4294967295 implicitly. The -1 was causing issues on the godot gen bindings for windows when using godotsteam.

We had this problem in #527, where you were involved as well. What has changed since?

Also, 0xffffffff is 2^32-1, not 2^64-1. Either way, u64 can represent it.
Can you elaborate where the -1 comes from? We should fix that -- negative bitflags are almost certainly a bug.

Also, the change to use u64 was done in #503. Semantically, bitfields are always unsigned, what do negative values even mean?

[TCROC]

We had this problem in #527, where you were involved as well. What has changed since?

A handful of things have changed:

1. In that pull request, we used i64. Not u64. So we have changed to using u64 from i64 which is now breaking things.
2. I did not fully understand the issue at hand during that time and I do now. I debugged the Steam source code and sure enough, Steam uses -1 as a bitflag on Windows which Godot supports given that it tracks bitflags as i64 here: https://github.com/godotengine/godot/blob/81f3d43cc1ba01136795fb2059bbaa55bc514a16/core/object/class_db.cpp#L949
3. Steam recently updated their sdk.

Can you elaborate where the -1 comes from? We should fix that -- negative bitflags are almost certainly a bug.

Its in Steam's sdk. It evaluates to -1 as a constant on Windows at compile time. I stepped through it with the debugger and found this to be the case. I can also hover over it in my IDE and it shows the same value:

Linux:

Windows:

The steam sdk can be downloaded from here: https://partner.steamgames.com/downloads/steamworks_sdk_159.zip

Semantically, bitfields are always unsigned, what do negative values even mean?

I 100% agree with this. I have no idea what a -1 bitfield means but for some reason Steam's sdk uses it on Windows. But I think this is besides the point. Gdext is for godot bindings. And currently Godot stores bitflags with the i64 datatype as seen here: https://github.com/godotengine/godot/blob/81f3d43cc1ba01136795fb2059bbaa55bc514a16/core/object/class_db.cpp#L949. In order to have correct bindings, I think we should match the data type godot uses.

[TCROC]

At the end of the day, this is a datatype binding issue. Not a bitfield issue. I think the concept of bitfields was distracting us from the point that these are godot bindings and should use the same data types godot uses. The -1 may be a bug, but that is for Steam to figure out. Our issue at hand is that we should match godot's data types with our bindings.

[Bromeon]

Thanks for the detailed answer!

---

And currently Godot stores bitflags with the i64 datatype as seen here: https://github.com/godotengine/godot/blob/81f3d43cc1ba01136795fb2059bbaa55bc514a16/core/object/class_db.cpp#L949. In order to have correct bindings, I think we should match the data type godot uses.

No, the Godot C++ implementation is mostly irrelevant to us. What is relevant is the GDExtension API.
Implementations change, APIs are stable.

But the API does indeed use i64, too:

typedef int64_t GDExtensionInt;

typedef void (*GDExtensionInterfaceClassdbRegisterExtensionClassIntegerConstant)(
    GDExtensionClassLibraryPtr p_library,
    GDExtensionConstStringNamePtr p_class_name,
    GDExtensionConstStringNamePtr p_enum_name,
    GDExtensionConstStringNamePtr p_constant_name,
    GDExtensionInt p_constant_value,
    GDExtensionBool p_is_bitfield
);

So yes, we can consider changing it. But we need to be sure to not introduce different problems. Changing u64 to i64 looks like an obvious fix, but we can't just ignore the question "what if values are negative". That's the entire idea behind using signed integers.

---

I have no idea what a -1 bitfield means but for some reason Steam's sdk uses it on Windows. But I think this is besides the point.

No, it's exactly the point. We have the responsibility to provide correct bindings, and if there are upstream errors, we should report them, not work around them.

The Steam constant is defined as 0xffffffff and NOT -1. These are different!

let a: i64 = 0xffffffff;

The value here is 4294967295 and not -1.

Their bit patterns are only equal in 32-bit integers, but not in 64-bit ones that we use! So semantics differ as soon as larger constants are introduced. Which may not be a problem for the Steam enum, but for other APIs.

The issue is that enum in C leaves the underlying integer up to implementation, so Windows uses signed and Linux unsigned. But the code's intention is clearly to use 0xffffffff, a positive integer.

It may not matter for bit operations, but if users access the ord() conversion, negative values like that may catch them off-guard. And they may cause further bugs down the line, as it's not unlikely that people rely on bitfields being positive.

And this doesn't even mention the fact that the very same constant, with the very same code, is now registered as 2 different values in Windows and Linux. Which can cause all sorts of nasty problems, with hashes, serialization, networking etc.

---

How does the -1 constant get into Godot? Because the declared value is 0xffffffff and Godot's GDExtension API expects i64 input -- so I'd argue it's responsibility of the constant provider to make sure the original value (0xffffffff as i64) is retained.

[TCROC]

so I'd argue it's responsibility of the constant provider to make sure the original value (0xffffffff as i64) is retained.

I agree. And in this case, the constant provider is Steam. I will open a bug report with Steam so that they can resolve it on their end or clarify with me the confusion.

For us, I think we should change to i64 so that gdext bindings match Godot's datatypes.

[Bromeon]

I agree. And in this case, the constant provider is Steam. I will open a bug report with Steam so that they can resolve it on their end or clarify with me the confusion.

But you only showed C code with the enum definition. Who registers that constant within Godot?

---

For us, I think we should change to i64 so that gdext bindings match Godot's datatypes.

I think this needs a wider discussion on Godot side. Negative bitfield values make no sense, so it might be better to outlaw them, or at least provide specific guidelines to prevent bugs like this.

[TCROC]

But you only showed C code with the enum definition. Who registers that constant within Godot?

I believe godotsteam does: https://github.com/CoaguCo-Industries/GodotSteam/tree/godot4. I'm not entirely sure though.

I think this needs a wider discussion on Godot side. Negative bitfield values make no sense, so it might be better to outlaw them, or at least provide specific guidelines to prevent bugs like this.

I agree that this should have a wider discussion on the Godot side. But I still think we should change to i64 so our datatype matches Godot's. And then if Godot changes their data type to u64, we should change ours to u64. I'm more concerned that our bindings match otherwise they are not correct bindings.

[Bromeon]

My point is:

The fact that we use u64 and not i64 just highlighted a bug (probably in GodotSteam). This would have gone unnoticed had we used i64, and caused problems down the line, possibly on multiple users' ends.

So I'd like to await a consensus here. Also considering that godot-cpp, the reference implementation, uses uint64_t.

[TCROC]

godot steam has a header for it here: https://github.com/CoaguCo-Industries/GodotSteam/blob/5348754e43e06ceae150888534047b64bbd6f5d2/godotsteam.h#L1482

And it looks like it gets registered by godotsteam here: https://github.com/CoaguCo-Industries/GodotSteam/blob/5348754e43e06ceae150888534047b64bbd6f5d2/godotsteam.h#L3039

I'm not super familiar with these API's tho so I'm just kinda skimming and intuitively guessing.

[TCROC]

The fact that we use u64 and not i64 just highlighted a bug (probably in GodotSteam). This would have gone unnoticed had we used i64, and caused problems down the line, possibly on multiple users' ends.

It potentially highlighted a bug. Its possible Steam actually uses -1 in their sdk on Windows for some reason. Hence the reason I will open a bug with them and ask for a fix or clarification.

So I'd like to await a consensus here. Also considering that godot-cpp, the reference implementation, uses uint64_t.

I still think this is an incorrect implementation. Its okay though as it is in my fork and is only a few lines of code so it should not be too difficult for me to maintain. But I think matching the datatypes when binding to Godot's source code is more important and correct when compared to trying to manipulate what we think those datatypes should be with our bindings.

If they should be u64, Godot should change them to u64 and then we change them on our side. In the mean time, I believe its most important that data type bindings match.

[TCROC]

Also a potentially more important problem: godotsteam users currently cannot use gdext on Windows. Gdext panics when trying to generate the bindings.

[Bromeon]

Also a potentially more important problem: godotsteam users currently cannot use gdext on Windows. Gdext panics when trying to generate the bindings.

In other words: we show an error at compile time that might otherwise only manifest at runtime 😉

It might work for bit operations, but it's really a matter of time until this causes problems.

---

It potentially highlighted a bug. Its possible Steam actually uses -1 in their sdk on Windows for some reason. Hence the reason I will open a bug with them and ask for a fix or clarification.

Not sure if this is a Steam issue. How do you expect them to fix this? They'd need to use something else than enum, which is likely a breaking change. But it can't hurt to ask.

I believe GodotSteam is responsible of providing constant values consistently to Godot, independently of the platform. Since they use C++ and not C, they can probably use enum MyBitfield : std::int64_t. Maybe this works already, or they need to explicitly convert the Steam values.

[TCROC]

In other words: we show an error at compile time that might otherwise only manifest at runtime 😉

Except I think the error is on our end as we aren't binding to the appropriate data types. It is possible (albeit unlikely) that -1 is indeed the correct value on Windows for the Steam sdk.

[TCROC]

And gdext was correctly using i64 previously but changed to the incorrect cpp gdextension implementation. Incorrect as far as bindings go that is. Not necessarily semantics. I hold that bindings and semantics are 2 separate issues and that this is a bindings issue.

[TCROC]

I will open an issue on the godotsteam board as well so we can get their thoughts. They may have more information regarding the bitflag.

[Bromeon]

I hold that bindings and semantics are 2 separate issues and that this is a bindings issue.

This distinction is not useful for any practical purpose. Of course we should try to adhere to the correct data type, but at the same time this example shows very well that being more explicit (bitfields must be unsigned) revealed a bug.

We need a holistic solution, not blindly sticking to other APIs without thinking.

---

Except I think the error is on our end as we aren't binding to the appropriate data types. It is possible (albeit unlikely) that -1 is indeed the correct value on Windows for the Steam sdk.

They might consider it correct in the sense of being the user's responsibility of mapping the enum to the correct integer type.

Again, for practical purposes there is absolutely no reason why the same constant by design should have 2 distinct values on Windows and Linux. No way this is intentional -- at best this is a limitation deemed acceptable due to other trade-offs.

---

I will open an issue on the godotsteam board as well so we can get their thoughts. They may have more information regarding the bitflag.

Thanks a lot! 👍

[TCROC]

We need a holistic solution, not blindly sticking to other APIs without thinking.

This is a good challenge you have presented! It actually made me think: "How do we holistically go about being explicit AND maintaining correct bindings?" And I think I thought of a solution that helps everyone! :)

1. Gdext should bind to Godot with the data types that Godot uses but not blindly or without thinking.
2. Following point 1, when we discover data types that don't make sense or we think can be improved, we open issues on Godot / the appropriate repo.
3. Following point 2, we also change our current panics to a WARNING so that it is explicit and clear that something potentially bad is happening with the bindings but we don't break it.
4. This has the benefit of improving the sources we are binding to while still maintaining compatibility with those sources!

If you agree with this proposal, I can update the PR to have a warning instead of a panic when it is generating the bindings while we wait on Godot and GodotSteam to evaluate the bitflags and whether they should be u64 instead of i64 :)

Everyone wins! :)

[Bromeon]

I think that sounds great, although I'd still like to get some input from Godot devs on the matter 🙂

How did you imagine the warning? Unfortunately 9 years of Rust is not enough to have proper compiler warnings as a feature 😕 so we'd need to have it at startup... and we'd need a way to suppress it 🙈

[TCROC]

I think that sounds great, although I'd still like to get some input from Godot devs on the matter 🙂

Yep I agree and we will. I'll open an issue with them and link them to this PR and discussion :)

How did you imagine the warning? Unfortunately 9 years of Rust is not enough to have proper compiler warnings as a feature 😕 so we'd need to have it at startup... and we'd need a way to suppress it 🙈

That's unfortunate :(. I guess a workaround for now could be to initially panic and when it panics, the message will link to the issues we have open regarding the issue and indicate that you can enable some kind of feature flag to suppress the panic. Something like "allow-negative-bitfields" or something along those lines.

Error message might be something like:

"Encountered negative bitfield. Bitfields should never be negative. This error is known and is being evaluated in the following issues [github.com/issue1, github.com/issue2, ...]. To allow for negative bitfields, enable the feature 'allow-negative-bitfields' in your Cargo.toml"

Something along those lines :)

[Bromeon]

A Cargo feature is overkill here, that would quickly lead to combinatoric explosion of all possible warnings. And it's a very specific edge case. Maybe for now we can just print a warning on startup with godot_warn!. I can then think about suppressing it later.

Can you link the upstream issues here, once you created them? 🙂

[TCROC]

Yep! I'll link those upstream issues as soon as I create them and then update the PR to be a warn :)

[TCROC]

Had a busy day today. Will create and link those issues tomorrow

[TCROC]

Added a very simple system for creating a warning if negative bitfields are encountered. The system can be expanded upon to gather more intricate messages and log them at startup, but I figured that was out of scope for this PR. For now, I just return a String from the functions that generate enums. If the String is empty, we know there isn't a warning. If it has a value, we generate the warning message at startup. When godot starts, it looks like this:

WARNING: Encountered negative bitfield. REMOTE_STORAGE_PLATFORM_ALL = -1. This is currently being discussed in the following issues: https://github.com/godotengine/godot/issues/88962, https://github.com/CoaguCo-Industries/GodotSteam/issues/424

And it can very easily be suppressed in the macro as well!

#[gdextension(ignore_warnings)]
unsafe impl ExtensionLibrary for RustExtension {}

I envision that a future system can be fleshed out for suppressing different kinds of warnings, but that would be overkill until that need presents itself. For now I just did it with a simple String being returned form a message and a simple "ignore_warnings" attribute on the gdext macro to suppress it.

Let me know if you want any changes for it. Looks like rustfmt is unhappy with me so I'll go fix that.

Also, I'll squash the commits into a single commit if / when ur happy with it as I know you like it. Just wanted to keep them both their for now while we discuss it.

[TCROC]

And these were the issues I opened:

Godot: godotengine/godot#88962
GodotSteam: https://github.com/CoaguCo-Industries/GodotSteam/issues/424

[TCROC]

Just checking in. Any thoughts on this PR while we wait on Godot and GodotSteam to discuss their bitflags?

[Bromeon]

Thanks for the update!

The tuple return type for the warning is now quite invasive and "infects" all codegen paths. For an extremely niche use case, this adds significant code complexity.

I think it would be nicer if you added a method Context::push_warning(), which can be invoked by individual functions. Most of the functions already take the ctx parameter. This would also allow to push multiple warnings, and in the end you just have a getter that returns a Vec or iterator to all warnings.

Also, the u64 -> i64 change and the warning should definitely remain two separate commits 🙂
I'm still awaiting more information from upstream to make a final decision on i64.

[TCROC]

Thanks for the feedback! I'll go take a look into implementing those suggestions! I definitely like the idea of a more general purpose and simpler implementation of the warnings through the Context::push_warning.

[TCROC]

And I'll make sure to leave them as 2 separate commits :)

[TCROC]

I updated it per your suggestions. It is definitely much much simpler. Your suggestion for using ctx and adding the ignore_codegen_warnings to the ExtensionLibrary trait were very elegant! :)

[TCROC]

It seems upstream is still deliberating. In the mean time, should we push this PR through? Or do you have any other feedback on it?

[Bromeon]

I'm still not fully sold on using i64 until there's an upstream decision. It might send the wrong message, indicating that bitflags can be negative. Also, if Godot decides to use u64 internally in the future, we'd need to revert this change and break user code twice.

On the other hand, you're right that currently GodotSteam doesn't work with gdext...

I'll bring it up with Godot devs again.

[Bromeon]

Just a heads-up, I haven't forgotten about this 🙂 but not much has moved upstream, and the Godot team has been very busy with preparing 4.3. I'll try to bring the topic up again for the 4.4 cycle.

Until then, I'd probably keep the status quo of u64. Don't worry if the branch diverges and runs into conflicts with master.

[Bromeon]

The GDExtension team has decided that bindings are supposed to use uint64_t for registering bitfields, even if the registration C function uses int64_t.

As such, we would stay with the current u64 representation in godot-rust, and this PR is no longer relevant. Thanks nonetheless for bringing this up, it has helped to both clarify the bitfield semantics in Godot and fix a bug in GodotSteam!