Promote CowData to 64 bits

[reduz]

Fixes a lot of bugs, please help me fill the list.

< I welcome the 64 bits overlords >
 ---------------------------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

After this is merged, other areas of the engine can be fixed, such as Image, which also uses 32 bits offsets and causes crashes in some reasonable scenarios (such as baking lightmaps), but it depends on this one. Serialization also may need to be fixed.

Bugsquad edit:

• Fixes Godot crash in ResourceSaver after 1073741824 bytes #62585
• Fixes Error when resizing PackedByteArray bigger than int32 limit #77256
• Fixes Attempting to read large file as string causes segfault #80533

[Mickeon]

Would fix #45296 and #54679 for sure.

[AThousandShips]

Note that this requires _get_alloc_size to be updated unless we still treat this as having a capped size in the 32 bit range, as it only handles 32 bits, next_power_of_2 needs to be updated

Also needs a godot-cpp companion

[RandomShaper]

Looks good overall.

I'm wondering if this is neglecting 32-bit builds (they don't need or won't support 64-bit stuff such as > 3 GiB allocations and maybe not even 64-bit atomics) and so size_t should be used intead.

UPDATE: Per @AThousandShips, size_t won't necessarilly be 64-bit on 64-bit targets, so maybe something else that matches sizeof(void *), but you get the idea.

[reduz]

I changed to use a typedef for the size, depending on compiler, so it remains 32 bits on 32 bit platforms.

[reduz]

@AThousandShips regarding GodotCPP, I am not certain if something has to change on that side.

[AThousandShips]

It does, it redeclares the type, unless we're okay with lack of parity over there

[AThousandShips]

Need a macro for the max as well, I'd suggest a SIZE_MAX/MIN and USIZE_MAX/MIN though might clash with definitions existing already

[reduz]

@AThousandShips ah I completely forgot we strive to use the same syntax in GodotCPP than in Godot modules.. Will have to ask @bruvzg to lend a hand changing the code there once this one is merged.

So I guess this PR is "kind of" compat breaking.. though it likely only affects GodotCPP, not the rest of the extensions.

[TokisanGames]

Thank God. I haven't tested yet, but will link up related issues. Lack of 64-bit resources is a problem for Terrain3D users with big terrains.

So hopefully this will help fix our issue, crashing when saving large terrains in a >1gb resource file.
TokisanGames/Terrain3D#159

And Godot issue #62585

Possibly superseding or related to PRs
#74582
#77306

[AThousandShips]

While changing the size values of String might be risky because it's exposed (granted so is Vector, indirectly), VMap and VSet uses CowData internally as well, RingBuffer uses Vector, should we unify all data structures using these sizes at least the non-exposed ones?

Also iteration over the packed vectors assume a 32 bit iteration value, might be worth looking into, and a number of other potential places, as well as any iteration over any of these types, avoiding errors

[reduz]

@AThousandShips A lot other structures have to be fixed, not just those. Image also assumes 32 bits and this is giving troubles, but I don't want to do a huge PR that will take a long time to get merged and that I have to keep up to date, so I prefer we merge this one first, then work on individual PRs for the other structures.

[AThousandShips]

As long as the iteration cases aren't broken it should be safe enough

[AThousandShips]

The code looks good to me, the sizing checks should work correctly with the new po2 method and my previous fix for the resize should still handle that just with a new range

Needs to ensure that any iteration being done isn't broken, might need a fix of any for (int...) loops using Vector so we don't get broken data etc., as part of the fixes involved in this are for cases where data does run out

Otherwise looks good, if everything else works right

[akien-mga]

There's a MSVC warning to fix:

Error: .\core/templates/ring_buffer.h(200): error C2220: the following warning is treated as an error
Warning: .\core/templates/ring_buffer.h(200): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)

---

And the "Test project converter" in the C# build fails with a C# stacktrace:
https://github.com/godotengine/godot/actions/runs/7472530025/job/20334943529?pr=86730

But I don't know if it's actually due to a C# issue, or just a Godot error that would happen on a non-dotnet build with just a different error (that project converter test is only run on the "Editor w/ Mono" Linux build).

The test is just this, so this can be attempted locally to see if the error is reproducible:

mkdir converter_test
cd converter_test
touch project.godot
godot --headless --validate-conversion-3to4
cd ..
rm converter_test -rf

[adamscott]

cc. @Bromeon @dsnopek (GDExtension related)
cc. @raulsntos (does it impact dotnet?)

[reduz]

fixed the shift issue.

[AThousandShips]

Would suggest compiling with -k on windows if you can to catch all the cases as there's likely many

[reduz]

I haven't used MSVC to compile in an eternity, will check during the week.

[dsnopek]

It is to note that this PR will break GodotCPP GDextension compatibility, yet again.

This is an unfortunate consequence of the fact that GodotCPP accesses internals it should technically not for the sake of being compatible with modules. We should maybe consider having a different set of bindings, or do some things in GodotCPP different to avoid this scenario.

I don't think this is exactly true.

It will only break godot-cpp, if godot-cpp is interacting with a String or Packed*Array or similar, that has more items than a 32-bit index can access. It isn't actually accessing any internals that it shouldn't, it just uses implementations of CowData and Vector etc that are derived from the code for the same classes in Godot itself, so they suffer from the same limitations.

I think with bruvzg's godot-cpp PR godotengine/godot-cpp#1357, we should be good going forward. And I think we can cherrypick it for the 4.2 and 4.1 versions of godot-cpp.

But if a GDExtension compiled with an older godot-cpp is used with a newer Godot, AND it's passed a String or Packed*Array with too many items, then there could be some breakage. However, this feels like a small enough edge case to probably be acceptable?

[raulsntos]

To the extent that this changes the memory layout of Variant types, it has an impact on C# interop structs.

In C# we define interop structs that must match the memory layout of the C++ types so they can be reinterpreted and consumed from C#. For example, we get the size of a Godot String like this:

godot/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs

Line 477 in 107f296

get => _ptr != IntPtr.Zero ? *((int*)_ptr - 1) : 0;

This is basically copied from the C++ code (CowData::get_size):

godot/core/templates/cowdata.h

Line 85 in 107f296

return reinterpret_cast<uint32_t *>(_ptr) - 1;

But this has changed in this PR since now the size is 64-bit, so it needs to be updated in C# too.

As long as the changes are kept internally, it should not break compatibility but changing the user-facing API would be a problem. And if any of these types actually has a size that doesn't fit in a 32-bit int then it will break C#¹.

Here's a somewhat naive diff that should fix the stackoverflow you are getting on CI:

diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
index d5d9404ed1b..c806263edb5 100644
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/InteropStructs.cs
@@ -474,7 +474,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != IntPtr.Zero ? *((int*)_ptr - 1) : 0;
+            get => _ptr != IntPtr.Zero ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -725,7 +725,7 @@ namespace Godot.NativeInterop
             public readonly unsafe int Size
             {
                 [MethodImpl(MethodImplOptions.AggressiveInlining)]
-                get => _ptr != null ? *((int*)_ptr - 1) : 0;
+                get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
             }
         }
 
@@ -875,7 +875,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -939,7 +939,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -971,7 +971,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -1003,7 +1003,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -1035,7 +1035,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -1067,7 +1067,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -1099,7 +1099,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 
@@ -1131,7 +1131,7 @@ namespace Godot.NativeInterop
         public readonly unsafe int Size
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _ptr != null ? *((int*)_ptr - 1) : 0;
+            get => _ptr != null ? (int)(*((ulong*)_ptr - 1)) : 0;
         }
     }
 

Footnotes

1. More context on this: https://github.com/dotnet/runtime/issues/12221 ↩

[reduz]

@raulsntos Yeah I guess the Mono bindings should not really be accessing these internals. We have explicit functions exposed in the extension API now to avoid these situations, so I suppose it would be just a matter to move to that eventually (when Mono is moved to extension)

