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

Enforce restype and argtypes for send_super #219

Closed
dgelessus opened this issue Jan 21, 2022 · 2 comments · Fixed by #220
Closed

Enforce restype and argtypes for send_super #219

dgelessus opened this issue Jan 21, 2022 · 2 comments · Fixed by #220
Labels
enhancement New features, or improvements to existing features.

Comments

@dgelessus
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
rubicon.objc.runtime.send_super doesn't check its arguments very much: it's possible to pass arguments without declaring their argtypes, in which case the arguments are assumed to be C-style varargs, with default types assigned by ctypes.

This is problematic because the assumed default argument types don't always match what is required/intended (e. g. integer sizes). On some architectures like AArch64 (iOS and ARM Macs), fixed and variable arguments are also passed in different incompatible ways (registers vs. stack), so assuming that all arguments are varargs doesn't work in most cases.

Describe the solution you'd like
For send_message, we have already introduced some stricter checks (see #174), so we should implement the same checks for send_super as well. This is a backwards-incompatible change - any existing code that calls send_super and doesn't already set restype/argtypes will have to be updated.

Describe alternatives you've considered
Instead of throwing a hard error, we could just show warnings if restype/argtypes are missing/mismatched. This would allow existing code to keep working on Intel and give callers more time to update their code. ARM would be broken either way though - if we don't throw an error, most affected calls will not have their arguments passed correctly, and will probably crash.

Additional context
The confusion of fixed and variable arguments has already caused some crashes in Toga on ARM Macs. This has been fixed now (beeware/toga#1406, beeware/toga#1411), but adding these checks in send_super would help detect similar problematic calls.

@dgelessus dgelessus added enhancement New features, or improvements to existing features. up-for-grabs labels Jan 21, 2022
@samschott
Copy link
Member

samschott commented Feb 15, 2022

Instead of throwing a hard error, we could just show warnings

I would advocate for a hard error, this will be easier to trouble-shoot than a crash in many cases. It would also be consistent with send_message which already raises a TypeError in such cases.

This of course isn't backward compatible but will hopefully force rubicon-objc users to keep up with the migration to ARM Macs. One of the major users, toga, at least is under our direct control...

This was referenced Feb 15, 2022
@freakboy3742
Copy link
Member

I agree that the hard error is preferable here, even with the backwards incompatibility. If someone is relying on the fact that the error fails silently on Intel, they've got a bug in their code - they just don't know about it. It's arguable whether catching a hidden bug is a break in backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants