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

Refactor all uses of GEN_FCN to use locals #52

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

hgmich
Copy link
Contributor

@hgmich hgmich commented Mar 7, 2022

An intermediate local value (cast to the appropriate signature) for lookups in the instruction generation jump table works around a code generation bug in macOS ARM64 clang that would cause their parameters to be pushed on the stack instead of passed in registers.

@hgmich
Copy link
Contributor Author

hgmich commented Mar 7, 2022

(addresses #43)

An intermediate local value for lookups in the instruction generation jump
table works around a code generation bug in macOS ARM64 clang that would cause
their parameters to be pushed on the stack instead of passed in registers.
@hgmich
Copy link
Contributor Author

hgmich commented Mar 15, 2022

I've had feedback on the pret Discord server that this works and people are able to build projects such as pokefirered on M1 Macs.

Further research indicates that both vanilla Clang and gcc are also affected, so this might be a more general ABI problem. Either way, the issue is fixed by these changes; it's a matter of if they're acceptable to merge given the extent of them.

@hgmich
Copy link
Contributor Author

hgmich commented May 5, 2022

I've recently found some documentation indicating these may be consequences of deliberate changes made to the ARM64 ABI on Apple platforms: https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms#Update-Code-that-Passes-Arguments-to-Variadic-Functions

@rawr51919
Copy link

rawr51919 commented Jun 18, 2022

@holmesmr
If you use Fixes #43 or similar on the first comment of a PR, it'll make that PR so it'll close the issue once it's merged.
Also works with Resolves #43 and Closes #43
This may also fix #46 and #38 (even though #38 already has a fix in #39)

@luckytyphlosion luckytyphlosion merged commit d59cfb5 into pret:master Sep 2, 2022
laqieer added a commit to laqieer/fireemblem8u that referenced this pull request Mar 23, 2023
You needn't to build holmesmr's fork of agbcc on Mac because that fix for Mac has been merged in pret/agbcc#52
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.

3 participants