Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[3.x] Make all file access 64-bit (take 2) #47254

Merged
merged 1 commit into from
May 16, 2021

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Mar 22, 2021

This is a redo of #27247 from @RandomShaper, as a draft as I intend to do some changes.

Opening against 3.x for now as several users need support for PCK files above 2.1 GB, so they can backport this to their custom 3.3 builds. We'll probably target 3.4 for this though as it's a fairly big change, coming a bit too late for 3.3. We could consider cherry-picking for 3.3.x depending on how it goes.
This will of course be forward-ported to master once ready.

Fixes #44363.
Fixes godotengine/godot-proposals#400.

Some changes I plan to make after discussion with @reduz and others in @godotengine/core:

  • It would be better to standardize on uint64_t instead of int64_t in the FileAccess API. That's closer to size_t / off_t and should be good for all these operations.
  • The File bindings should use int64_t where relevant to validate that input is unsigned (as the Variant binding system only has signed ints).
  • I'd revert the Directory::get_space_left() change to bytes instead of MiB as it breaks compat. We can consider doing it for 4.0 but it shouldn't really be in the PR.

I'll make changes as a separate commit for review.


This changes the types of a big number of variables.

General rules:

  • Using int64_t in general. With 64-bit we no longer need to use unsigned type. That matches Variant better and avoids akward casts. Checks for negative numbers are performed where they don't make sense (seek, read/write buffer size).
  • Previous point means no more size_t for file size/offsets.
  • Not worrying about FileAccess::get_64/store_64 working with unsigneds. I haven't found any place where conversion to unsigned will cause trouble and saves us from having to add get/store_64_signed or similar.
  • Using int32_t integers for concepts not needing such a huge range, like pages, blocks, etc.

In addition:

  • Improve usage of integer types in some related places; namely, DirAccess, core binds.
  • The binding for Directory::get_space_left() tried to return the size in MiB, but the expression was missing parentheses, so the 1024 * 1024 is removed and it keeps working as usual, returning bytes.

@akien-mga akien-mga added this to the 3.4 milestone Mar 22, 2021
@akien-mga akien-mga requested review from a team and RandomShaper March 22, 2021 12:21
@akien-mga akien-mga force-pushed the file-access-64-bit branch 2 times, most recently from 5ff1db4 to 0a6ecbe Compare March 22, 2021 15:12
@akien-mga
Copy link
Member Author

akien-mga commented Mar 22, 2021

I'll make changes as a separate commit for review.

Done. As a side note, they're separate for review by @RandomShaper mostly, who is familiar with the first commit and can see the changes.

For others, I'd suggest reviewing the whole PR at once instead of going through the changes twice: https://github.com/godotengine/godot/pull/47254/files

@akien-mga
Copy link
Member Author

akien-mga commented Mar 22, 2021

I've tested this successfully on Linux with a 32-bit build (on a 64-bit host) which allowed me to export a PCK of 2.5 GiB, and run it with the 32-bit editor build.

Needs testing on all other platforms too.

A MRP would be fairly heavy (needs a project which is more than 2.1 GiB), but here's how I create one easily:

@akien-mga akien-mga force-pushed the file-access-64-bit branch 2 times, most recently from d8b0759 to a170310 Compare March 22, 2021 15:33

ERR_FAIL_COND_V_MSG(!d, 0, "Directory must be opened before use.");
return d->get_space_left() / 1024 * 1024; //return value in megabytes, given binding is int
return d->get_space_left() / 1024 * 1024; // Truncate to closest MiB.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was a misunderstanding / the comment was bogus. This does convert bytes to megabytes, it truncates to the nearest MiB and then (supposedly) converts back to KiB.

That being said, the method seems bogus on Linux at least, but that's a separate issue.

Copy link
Member Author

@akien-mga akien-mga Mar 22, 2021

Choose a reason for hiding this comment

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

Heh interesting, the Linux 64-bit implementation seems to be broken.

On my system:

Filesystem      Size  Used Avail Use% Mounted on
/dev/nvme0n1p6   59G   46G   11G  82% /
/dev/nvme0n1p7  196G  172G   23G  89% /home

If I print the get_space_left() for / and /home, I get:

32-bit:

14093373440
25898131456

Checks out taking into account reserved block count I guess? Assuming the value is in bytes and not MiB as the comment suggests.

64-bit:

1208475648
222502912

what?

Copy link
Member

Choose a reason for hiding this comment

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

Also broken on macOS (it's the same shared Unix code):

> df -H
Filesystem          Size   Used  Avail Capacity   iused      ifree %iused  Mounted on
/dev/disk1s5s1      250G    15G    18G    47%    568975 2442362825    0%   /
...
/dev/disk2s2        107G    52G    55G    49%    492742 4294474553    0%   /Volumes/Windows 10
...
> get_space_left()
8983348772864 for "/"
8983348772864 for "/Volumes/Windows 10"

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #47262 to keep track of it.

@akien-mga akien-mga force-pushed the file-access-64-bit branch 3 times, most recently from 59f1fa4 to 58da64c Compare March 22, 2021 16:18
@fire
Copy link
Member

fire commented Mar 22, 2021

Does gdnative have problem when returning the uint64 size of an array?

For example I remember Java having problems with unsigned types. I wanted to figure out the reason why other languages also don't use unsigned types like uint64_t.

@akien-mga
Copy link
Member Author

akien-mga commented Mar 25, 2021

Did some testing on Windows 10 64-bit using binaries in https://downloads.tuxfamily.org/godotengine/testing/godot-3.3-rc-windows-64bit-pck-test.zip built from this PR.

Exporting from 64-bit editor build

The PCK is exported successfully (2.7 GiB in my test project), and can be run by both the 32-bit and 64-bit release export templates (didn't test debug but it should be fine too).

Exporting from 32-bit editor build

The PCK seems to be exported successfully (also 2.7 GiB, exact same size as the PCK from 64-bit editor), yet it seems to be broken. Both the 32-bit and 64-bit release export templates seem to fail running it (sounds like project.binary and my .tscn might have been truncated):

$ ./windows_64_release.exe --main-pack hello32.pck
Godot Engine v3.3.rc.official - https://godotengine.org
OpenGL ES 3.0 Renderer: Radeon RX Vega
OpenGL ES Batching: ON

ERROR: open: res://default_env.tres:1 - Parse Error: Expected '['
   At: scene/resources/resource_format_text.cpp:848
ERROR: load: Condition "err != OK" is true. Returned: RES()
   At: core/io/resource_loader.cpp:208
ERROR: _load: Condition "found" is true. Returned: RES()
   At: core/io/resource_loader.cpp:278
ERROR: SceneTree: Default Environment as specified in Project Settings (Rendering -> Environment -> Default Environment) could not be loaded.
   At: scene/main/scene_tree.cpp:2131
ERROR: open: res://VideoPlayer.tscn:1 - Parse Error: Expected '['
   At: scene/resources/resource_format_text.cpp:848
ERROR: load: Condition "err != OK" is true. Returned: RES()
   At: core/io/resource_loader.cpp:208
ERROR: _load: Condition "found" is true. Returned: RES()
   At: core/io/resource_loader.cpp:278
ERROR: start: Condition "!scene" is true. Returned: false
   At: main/main.cpp:1961
WARNING: cleanup: ObjectDB instances leaked at exit (run with --verbose for details).
     At: core/object.cpp:2132

File sizes are exactly identical:

-rw-r--r-- 1 Remi 197121 2878197088 Mar 25 09:08 hello32.pck
-rw-r--r-- 1 Remi 197121 2878197088 Mar 25 09:07 hello64.pck

But MD5 sums do differ:

$ md5sum.exe hello32.pck
ec63aac7e49b5093641576935c3895de *hello32.pck

$ md5sum.exe hello64.pck
a3a3bb64cde466a41bce42efff458854 *hello64.pck

@akien-mga
Copy link
Member Author

Output from Windows FC (File Compare) on the two PCKs:

$ FC hello64.pck hello32.pck

Comparing files hello64.pck and HELLO32.PCK
***** hello64.pck

res://.import/icon.png-487276ed1e3a0c39cad0279d744ee560.stex_?▒

***** HELLO32.PCK

res://.import/icon.png-487276ed1e3a0c39cad0279d744ee560.stex-_?▒

*****

***** hello64.pck

res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (10).webmPA


***** HELLO32.PCK

res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (10).webmXA


*****

***** hello64.pck

res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (11).webmA▒?

***** HELLO32.PCK

res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (11).webm▒▒?

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (2).webm
0▒A$

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (2).webm
<▒A$

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (3).webm
▒▒1

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (3).webm
▒1

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (4).webm
▒D=

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (4).webm
▒D=

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (5).webm
?▒?I

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (5).webm
?▒?I

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (6).webm
dU▒U

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (6).webm
▒U▒U

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (7).webm
`Ob

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (7).webm
kOb

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (8).webm
DIHn

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (8).webm
xIHn

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (9).webm
@▒?z

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy (9).webm
N▒?z

*****

***** hello64.pck
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy.webm
▒▒E?

***** HELLO32.PCK
res://Spring_-_Blender_Open_Movie.webm.1080p - Copy - Copy.webm
▒▒E?

*****

***** hello64.pck

res://Spring_-_Blender_Open_Movie.webm.1080p - Copy.webm 1
                                                          ?

***** HELLO32.PCK

res://Spring_-_Blender_Open_Movie.webm.1080p - Copy.webm$1
                                                          ?

*****

***** hello64.pck

?▒L?

***** HELLO32.PCK

?▒L?

*****

***** hello64.pck

d_?▒

***** HELLO32.PCK

▒?▒

*****

***** hello64.pck


▒?▒

***** HELLO32.PCK

▒?▒

*****

***** hello64.pck

D▒?▒

***** HELLO32.PCK

O▒?▒

*****

***** hello64.pck

`r?▒

***** HELLO32.PCK

vr?▒

*****

***** hello64.pck

 ▒?▒

***** HELLO32.PCK

I▒?▒

*****

***** hello64.pck

p▒?▒

***** HELLO32.PCK

t▒?▒

*****

Something off around strings?

@akien-mga
Copy link
Member Author

akien-mga commented Mar 25, 2021

I can reproduce the issue with the simplest MRP (new project with a main scene), it seems the PCK generation for Windows 32-bit is broken in this PR. Exporting a PCK with Windows 32-bit editor from 3.3 RC 6 works fine (same size, different MD5 sum), and can be run with the test templates from this PR.

Edit: However with a local build from this branch using MSVC, it seems to work fine. So it might be something specific to the official builds compiled with MinGW-GCC.

@bruvzg
Copy link
Member

bruvzg commented Mar 25, 2021

Something off around strings?

Or with file offset/padding, which is stored right after the strings

f->store_64(pd.file_ofs[i].ofs + header_padding + header_size);

JOELwindows7 added a commit to Perkedel/HexagonEngine that referenced this pull request Apr 20, 2021
oh no. the PCK file exceed 2GB and when we run the export it doesn't work. we don't know how to reduce the import less than 2GB as this is the Godot bug in file system access.

- godotengine/godot#27168 pck more than 2.1 gb error
- godotengine/godot#44363 manually put an export template in root folder of this project
- https://godotengine.org/qa/75329/game-over-and-not-working-what-best-handle-this-godot-stable
- godotengine/godot-proposals#400 it seems that it will only fix in Godot 4.0
- https://docs.godotengine.org/en/stable/getting_started/workflow/export/exporting_pcks.html
- godotengine/godot#44363
- godotengine/godot#47254 akien is fixing, but this PR here is still WIP for now unfortunately

so now we added 2 export template: Windows and Linux, both 64 bit in the project root folder. simply run HexagonEngine.bat for Windows or HexagonEngine.sh for Linux respectively. those script will print "Hexagon Engine by Perkedel Technologies" then run respective export template.
@akien-mga akien-mga force-pushed the file-access-64-bit branch from 58da64c to 45d00d8 Compare May 5, 2021 14:30
@akien-mga
Copy link
Member Author

Rebased after recent codestyle changes in 3.x branch.

@RandomShaper
Copy link
Member

Building on Windows with both MSVC and MinGW-w64 (actually, TDM-GCC-64) I got good exports.

Because of that, I just inspected the code analytically, and I found that the only possibility for the bad offsets in the PCK index is that the _ftelli64() in the MinGW used for the builds is returning a wrong value. I did some searches and couldn't find a confirmation, though.

Something we could try is, in EditorExportPlatform::_save_pack_file(), just before sd.ofs = pd->f->get_position(); doing a flush to see if that somehow forces an update of the internal tracking of the file position.

If that allowed it to generate good PCKs, we'd at least know that such line of investigation goes in the right direction and we could look deeper into that or do one of these:
a) Using something different than _fseeki64()-_ftelli64() under MinGW.
b) Use a more recent release of MinGW if we are not already using the latest one.
c) Under MinGW, force a flush in File::get_position().

@RandomShaper
Copy link
Member

For the record, this table shows the offsets in a test PCK I made, compared between a good and bad build:

Good Bad Error
464 464 0
704 713 9
880 896 16
4304 4335 31
4944 4966 22
8256 8298 42

As you can see, the error is not constant, which, by the way, lead me to rule out some potential causes of the issue.

@bruvzg
Copy link
Member

bruvzg commented May 16, 2021

Relevant commit in the MinGW: mirror/mingw-w64@edeeef2#diff-a68e7d03903adc47c8429d92fe49bc612c166bf22450f71328ba62cfde8dfa7e

The old implementation of _ftelli64() was introduced in 518dd33 in
2007. However it sometimes reports incorrect values. This program, after
being compiled with i686-w64-mingw32-gcc, outputs -15 on the second
line on my Windows 7 Professional:

@akien-mga akien-mga force-pushed the file-access-64-bit branch from 45d00d8 to 4f3b856 Compare May 16, 2021 09:37
@akien-mga akien-mga marked this pull request as ready for review May 16, 2021 09:37
@akien-mga akien-mga requested review from a team as code owners May 16, 2021 09:37
@akien-mga
Copy link
Member Author

akien-mga commented May 16, 2021

With that find, it seems the PR should be good to go, as long as it's compiled with MinGW 8.0.0 or later (which I plan to do for 3.4+, and is already necessary for Vulkan support in 4.0).

I squashed the two commits and amended the commit log to account for the changes I did compared to the original implementation (might need review).

I added a note about the MinGW bug, but ideally if we can figure out a simple patch that works around the issue with #if !defined(__MINGW64__) && defined(__MINGW32_VERSION_MAJOR) && __MINGW32_VERSION_MAJOR < 8 (i.e. 32-bit mingw < 8.0), that'd be good. It's fine IMO if that workaround doesn't handle big files, as long as it still handles files < 2.1 GiB properly.

@akien-mga akien-mga force-pushed the file-access-64-bit branch from 4f3b856 to 3f85ab3 Compare May 16, 2021 09:42
@akien-mga
Copy link
Member Author

akien-mga commented May 16, 2021

I made a test build of that PR for Windows 32-bit and 64-bit using mingw-w64 8.0.0 and GCC 10.3.1 from Fedora 34: https://downloads.tuxfamily.org/godotengine/testing/3.4-beta-pr47254-windows.zip

Note: GitHub Actions is failing, the service is having a major outage now. Will restart the build once it's fixed, but until then we shouldn't merge.

This changes the types of a big number of variables.

General rules:
- Using `uint64_t` in general. We also considered `int64_t` but eventually
  settled on keeping it unsigned, which is also closer to what one would expect
  with `size_t`/`off_t`.
- We only keep `int64_t` for `seek_end` (takes a negative offset from the end)
  and for the `Variant` bindings, since `Variant::INT` is `int64_t`. This means
  we only need to guard against passing negative values in `core_bind.cpp`.
- Using `uint32_t` integers for concepts not needing such a huge range, like
  pages, blocks, etc.

In addition:
- Improve usage of integer types in some related places; namely, `DirAccess`,
  core binds.

Note:
- On Windows, `_ftelli64` reports invalid values when using 32-bit MinGW with
  version < 8.0. This was an upstream bug fixed in 8.0. It breaks support for
  big files on 32-bit Windows builds made with that toolchain. We might add a
  workaround.

Fixes godotengine#44363.
Fixes godotengine/godot-proposals#400.

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@akien-mga akien-mga force-pushed the file-access-64-bit branch from 3f85ab3 to 817ffc0 Compare May 16, 2021 15:52
@akien-mga akien-mga merged commit 30f252b into godotengine:3.x May 16, 2021
@akien-mga akien-mga deleted the file-access-64-bit branch May 16, 2021 18:55
@akien-mga
Copy link
Member Author

I made a test build of that PR for Windows 32-bit and 64-bit using mingw-w64 8.0.0 and GCC 10.3.1 from Fedora 34: https://downloads.tuxfamily.org/godotengine/testing/3.4-beta-pr47254-windows.zip

Did some testing on Windows 10 Home Build 19042 with the above binaries.

Exporting a big PCK (> 2.1 GiB) seems to work fine both when using the Win32 and Win64 editor binaries:
image

The resulting export works fine (tested by playing back the webm video it contains).

The MD5 and SHA256 sums for PCKs exported with the Win32 and Win64 editor binaries are identical, so that confirms that the issue outlined in #47254 (comment) is fixed (binaries built with MinGW-w64 8.0.0).

I then tested loading the big PCK using the File API in that same project, with this code:

func _ready():
	var file = File.new()
	var err = file.open("res://EXPORT/BigProjWin32Debug.pck", File.READ)
	assert(err == OK)
	print(file.get_md5("res://EXPORT/BigProjWin32Debug.pck"))
	var length = file.get_len()
	print(length)
	file.seek(length - 10)
	assert(file.get_position() == length - 10)
	print(file.get_32())

This confirmed that Godot can open and calculate the MD5 sum of this big file (which matches the one calculated by md5sum.exe in git-bash.exe). The length in bytes matches the one reported by Windows.

I did find one bug for which I'll open a dedicated bug report. Trying to get the file contents as a buffer still fails from GDScript:

	file.seek(0)
	var buf = file.get_buffer(length)
	print(buf.size())
ERROR: Size of PoolVector cannot be negative.
   At: ./core/pool_vector.h:501
ERROR: Can't resize data to 2672611824 elements.
   At: core/bind/core_bind.cpp:1967

@RandomShaper
Copy link
Member

The issue is that PoolVector is currently limited to a 32-bit element count, so the file API can't do any better.

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

Successfully merging this pull request may close these issues.

4 participants