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

Initial aarch64-apple-darwin support #123

Merged
merged 4 commits into from
Mar 5, 2021
Merged

Initial aarch64-apple-darwin support #123

merged 4 commits into from
Mar 5, 2021

Conversation

repi
Copy link
Contributor

@repi repi commented Dec 19, 2020

Adds initial support for Mac M1 ARM CPUs.

This may not be correct as had problems running our pxbind structgen utility on Mac M1 so for now just copy'n'pasted the x86_64 Mac structgen files. Does seem to run but could be subtle differences (that could cause catastrophic failures)?

Resolves: #92

@hrydgard
Copy link
Contributor

hrydgard commented Dec 19, 2020

I'm not actually sure what would be different between archs here. It's just a bunch of structs with repr(C), and all the platforms we support are 64-bit and, I think, pretty much entirely layout structs the same way? Do we really need platform specific structgen.rs ?

@repi
Copy link
Contributor Author

repi commented Dec 19, 2020

Yeah I'm not fully sure either, I've done it this way before also of just copying it over from other platforms, @h3r2tic do you know more specifically where compiler/arch differences could affect the structgen within 64-bit?

@repi
Copy link
Contributor Author

repi commented Mar 4, 2021

@h3r2tic what do you think of the risks of just getting this in vs generating a proper definition?

Seems to be working well, been using it for a couple of months and no seen physx issues

@repi repi marked this pull request as ready for review March 4, 2021 21:28
@tgolsson
Copy link
Member

tgolsson commented Mar 4, 2021

I spent a couple of evenings trying to replace the bindgen stuff with an easier to maintain Python version but the clang-bindings lacked some things, so getting a 1-1 match in output wasn't possible. It might be more worthwhile trying to use the libclang Rust bindings to generate it.

@tgolsson
Copy link
Member

tgolsson commented Mar 4, 2021

Added an issue here: #127, if we want to tackle it.

@h3r2tic
Copy link
Contributor

h3r2tic commented Mar 5, 2021

@h3r2tic what do you think of the risks of just getting this in vs generating a proper definition?

Seems to be working well, been using it for a couple of months and no seen physx issues

In reality: who knows! If we reuse x64 struct definitions for arm64, we are crossing fingers and relying on C++ ABI compatibility between those (we rely on repr(C) between C and Rust, but the generated struct layout matches the C++ ABI). In theory we shouldn't be relying on pre-generated struct layouts at all, although they are de-facto stable.

So we should run structgen on aarch64-apple-darwin, but the build process has changed for clang/libtooling, and I'm at a loss trying to get it to work. I'd much rather go with @tgolsson's idea of relying on the devs of Python's or Rust's bindings, and using a sane build system.

Seeing as that's not something we have right now, and considering you've been testing aarch64-apple-darwin for a while, we could just merge this, and at some point replace the generator with one written in not-C++.

@repi
Copy link
Contributor Author

repi commented Mar 5, 2021

Thanks! and agreed, think we should get this in @hrydgard @tgolsson

@mergify mergify bot merged commit 016a12e into main Mar 5, 2021
@mergify mergify bot deleted the mac-aarch64-suppport branch March 5, 2021 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for aarch64-apple-darwin
4 participants