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

Fix some issues with Fiddle #4511

Merged
merged 19 commits into from Mar 1, 2017

Conversation

Projects
None yet
3 participants
@jellymann
Contributor

jellymann commented Feb 26, 2017

After having some trouble getting Mittsu working in JRuby, I thought I'd have a look at JRuby's Fiddle implementation. There are quite a number of inconsistencies and straight-up broken bits, so I thought I'd take a crack at some of them. This is not a conclusive fix or anything, but hopefully I can get it working enough for most.

Here are the list of the bigger changes:

  1. TYPE_PTRDIFF_T was not defined. This has been voiced before (#3477). I had a look, and other types such as TYPE_SIZE_T, etc. were also missing. I've added all the missing types, basing their byte sizes and alignment on TYPE_VOIDP
  2. Make Fiddle::Pointer behave more like MRI
    a. #[] now gets the correct number of bytes, regardless of null-terminators
    b. Added Pointer#[]=
    c. #to_s and #to_str should behave differently when len is omitted. Notably, #to_s takes in to account null-terminators, while to_str uses the Pointer size
    d. Throw DLError instead of FFI errors in some cases
    e. Pointer.new accepts anything that can be converted to an Integer as an address, using Integer()
    f. Added Pointer#free and Pointer#free=. @free is now stored as an integer.
    g. Pointer.malloc does not assign a default free function to the resulting pointer
    h. Added Ponter#== and Pointer#<=>
  3. Added some missing items in the Fiddle module, notably
    a. Fiddle.malloc and Fiddle.free, which map directly to their libc counterparts
    b. Fiddle::RUBY_FREE, which is just the Integer memory address of the libc free function
    c. Fiddle::BUILD_RUBY_PLATFORM, which I've just set to RUBY_PLATFORM, which is always "java"
    d. Fiddle::NULL, a null pointer
    e. Fiddle::LibC, an FFI wrapper to libc, not exactly required, but it serves to facilitate the implementation of malloc and free (also resolves #3462)
  4. Added a check to fiddle/helper in the tests, where the location of libc dynamic library could not be found because platform is always "java". Changed it so that it uses RbConfig to detect the actual OS.

There are a great deal of things I have not been able to address thus far, most notably:

  1. Pointer.to_ptr(string) makes a copy of the given string instead of wrapping the string itself. This is evident when you attempt to modify the data through the pointer, only to find the string was not modified. This issue affects the case of shovelling into the string, where in JRuby the Pointer data is then not reflected. I believe in order to rectify this we would have to get the address of the underlying ByteList of the string, but i worry that this will be infeasible in Java, especially with the JVM not guaranteeing the memory address of all it's object all the time.
  2. Pointer.to_ptr(file) does not work. Right not it will use File#to_i to get the address, which is obviously incorrect. MRI converts the File object to a libc FILE, and I am unsure as to how that could be achieved in JRuby.
  3. Fiddle::Function returns an FFI::Pointer instead of Fiddle::Pointer. I've made an issue here (#4510)
  4. Fiddle::Function does not accept certain values where MRI's fiddle does. For example, it does not appear to accept integers in place of pointers, or even subclasses of Fiddle::Closure for that matter. This, and point 3, seem to be that Fiddle::Function is just a straight-up wrapper of FFI::Function, so it only accepts FFI arguments and returns FFI values.
  5. Related to point 1, and probably an easy fix, is that Pointer.to_ptr(string) returns a Pointer 1 byte bigger than on MRI. I think that probably has to do with null-terminators or something, although the behaviour doesn't seem to be affected if I "fix" the issue. I reverted the fix since I wanted some more input on the matter.

My personal observation is that it seems JRuby's Fiddle implementation was hastily put together on top of FFI once Ruby adopted Fiddle as the defacto foreign-function interface. MRI's Fiddle is written directly on top of libffi, and in C. Perhaps it is time that JRuby's Fiddle was build directly on top of libffi/jffi, and in Java instead of Ruby.

jellymann added some commits Feb 25, 2017

feat: add missing RUBY_BUILD_PLATFORM
I just made it equal to RUBY_PLATFORM, which will always be Java
Make Pointer#free and act more like MRI
Pointer#free= should accept an Integer or Pointer
Pointer#free should return a new instance of Fiddle::Function
Pointer#free should return nil if free is zero
Fiddle::Pointer.new should accept anything Integer-like as an address
Behaves more like MRI. Previously passing something in that was neither an FFI::Pointer nor an Integer would cause undesirable results.
Make Pointer#to_s and Pointer#to_str behave like MRI
The difference is, without passing `len` in to the function:
1. to_s returns a null-terminated string
2. to_str returns a string of as many bytes a the pointer's size
Make Pointer#[]= act even more like MRI
Some hidden, undocumented "features" of Pointer#[]= include:
1. Passing a Pointer instead of a String, whereby data is copied from the passed-in pointer
2. Passing an Integer instead of a string, using the integer as a memory address to copy from

@enebo enebo added this to the JRuby 9.2.0.0 milestone Feb 27, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 1, 2017

Member

Thanks for this! Our Fiddle implementation has not had a lot of love since the initial implementation, so any help you can provide here is quite welcome!

We've decided to up the priority on this and get it into 9.1.8.0 rather than waiting for 9.2. If you can help fix the remaining issues after 9.1.8.0 that would be a great help also. I'll take a quick look at them today.

Member

headius commented Mar 1, 2017

Thanks for this! Our Fiddle implementation has not had a lot of love since the initial implementation, so any help you can provide here is quite welcome!

We've decided to up the priority on this and get it into 9.1.8.0 rather than waiting for 9.2. If you can help fix the remaining issues after 9.1.8.0 that would be a great help also. I'll take a quick look at them today.

@headius headius merged commit 70a471e into jruby:master Mar 1, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@headius headius modified the milestones: JRuby 9.1.8.0, JRuby 9.2.0.0 Mar 1, 2017

headius added a commit that referenced this pull request Mar 1, 2017

Add Fiddle tests to MRI suite index.
Thanks to @jellyman for Fiddle fixes that enabled these tests to
run. See #4511.

headius added a commit that referenced this pull request Mar 1, 2017

Revert "Merge pull request #4511 from jellymann/ds-fix-fiddle"
This reverts commit 70a471e, reversing
changes made to af0556b.

headius added a commit to headius/jruby that referenced this pull request Mar 1, 2017

Merge pull request #4511 from jellymann/ds-fix-fiddle
This is a re-merge on a separate branch due to Wnidows breakage
caused by the original fixes.

headius added a commit to headius/jruby that referenced this pull request Mar 1, 2017

Add Fiddle tests to MRI suite index.
This is a re-commit on a new branch because of Windows breakage
caused by #4511.

@headius headius referenced this pull request Mar 1, 2017

Open

Fiddle fixes #4518

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 1, 2017

Member

Ok, so these looked nice on Linux but it appears to have broken something on Windows. I've created #4518 containing your changes and my commit to enable Fiddle tests. We can iterate together on getting things working right based on https://github.com/headius/jruby/tree/fiddle-fixes.

Member

headius commented Mar 1, 2017

Ok, so these looked nice on Linux but it appears to have broken something on Windows. I've created #4518 containing your changes and my commit to enable Fiddle tests. We can iterate together on getting things working right based on https://github.com/headius/jruby/tree/fiddle-fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment