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

Update shared FFI code #5948

Merged
merged 29 commits into from Feb 18, 2020
Merged

Update shared FFI code #5948

merged 29 commits into from Feb 18, 2020

Conversation

headius
Copy link
Member

@headius headius commented Oct 28, 2019

This PR will track the process of getting files we have in common with the ffi gem updated so we can share updates and maintenance effort.

See #5947

@headius headius added this to the JRuby 9.2.10.0 milestone Oct 28, 2019
The load path tweaking was breaking other requires, and this
change aligns how we load JRuby's version of the extension.
@@ -1,139 +0,0 @@
module Platform;end
Copy link
Contributor

Choose a reason for hiding this comment

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

jruby specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think these are template files that originally lived outside of the generation logic but now live in some form in the gen/ dir?

This fixes issues building the test library on Darwin. There's no
reason to build for a platform we won't test.
I'm not sure what the type mapping from the C version corresponds
to in the Java version.
* Fill in missing read/write/put/get aliases
* Add put and get with type
* Change TypeError to ArgumentError
* Don't redispatch to Ruby while looking up type
* AutoPointer is in Ruby now
* Change TypeError to ArgumentError for pointer type failure

Note that this moves AutoPointer cleanup logic from our JVM
Reference-based reaper to one based on ObjectSpace finalizers.
This will mean additional overhead for AutoPointer compared to
the original logic.
This caching breaks code that attempts to layout the same struct
class more than once, as in struct_spec.rb's layout specs that
reopen PairLayout in two successive specs.

I do not think this behavior should be supported, and laying out
the same struct twice should probably be an error. Doing it this
way requires at least a guard on the layout value and at most
repeated lookups of that layout via instance variables.
@headius
Copy link
Member Author

headius commented Jan 8, 2020

I have had to disable the caching of the Struct layout as part of this PR, but I believe this is due to a misfeature in the C FFI. I have filed an issue here: ffi/ffi#734

This reverts commit 21a8a8c.

Per ffi/ffi#734, the behavior that broke this caching is now
deprecated and will be removed in ffi 2.0.
@headius
Copy link
Member Author

headius commented Jan 10, 2020

I have had to disable the caching of the Struct layout

ffi/ffi#734 is fixed now (deprecated behavior to be removed in 2.0) so I reverted the removal of the caching logic.

@headius headius added this to the JRuby 9.3.0.0 milestone Feb 12, 2020
@headius
Copy link
Member Author

headius commented Feb 12, 2020

Retargeted to 9.3. Help wanted!

@ahorek
Copy link
Contributor

ahorek commented Feb 15, 2020

the remaining failure is

  Struct tests :enum field r/w
     Failure/Error: @orig_field.put(ptr, type.to_native(value, nil))

     NoMethodError:
       undefined method `put' for #<FFI::StructLayout::Number:0x71a509c>
       Did you mean?  puts
                      putc

this change 221b11d is in conflict with
https://github.com/ffi/ffi/blob/21dc3d48f672ecf334058b36f485292721de8a87/lib/ffi/struct_layout.rb#L92

should we fork the ruby version?

@headius
Copy link
Member Author

headius commented Feb 16, 2020

should we fork the ruby version?

Ideally, no, we should match the Ruby code exactly.

I'm not sure what you mean by a conflict. It seems that Field simply doesn't define put and get. I'm adding those now.

@ahorek
Copy link
Contributor

ahorek commented Feb 16, 2020

the previous java version used to delegate the put method to MappedFieldIO

public void put(ThreadContext context, Storage cache, Member m, AbstractMemory ptr, IRubyObject value) {

your fix looks good f40efb1 thanks

@headius
Copy link
Member Author

headius commented Feb 16, 2020

With these last changes from @ahorek and the missing Field put and get in place, it appears we are green!

@headius
Copy link
Member Author

headius commented Feb 16, 2020

the previous java version used to delegate the put method to MappedFieldIO

Ahh yes, but it was never bound in Ruby. All good now!

@headius headius merged commit b267aa1 into jruby:master Feb 18, 2020
@headius headius deleted the update_ffi branch February 18, 2020 22:56
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.

None yet

3 participants