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

Fix return type encoding for ptrcall. #721

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Mar 9, 2022

Fixes return and temporary variable types used for ptrcall according to conversions defined here:

MAKE_PTRARGCONV(bool, uint8_t);
// Integer types.
MAKE_PTRARGCONV(uint8_t, int64_t);
MAKE_PTRARGCONV(int8_t, int64_t);
MAKE_PTRARGCONV(uint16_t, int64_t);
MAKE_PTRARGCONV(int16_t, int64_t);
MAKE_PTRARGCONV(uint32_t, int64_t);
MAKE_PTRARGCONV(int32_t, int64_t);
MAKE_PTRARG(int64_t);
MAKE_PTRARG(uint64_t);
// Float types
MAKE_PTRARGCONV(float, double);

Prevents stack corruption in the methods like int Node::get_child_count(bool p_include_internal) const, which are using 32-bit int return.

@bruvzg bruvzg added bug This has been identified as a bug crash topic:gdextension This relates to the new Godot 4 extension implementation labels Mar 9, 2022
@bruvzg bruvzg added this to the 4.0 milestone Mar 9, 2022
binding_generator.py Outdated Show resolved Hide resolved
@bruvzg bruvzg marked this pull request as draft March 9, 2022 11:06
@bruvzg bruvzg force-pushed the fix_return_type_encoding branch from 0815d03 to 2b5ce66 Compare March 9, 2022 11:49
@bruvzg
Copy link
Member Author

bruvzg commented Mar 9, 2022

DownloadString('https://get.scoop.sh/') in the windows CI seems to be randomly failing.

@akien-mga
Copy link
Member

akien-mga commented Mar 9, 2022

DownloadString('https://get.scoop.sh/') in the windows CI seems to be randomly failing.

It seems to be a change in the script at https://get.scoop.sh, it wants us to set -RunAsAdmin:

# Detect if RunAsAdministrator, there is no need to run as administrator when installing Scoop.
    if (!$RunAsAdmin -and (Test-IsAdministrator)) {
        Deny-Install "Running the installer as administrator is disabled by default, use -RunAsAdmin parameter if you know what you are doing."
    }

Here: https://github.com/godotengine/godot-cpp/blob/master/.github/workflows/ci.yml#L76

Testing a potential fix in #722

@bruvzg
Copy link
Member Author

bruvzg commented Mar 9, 2022

It seems to be a change in the script at https://get.scoop.sh/, it wants us to set -RunAsAdmin:

It downloaded it successfully in the Windows (x86_64, MSVC) task, and my other PR failed once, but worked after I have restarted it, so it might be something else.

@bruvzg bruvzg marked this pull request as ready for review March 9, 2022 12:05
@akien-mga
Copy link
Member

Rebasing on #723 should fix the CI.

@bruvzg bruvzg force-pushed the fix_return_type_encoding branch from 2b5ce66 to b8b9a2f Compare March 9, 2022 15:18
@akien-mga akien-mga merged commit f4d2bfd into godotengine:master Mar 9, 2022
@akien-mga
Copy link
Member

Thanks!

@bruvzg bruvzg deleted the fix_return_type_encoding branch March 9, 2022 17:25
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 crash topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants