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

Handle return type promotion in the high layer #69

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

uweigand
Copy link
Contributor

As described in #66, the libffi library implicitly extends small integer types used as return types, both in calls and closure callbacks. This is not currently addressed by the existing code base.

For the low and middle layers, the user can manually address this issue by using appropriate return types. However, for the high layer, the types are automatically determined, and are currently incorrect. This leads to problems (primarily) on big-endian platforms.

This PR addresses the return type extension in the high layer by storing the extended type used by libffi as an associated type RetType in the CType trait, and using that type when performing calls or constructing closure callbacks.

This implementation does not change the libffi-rs API, and should not add any significant run-time overhead.

@yorickpeterse
Copy link
Collaborator

Are these changes backwards compatible, or should they be accompanied by a major version bump?

@uweigand uweigand force-pushed the retval-extend branch 2 times, most recently from 223f054 to 2810a1f Compare February 23, 2023 10:08
@uweigand
Copy link
Contributor Author

Are these changes backwards compatible, or should they be accompanied by a major version bump?

These changes should be backwards compatible. There should be no change in the public API, and the only changes in generated code actually fix the underlying bug (missing type promotion).

I noticed that tests were failing on Rust 1.48.0, because I was using the unwrap_unchecked function which isn't available yet in that release. I've replaced this now, which should remove those failures.

After re-running the tests I still see two failures that do not appear to be obviously related to my changes, not sure what's going on there.

As described in tov#66, the
libffi library implicitly extends small integer types used as
return types, both in calls and closure callbacks.  This is not
currently addressed by the existing code base.

For the low and middle layers, the user can manually address this
issue by using appropriate return types.  However, for the high
layer, the types are automatically determined, and are currently
incorrect.  This leads to problems (primarily) on big-endian
platforms.

This PR addresses the return type extension in the high layer
by storing the extended type used by libffi as an associated
type `RetType` in the `CType` trait, and using that type when
performing calls or constructing closure callbacks.

This implementation does not change the libffi-rs API, and should
not add any significant run-time overhead.
@uweigand
Copy link
Contributor Author

Ping? Any other questions or issues with this patch?

@yorickpeterse yorickpeterse merged commit 7acf2a2 into tov:master Mar 28, 2023
@uweigand uweigand deleted the retval-extend branch March 28, 2023 15:02
@uweigand
Copy link
Contributor Author

Thank you! With those PRs merged, the test suite now fully passes on s390x.

In order to refer to the updated libffi-rs from other rust packages, it would be good to have a new version on cargo. Do you have an outlook when you're planning to publish the next version?

@yorickpeterse
Copy link
Collaborator

@uweigand All recent changes have just been pushed as new versions of libffi-sys and libffi 😃

@uweigand
Copy link
Contributor Author

@uweigand All recent changes have just been pushed as new versions of libffi-sys and libffi smiley

Perfect! Thank you very much for your support :-)

@uweigand
Copy link
Contributor Author

Unfortunately trying to pull the new release into upstream rust/miri failed:

  ../src/tramp.c:262:22: error: call to undeclared function 'open_temp_exec_file'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    tramp_globals.fd = open_temp_exec_file ();
                       ^
  1 error generated.

This looks like it has been fixed upstream in libffi here: libffi/libffi#764
(Not sure why I didn't see this when doing standalone build - they must be using different compiler options ...)

What would be the best way forward here? Just backport that fix?

@yorickpeterse
Copy link
Collaborator

@uweigand A new release of libffi would have to be published. Maintaining a set of patches isn't something we're interested in.

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.

2 participants