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

Improve architectures in OS::has_feature and make them work on MSVC #61732

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

aaronfranke
Copy link
Member

This needs testing! I have not yet tested if this works as expected.

Based on this MSDN article, we need to check for _M_IX86, _M_X64, _M_ARM, and _M_ARM64 to detect x86 and ARM on MSVC. This should make the x86_64, x86, arm64, and arm OS features work on MSVC.

I modified the logic a bit so that x86_32 and arm32 are for the 32-bit versions specifically, and x86 and arm will return true for both the 32-bit and 64-bit versions. I also added WebAssembly (wasm, wasm32, wasm64) to the list of architectures, based on the defines here. For these reasons, this PR is also an enhancement.

We should also backport some of these changes to 3.x (except leave "x86" and "arm" as they are for backwards compatibility for any 3.x projects using those to detect 32-bit versions of the architectures).

core/os/os.cpp Outdated
if (p_feature == "arm64") {
return true;
}
#elif defined(__arm__)
#else
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#else
#elif defined(__arm__) || defined(_M_ARM)

Copy link
Member

Choose a reason for hiding this comment

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

This was marked as resolved but I still think it makes sense. It's not needed per se due to the previous-level list of conditions, but that's more explicit and consistent with what is done for x86 arches.

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 keep flip-flopping on this, but I added it in again and pushed to the PR.

@againey
Copy link
Contributor

againey commented Jun 5, 2022

The #if conditions seem to work for MSVC (VS2022) x86, both 32 and 64 bit. ✔️

But when I run the example, I get a new error I hadn't noticed before, at object.cpp:1771

ERR_FAIL_COND(_instance_bindings != nullptr);

The instance bindings are first created at object.cpp:1794 while calling Node::get_viewport from Example::return_something_const. Then shortly after, within Viewport::___binding_create_callback, it hits Wrapped::_postinitialize and tries to set the instance bindings again.

My suspicion is that this is unrelated, but this is far beyond my current understanding, since I just started looking at Godot code yesterday. I'll see if I can investigate further tomorrow.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Works with VS 2019 command line tools (both 32 and 64 bits), and does not cause regression on mingw.

As a note, we might want (at some point) to make the architecture and build feature(s) generated by the build system and included instead of detected via macros to better support different toolchains.

I am seeing a crash on the windows 32 bits build, but it is unrelated to this PR. I've confirmed that the correct features are set using the following script:

extends MainLoop

func _initialize():
        for feat in ["x86_32", "x86_64"]:
                print("%s: %s" % [feat, OS.has_feature(feat)])

@akien-mga akien-mga merged commit 3f0ce75 into godotengine:master Jun 6, 2022
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants