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

Activate strict FFI #15683

Merged

Conversation

guillep
Copy link
Member

@guillep guillep commented Dec 4, 2023

FFI callouts should (unless told otherwise) specify the types for all call arguments.
Otherwise this leads to ambiguities that the FFI engine cannot correctly state:

what is the type of 0? signed or unsigned? 1, 2, 4 or 8 bytes long?
what is the type of 1.5? a float or a double?
what is the type of self?

When the strict mode is disabled, types are inferred, and code will execute mistakenly in many cases.
Important: This could lead to crashes.
Instead, the strict mode just throws an exception when this happens.

Notice that this option can be disabled locally per library (just redefining the options method), but I strongly encourage people to fix their lib bindings instead of using the non-strict mode.

Regarding adoption in the Pharo core IDE, we should check if this works as is, or if we should disable it momentarily for some of the libraries used internally in Pharo (e.g., SDL) that would prevent us migrating.

However, this cannot be integrated yet as SDL has some functions that are not properly mapped

FFICallout>>unsupportedUntypedLiteral:
FFIStrictResolutionMode>>resolveUndeclaredTypeForArgument:withResolver:
FFICallout>>resolveUntypedArgument:
[…]
SDL_Texture(Object)>>ffiCall:
SDL_Texture>>lockPixels:pitch:
[…]
OSSDL2FormRenderer>>updateAreas:immediate:
OSWorldRenderer>>updateDamage:

@MarcusDenker MarcusDenker added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Dec 4, 2023
@guillep guillep changed the title Revert "Revert "[WIP] Redo "Activate strict FFI"""" Activate strict FFI Dec 6, 2023
@Ducasse Ducasse closed this Dec 8, 2023
@Ducasse Ducasse deleted the revert-15678-revert-15628-revert-15627-revert-15506-strictffi branch December 8, 2023 13:23
@jecisc jecisc restored the revert-15678-revert-15628-revert-15627-revert-15506-strictffi branch December 8, 2023 13:28
@jecisc jecisc reopened this Dec 8, 2023
@guillep guillep marked this pull request as ready for review February 14, 2024 14:37
@guillep guillep merged commit 0a42f6a into Pharo12 Feb 14, 2024
2 of 3 checks passed
@guillep guillep deleted the revert-15678-revert-15628-revert-15627-revert-15506-strictffi branch February 14, 2024 14:37
@jecisc jecisc mentioned this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Need more work The issue is nearly ready. Waiting some last bits.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants