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.

Sign up for GitHub

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

Jump to bottom

Revert struct-padding workaround once upstream bug is fixed in JNA #334

Closed
rfk opened this issue Oct 26, 2020 · 4 comments · Fixed by #995
Closed

Revert struct-padding workaround once upstream bug is fixed in JNA #334

rfk opened this issue Oct 26, 2020 · 4 comments · Fixed by #995
Assignees
@badboy
Labels
good first issue Good for newcomers to get started with development

Comments

@rfk
Copy link
Collaborator

rfk commented Oct 26, 2020 •
edited by data-sync-user
Loading

The code generated by uniffi makes heavy use of passing structs by value, and this seems to have uncovered an argument-passing bug in JNA. We landed a workaround for this bug in #335 but it works by artificially padding our structs, so it seems likely to have some runtime performance costs.

(Briefly: it pads our structs to be more than 16 bytes in size, so that they will always be passed as a pointer, which works around the bug).

Once the underlying issue has been resolved in a JNA release, we should revert the above workaround.

┆Issue is synchronized with this Jira Task
┆Issue Number: UNIFFI-33
rfk added a commit that referenced this issue Oct 26, 2020
@rfk
Add workaround for JNA bug when passing small structs by value.
2f03a37 
The current version of JNA appears to have a bug when passing small structs
as arguments by value, on arm64 platforms, when the function call has too
many arguments to pass the struct in registers. It turnsout that our generated
code does this a lot thanks to the way we pass around `RustBuffer` structs.

This commit adds a temporary workaround for the issue, by padding our structs
to be larger than 16 bytes, which causes them to be passed as pointers rather
than by value.

See mozilla/application-services#3646 for investigation
of the JNA issue, and #334 to track
the work of removing this workaround once a fix is released in upstream JNA.
@rfk rfk mentioned this issue Oct 26, 2020
Merged
rfk added a commit that referenced this issue Oct 26, 2020
@rfk
Add workaround for JNA bug when passing small structs by value.
ab7a447 
The current version of JNA appears to have a bug when passing small structs
as arguments by value, on arm64 platforms, when the function call has too
many arguments to pass the struct in registers. It turns out that our generated
code does this a lot thanks to the way we pass around `RustBuffer` structs.

This commit adds a temporary workaround for the issue, by padding our structs
to be larger than 16 bytes, which causes them to be passed as pointers rather
than by value.

See mozilla/application-services#3646 for investigation
of the JNA issue, and #334 to track
the work of removing this workaround once a fix is released in upstream JNA.
@rfk
Copy link
Collaborator Author

rfk commented Jul 20, 2021

The latest release of JNA (5.8) has an updated version of libffi and appears to have fixed the issue!

@rfk rfk mentioned this issue Jul 20, 2021
Merged
@rfk rfk added get involved Good opportunity for deeper-dive onboarding good first issue Good for newcomers to get started with development and removed get involved Good opportunity for deeper-dive onboarding labels Jul 20, 2021
@rfk
Copy link
Collaborator Author

rfk commented Jul 20, 2021

I believe this bug is now actionable, and could make a good first issue. Basically, we need to:

  • Remove the padding and padding2 fields that were added in Add workaround for JNA bug when passing small structs by value. #335 (there are other cleanups in that PR, so we shouldn't revert the whole thing). This will need to be done in both the Rust definitions, and the various foreign-language bindings.
  • Document somewhere in the manual that we require JNA 5.7 or greater; I don't think we have a good place for this yet, but it could go somewhere in the "Kotlin" section.
  • Update docker/Dockerfile-build to install the newer version of JNA; this won't have any effect on tests passing because we weren't able to repro that crash in CI, but it'll be useful for completeness.

We'll also have to keep a careful eye out for any increase in crashes on Android once the new version rolls out to consumers.

@badboy
Copy link
Member

badboy commented Aug 3, 2021

I will take a shot at this.

@rfk
Copy link
Collaborator Author

rfk commented Aug 3, 2021

sounds good - especially getting your head around the docker image and tests will be valuable as well

badboy added a commit to badboy/uniffi-rs that referenced this issue Aug 3, 2021
@badboy
Remove workaround for JNA bug when passing small structs by value.
d3bf387 
This (partially) reverts commit 937f9b7

The upstream JNA bug[1] has been resolved.
JNA 5.7 includes the fix.

We're upgrading our Docker test image to 5.8 already.

Fixes mozilla#334

[1]: java-native-access/jna#1259
@badboy badboy mentioned this issue Aug 3, 2021
Merged
badboy added a commit to badboy/uniffi-rs that referenced this issue Aug 3, 2021
@badboy
Remove workaround for JNA bug when passing small structs by value.
1e580ce 
This (partially) reverts commit 937f9b7

The upstream JNA bug[1] has been resolved.
JNA 5.7 includes the fix.

We're upgrading our Docker test image to 5.8 already.

Fixes mozilla#334

[1]: java-native-access/jna#1259
badboy added a commit to badboy/uniffi-rs that referenced this issue Aug 4, 2021
@badboy
Remove workaround for JNA bug when passing small structs by value.
2faf208 
This (partially) reverts commit 937f9b7

The upstream JNA bug[1] has been resolved.
JNA 5.7 includes the fix.

We're upgrading our Docker test image to 5.8 already.

Fixes mozilla#334

[1]: java-native-access/jna#1259
@rfk rfk closed this as completed in #995 Aug 5, 2021
rfk pushed a commit that referenced this issue Aug 5, 2021
@badboy @rfk
Remove workaround for JNA bug when passing small structs by value.
61ae695 
This (partially) reverts commit 937f9b7

The upstream JNA bug[1] has been resolved.
JNA 5.7 includes the fix.

We're upgrading our Docker test image to 5.8 already.

Fixes #334

[1]: java-native-access/jna#1259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@badboy badboy

Labels
good first issue Good for newcomers to get started with development
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove workaround for JNA bug when passing small structs by value. badboy/uniffi-rs
2 participants
@badboy @rfk