[raulsntos]

I don't want to get too off-topic, but I don't agree that the move to GDExtensions would help here. The API exposed to retrieve this data would have a bigger cost than accessing the internals directly as we are doing today so we wouldn't change that in the new system.

And to be honest that's not the problem, we just need to update the C# internals to match the C++ internals (similar to how godot-cpp is doing). I shared a diff that does just that and doesn't break compatibility since it doesn't change publicly exposed API.

The main issue is that C# types like System.String or arrays define their length as 32-bit integers so they can't possible work with data that is bigger than what can be represented with a int32_t¹.

To put it in simple terms, if we have a String coming from Godot that has more items than what can fit in a 32-bit length then we wouldn't be able to marshal that into a C# String.

Footnotes

1. https://github.com/dotnet/runtime/issues/12221 ↩

[dsnopek]

And to be honest that's not the problem, we just need to update the C# internals to match the C++ internals (similar to how godot-cpp is doing).

Just to be clear (although, it probably isn't necessary :-)) this isn't what the godot-cpp change is doing. godot-cpp doesn't directly access Godot's internals, it always goes through the GDExtension interface.

The change to godot-cpp is doing two things:

1. Updating the C++ classes that exist entirely on the godot-cpp side to account for the possibility that the data on the Godot side could have a large enough number of elements that it requires a 64-bit integer to count/index them. This is the part that relates to interop with Godot.
2. Updating the equivalent CowData and Vector in godot-cpp to match Godot, so that you can use them in your GDExtension exactly the same as you would in Godot itself, removing the 32-bit limitations of the previous implementation. This doesn't affect interop with Godot, this is for where these templates are used purely on the godot-cpp side.

[akien-mga]

Fixes a lot of bugs, please help me fill the list.

Trying to review some of the ones mentioned in this PR, linked issues, or some I could find searching for references to CowData and 32/64 bits.

• BakedLightmap CPU lightmapper crashes on large scenes #45296

3.x issue so it won't be solved by this (and likely won't ever be solved).

• BitMap class limited to XY size 46340x46340 #48399

Reproducible in master. This PR doesn't fix it because BitMap now has guards against using a size bigger than INT32_MAX. This PR should however make it possible to refactor BitMap to use int64_t.

• Geometry.make_atlas() crashes Godot when sizes are large #51154

Not fixed by this PR. The stacktrace changes from CowData<_AtlasWorkRectResult>::get(int) to CowData<_AtlasWorkRectResult>::get(long) but it's still crashing. Likely another one that could be fixed now as a follow-up.

• Vulkan: Crash when baking lightmaps with LightmapGI if the resulting image is too large (due to CowData using 32-bit ints) #54679

Seems related indeed, but I tested and this PR does not solve the issue. It still crashes in the same place.
Relevant Image code is still using int for sizes so I assume it needs further work after this PR to actually fix the crash.

[3] Image::initialize_data(int, int, bool, Image::Format) (/home/akien/Projects/godot/godot.git/./core/io/image.cpp:2187 (discriminator 4))

• Godot crash in ResourceSaver after 1073741824 bytes #62585

Tested with the MRP from #62585 (comment), this fixes it.
I don't get a crash however before this PR in a 2 weeks old masterbuild, but I do get a p_size < 0` error which is now fixed.

• Error when resizing PackedByteArray bigger than int32 limit #77256

Fixed by this PR.

• Attempting to read large file as string causes segfault #80533

Fixed by this PR.

[akien-mga]

Diff that should make the PR pass CI (two more MSVC warning fixes, and @raulsntos' C# changes):
akien-mga@6eff4f6

[reduz]

@akien-mga thanks lots for taking the time for this!

[akien-mga]

Tested locally, seems to work well!

As I pointed out, further work will be needed at least for Image and BitMap to make their size handling 64-bit too. Possibly other classes based on CowData internally.

[akien-mga]

Thanks!