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

ffi_backend: convert numeric function args to pointers #162

Merged
merged 11 commits into from
Dec 11, 2024

Conversation

danini-the-panini
Copy link
Contributor

This allows for passing integers as pointer arguments to functions when using the FFI backend. This is a workaround until we can get JRuby's FFI implementation to allow for it directly (see also jruby/jruby#8423)

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for this case?

lib/fiddle/ffi_backend.rb Outdated Show resolved Hide resolved
@headius
Copy link

headius commented Dec 4, 2024

Passing integers as pointers might be a good feature to add, but patching it into JRuby led to failures in FFI specs that confirm integers are prevented from being passed as pointers. I've asked @danini-the-panini to propose it to ffi/ffi and we can discuss from there.

The workaround provided here will be needed until that discussion and feature can happen.

@danini-the-panini danini-the-panini changed the title ffi_backend: convert int function args to pointers ffi_backend: convert numeric function args to pointers Dec 5, 2024
@danini-the-panini
Copy link
Contributor Author

I've also allowed any integer coercible to passed in as a pointer address, to align with native Fiddle

lib/fiddle/ffi_backend.rb Outdated Show resolved Hide resolved
Co-authored-by: Benoit Daloze <eregontp@gmail.com>
@larskanis
Copy link
Contributor

This looks good to me now.

@larskanis
Copy link
Contributor

The idea of accepting integers where a pointer is expected in a function call is more or less rejected in ffi/ffi#1130 . So this PR seems to be the way to proceed.

Copy link

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/fiddle/ffi_backend.rb Outdated Show resolved Hide resolved
lib/fiddle/ffi_backend.rb Outdated Show resolved Hide resolved
test/fiddle/test_function.rb Outdated Show resolved Hide resolved
ptr = Pointer.new 0
assert_equal ptr, Pointer[0]
end

def test_to_ptr_with_num
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_to_ptr_with_num
def test_to_ptr_with_float

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't really testing float specific (see my comment below) but rather testing integer coercion. Perhaps this is a more apt name?

Suggested change
def test_to_ptr_with_num
def test_to_ptr_with_int_coercion

ptr = Pointer.new 0
assert_equal ptr, Pointer[0]
end

def test_to_ptr_with_num
ptr = Pointer.new 0
assert_equal ptr, Pointer[0.0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it may be better that we reject Float for address. Because Float address is invalid.

@tenderlove What do you think about this?

Copy link
Contributor Author

@danini-the-panini danini-the-panini Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I added this test is to make sure the functionality of the FFI backend matches the C implementation. I just picked an arbitrary builtin that can be coerced into an integer 🤷🏻‍♀️ There might be other types that make more sense, or I could just create a dummy int-wrapper and use that instead, e.g.

IntWrapper = Struct.new(:value) do
  def to_int
    value
  end
end

and then

Suggested change
assert_equal ptr, Pointer[0.0]
assert_equal ptr, Pointer[IntWrapper.new(0)]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is just a test for coercion, I guess. @danini-the-panini maybe adding the wrapper will show the intention of the test better? I don't really have a strong opinion about float vs wrapper object as long as the intention of the test is clear. Either a comment or the wrapper class seems sufficient to me.

I assume this is calling to_int on things because it wants to accept other pointer instances as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's calling rb_Integer in C, and Kernel#Integer in the FFI backend

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kou @tenderlove I've updated it to use a wrapper and renamed the test method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's calling rb_Integer in C, and Kernel#Integer in the FFI backend

ah, gotcha.

@kou @tenderlove I've updated it to use a wrapper and renamed the test method

Thank you!

@tenderlove tenderlove merged commit e2f0952 into ruby:master Dec 11, 2024
51 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 16, 2024
(ruby/fiddle#162)

This allows for passing integers as pointer arguments to functions when
using the FFI backend. This is a workaround until we can get JRuby's FFI
implementation to allow for it directly (see also
jruby/jruby#8423)

---------

ruby/fiddle@e2f0952e9b

Co-authored-by: Benoit Daloze <eregontp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants