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

Fiddle not working for small demo application #5786

Open
chrisseaton opened this issue Jul 8, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@chrisseaton
Copy link
Contributor

commented Jul 8, 2019

Environment

Provide at least:

  • JRuby version (jruby -v) and command line (flags, JRUBY_OPTS, etc) jruby 9.2.7.0 (2.5.3) 2019-04-09 8a269e3 Java HotSpot(TM) 64-Bit Server VM 25.201-b09 on 1.8.0_201-b09 +jit [darwin-x86_64]
  • Operating system and platform (e.g. uname -a) Darwin Chriss-MacBook-Pro.local 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64

Expected Behavior

Fiddle works for a small demo application.

brew install sdl2
git clone git@github.com:marcinruszkiewicz/doomfire.git
cd doomfire
bundle install
bundle exec ruby [-J-XstartOnFirstThread] exe/doomfire --sdl

Actual Behavior

NoMethodError: undefined method `malloc' for Fiddle:Module
          malloc at /Users/chrisseaton/.rbenv/versions/jruby-9.2.7.0/lib/ruby/stdlib/fiddle/struct.rb:70
  prepare_output at /Users/chrisseaton/Documents/doomfire/lib/doomfire/sdl.rb:110
      initialize at /Users/chrisseaton/Documents/doomfire/lib/doomfire/base.rb:51
          <main> at exe/doomfire:28

Seems like malloc of a struct doesn't work?

You can work around that with this:

diff --git a/lib/doomfire/sdl.rb b/lib/doomfire/sdl.rb
index 7d48788..d9dd6a9 100644
--- a/lib/doomfire/sdl.rb
+++ b/lib/doomfire/sdl.rb
@@ -57,8 +57,8 @@ module Doomfire
       loop do
         next if @event.nil? || @window.nil? || @renderer.nil? || @texture.nil?
 
-        FFI_SDL.SDL_PollEvent(@event)
-        @exit_requested = true if @event.type == FFI_SDL::SDL_QUIT
+        FFI_SDL.SDL_PollEvent(Fiddle::NULL) # @event
+        # @exit_requested = true if @event.type == FFI_SDL::SDL_QUIT
 
         if @exit_requested
           stop_fire if @counter.zero?
@@ -107,7 +107,7 @@ module Doomfire
         @fire_width,
         @fire_height
       )
-      @event = FFI_SDL::SDL_Event.malloc
+      # @event = FFI_SDL::SDL_Event.malloc
     end
 
     def clear

If you do that you'll not get the window appearing, so something else is wrong somewhere beyond the first problem.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Seems like malloc is just missing, possibly added some time after we last "fiddled" with this library.

Interesting that nobody has reported it. How did you come across it? I assume you're not doing SDL development.

@chrisseaton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

I was looking at Fiddle for implementing it for TruffleRuby (we've ported the C extension part to Ruby using a couple of primitives.)

But the repository in the reproduction instructions is someone else's and is existing code I haven't modified https://github.com/marcinruszkiewicz/doomfire, although I've now added a PR to add benchmarks.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Given that you have support for FFI, it might makes sense for us to share a single FFI-based Fiddle.

FWIW malloc should be trivial to add to our version.

@chrisseaton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Yes I'm in favour of an FFI-based Fiddle. Some people in our team think that could be more work than using our primitives and could add indirection, but what I'll do when it's working with the primitives is see what the diff is like to switch to FFI - the primitives are very simple so should be easy. Then I'll report back to you if that works. Maybe MRI would be willing to move Fiddle to an external gem and have it depend on FFI normally.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Fiddle was originally introduced because for a few years, the native FFI gem was LGPL, so it couldn't be a hard dependency in CRuby. Instead, they implemented their own tiny FFI in Fiddle.

The move to LGPL was temporary and I switched it back to BSD, but by then the decision had been made. It's pretty unlikely that they'll switch to a dependency on FFI now, and at least @tenderlove has told me he'd be willing to support FFI atop Fiddle, but definitely not support the reverse direction or making FFI the core dependency. We're on our own.

An alternative direction that would solve this issue and benefit the community would be to rework the FFI gem to use Fiddle, and then we other implementations just need to support Fiddle. However that requires significantly more effort than making the pure-Ruby Fiddle work properly, and since FFI is by far the most widely used foreign-function interface, I don't see that ever happening.

@enebo

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

If I remember right FFI had some issues (32/64?) that @tenderlove took issue with as well. I don't recall but I think there was a technical reason as well.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@enebo Yah there were a few other technical issues like 32/64 and support for everything DL needs, but those were technical issues we could have fixed. Of course, we could have fixed the license (and did so later) but that issue was not brought to our attention at the time.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I made a quick attempt to patch this, but there's further complications since FFI really wants to return an FFI::Pointer object for pointers. At first I simply bound malloc to return :pointer, but that produces FFI::Pointer that some of the other Fiddle code doesn't know how to handle. A modified version that returns :long gets further, but eventually the actual SDL function bindings start returning FFI::Pointer and it falls apart again.

So generally speaking, our Fiddle-on-FFI works well enough for simple things, but theres some difficult mismatches to fix. I hope we can work together to make a reliable FFI-based Fiddle that all impls could share rather than building yet another impl-specific FFI binding.

headius added a commit to headius/jruby that referenced this issue Jul 10, 2019

Restore LibC module into Fiddle namespace and add malloc.
The LibC module was previously used only by Fiddle::Pointer and
was removed during a stdlib update. This commit restores it, moves
it under the Fiddle::JRuby namespace, and adds the missing
Fiddle.malloc to fix jruby#5786.

@headius headius referenced a pull request that will close this issue Jul 10, 2019

Open

Restore LibC module into Fiddle namespace and add malloc. #5787

@headius

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I opened #5787 to add the missing malloc, using a "direct" binding that doesn't produce an FFI::Pointer. However, due to reasons I spelled out above and in the PR, this isn't sufficient as a fix for this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.