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

Make all file access 64-bit (uint64_t) #48768

Merged
merged 1 commit into from
May 17, 2021

Conversation

akien-mga
Copy link
Member

Forward port of #47254 to master. Draft for now as I haven't tested the changes yet, nor done a full pass to check potential type mismatches on calls to get_buffer() and similar.


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 #44363.
Fixes godotengine/godot-proposals#400.

@akien-mga
Copy link
Member Author

I made a test build for Windows (32-bit and 64-bit, both editor and templates) to help validate this PR: https://downloads.tuxfamily.org/godotengine/testing/4.0-dev-pr48768-windows.zip

See #47254 (comment) for suggestions on how to make a reproduction project.

@akien-mga
Copy link
Member Author

akien-mga commented May 17, 2021

I made a test build for Windows (32-bit and 64-bit, both editor and templates) to help validate this PR: https://downloads.tuxfamily.org/godotengine/testing/4.0-dev-pr48768-windows.zip

Did some testing with this build like in #47254 (comment)

I have the same results, big PCKs are working properly, and this error also happens when trying to get too big a buffer:

ERROR: Condition "p_size < 0" is true. Returning: ERR_INVALID_PARAMETER
   at: resize (./core/templates/cowdata.h:260)
ERROR: Can't resize data to 2672612761 elements.
   at: (core/core_bind.cpp:1331)

That seems to be related to #46842.

Also tested the "Embed PCK" option with a Win32 binary exported from the Win32 editor and it works fine.

Edit: However I seem to get a different MD5 sum every time I export, so it seems there's something that makes the PCK non-deterministic in master.

@akien-mga akien-mga marked this pull request as ready for review May 17, 2021 09:47
@akien-mga akien-mga requested review from a team as code owners May 17, 2021 09:47
@akien-mga akien-mga removed request for a team May 17, 2021 09:48
@akien-mga akien-mga requested review from a team and removed request for a team May 17, 2021 09:48
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 requested a review from a team as a code owner May 17, 2021 13:07
@akien-mga akien-mga removed the request for review from a team May 17, 2021 13:07
@akien-mga
Copy link
Member Author

Should be good to go as since it works in my tests (I just pushed an amend to handle a few missed get_len() and get_buffer() return types that could be uint64_t).

@akien-mga akien-mga merged commit b0a51bf into godotengine:master May 17, 2021
@akien-mga akien-mga deleted the file-access-64-bit-4.0 branch May 17, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants