Skip to content

add integer pointer memory strategy #8423

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

Closed

Conversation

danini-the-panini
Copy link
Contributor

Allows implicitly passing integers into FFI functions that take pointers, using the passed in integer as the address.

This mimics the behavior of CRuby.

@danini-the-panini
Copy link
Contributor Author

@headius I've added a naive integer pointer parameter strategy, but it's causing a segfault. I thought simply wrapping the integer using NativeMemoryIO would work, but apparently not. Any ideas?

On a promising note, it seems to allow some of mittsu's examples to run, although most of them do segfault.
image

@headius
Copy link
Member

headius commented Nov 20, 2024

Hmm I would have expected that to work ok. Can you point me toward a segfault report or show me how to generate the same error?

@danini-the-panini
Copy link
Contributor Author

@headius You can trigger a segfault by just running the ffi specs. I don't even know which spec is actually failing since it crashes the whole VM. Also I haven't figured out how to debug JNI yet, any tips?

In the meantime, I've managed a workaround in Fiddle's FFI backend: ruby/fiddle#162

@headius
Copy link
Member

headius commented Dec 3, 2024

@danini-the-panini Looking into this today. If you can stop by our Matrix channel I'll be in the office.

@headius
Copy link
Member

headius commented Dec 3, 2024

Deleted previous repro and stack because it was reflecting known failures on macOS with function callbacks (see #6995).

I dug out the first failure in the FFI suite, and I think this is a failure due to integers now looking like they could be used as a valid pointer.

New repro and stack:

require 'ffi'
require 'delegate'

module PointerTestLib
  extend FFI::Library
  ffi_lib "spec/ffi/fixtures/libtest.so"
  attach_function :ptr_ret_int32_t, [ :pointer, :int ], :int
end

PointerTestLib.ptr_ret_int32_t(0, 0)

This is from the "Fixnum cannot be used as a Pointer argument" spec in spec/ffi/pointer_spec.rb. It properly raised the error before because Integers (Fixnum is the old name for 64-bit range Integers) could not be converted to a pointer. Now that you added such a strategy, it goes ahead and tries to pointerify the zero value in the first argument to ptr_ret_int32, and that naturally blows up when accessed.

The JVM trace is below:

Current thread (0x000077d518019b10):  JavaThread "main"             [_thread_in_native, id=688093, stack(0x000077d51ce00000,0x000077d51d000000) (2048K)]

Stack: [0x000077d51ce00000,0x000077d51d000000],  sp=0x000077d51cffd7f8,  free space=2037k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C  [libtest.so+0x2c877]  ptr_ret_int32_t+0x7
Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
J 4286  com.kenai.jffi.Foreign.invokeArrayReturnInt(JJ[B)I org.jruby.dist (0 bytes) @ 0x000077d5084a0110 [0x000077d5084a00a0+0x0000000000000070]
j  com.kenai.jffi.Invoker.invokeInt(Lcom/kenai/jffi/CallContext;JLcom/kenai/jffi/HeapInvocationBuffer;)I+38 org.jruby.dist
j  com.kenai.jffi.Invoker.invokeInt(Lcom/kenai/jffi/Function;Lcom/kenai/jffi/HeapInvocationBuffer;)I+10 org.jruby.dist
j  org.jruby.ext.ffi.jffi.DefaultMethodFactory$Signed32Invoker.invoke(Lorg/jruby/runtime/ThreadContext;Lcom/kenai/jffi/Function;Lcom/kenai/jffi/HeapInvocationBuffer;)Lorg/jruby/runtime/builtin/IRubyObject;+6 org.jruby.dist
j  org.jruby.ext.ffi.jffi.BufferNativeInvoker.call(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/RubyModule;Ljava/lang/String;[Lorg/jruby/runtime/builtin/IRubyObject;)Lorg/jruby/runtime/builtin/IRubyObject;+160 org.jruby.dist
j  org.jruby.ext.ffi.jffi.NativeInvoker.call(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/RubyModule;Ljava/lang/String;[Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/Block;)Lorg/jruby/runtime/builtin/IRubyObject;+36 org.jruby.dist
j  org.jruby.ext.ffi.jffi.DefaultMethod.call(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/RubyModule;Ljava/lang/String;[Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/Block;)Lorg/jruby/runtime/builtin/IRubyObject;+13 org.jruby.dist
j  org.jruby.internal.runtime.methods.DynamicMethod.call(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/RubyModule;Ljava/lang/String;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/Block;)Lorg/jruby/runtime/builtin/IRubyObject;+22 org.jruby.dist
J 5082 c1 org.jruby.internal.runtime.methods.DynamicMethod.call(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/RubyModule;Ljava/lang/String;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;)Lorg/jruby/runtime/builtin/IRubyObject; org.jruby.dist (17 bytes) @ 0x000077d50116b55c [0x000077d50116b440+0x000000000000011c]
j  org.jruby.runtime.callsite.CachingCallSite.cacheAndCall(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/RubyClass;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;)Lorg/jruby/runtime/builtin/IRubyObject;+31 org.jruby.dist
J 3806 c1 org.jruby.runtime.callsite.CachingCallSite.call(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;)Lorg/jruby/runtime/builtin/IRubyObject; org.jruby.dist (60 bytes) @ 0x000077d500eefc34 [0x000077d500eef800+0x0000000000000434]
j  blah.invokeOther5:ptr_ret_int32_t(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/builtin/IRubyObject;)Lorg/jruby/runtime/builtin/IRubyObject;+11
j  blah.RUBY$script(Lorg/jruby/runtime/ThreadContext;Lorg/jruby/parser/StaticScope;Lorg/jruby/runtime/builtin/IRubyObject;[Lorg/jruby/runtime/builtin/IRubyObject;Lorg/jruby/runtime/Block;Lorg/jruby/RubyModule;Ljava/lang/String;)Lorg/jruby/runtime/builtin/IRubyObject;+139

@danini-the-panini Everything seems like it's actually working correctly! The problem is that now it accepts integer values for pointer arguments, but most places you pass a pointer it should apparently NOT accept an integer (according to FFI specs).

@headius
Copy link
Member

headius commented Dec 3, 2024

@danini-the-panini Can you show me what this change fixed in your codebase? I'm wondering if it's perhaps a feature that Fiddle supports but FFI is supposed to error, so this spec is right for FFI but prevents some functionality Fiddle needs.

@danini-the-panini
Copy link
Contributor Author

@headius ok that makes sense, I had a sneaky suspicion this wasn't going to be the correct fix. I had tried something similar by just adding Integer#to_ptr and it broke everything.

Fiddle does seem to allow integers as pointers in CRuby, and that is used extensively in mittsu-opengl since OpenGL has a few methods that have VOIDP arguments that are actually just used as byte offsets e.g. glVertexAttribPointer

Here is an example use in Mittsu: https://github.com/danini-the-panini/mittsu-opengl/blob/a69434802b183577a6f0f770ca17de9d9a48d37f/lib/mittsu/opengl/geometry_like.rb#L43

In light of FFI's spec, I'll close this and finish up the fix in Fiddle's ffi_backend

@danini-the-panini
Copy link
Contributor Author

Interestingly, I managed yesterday to run all of Mittsu's examples using this patch instead of the ffi_backend fix, and I didn't get any segfaults. This is on Apple Silicon (M2 Pro) with callbacks 🤔

@headius
Copy link
Member

headius commented Dec 4, 2024

@danini-the-panini I don't think anything is wrong with the patch other than suddenly allowing integers to be passed for pointer arguments. That may be a valid feature to add, or it may lead to confusion if someone passes an integer they don't expect to become a pointer. Perhaps open an issue with ffi/ffi and we can discuss there with other stakeholders?

@danini-the-panini
Copy link
Contributor Author

@headius Fair enough. It appears this was a feature that was removed for "JRuby compat" ffi/ffi@59b48a9

@danini-the-panini danini-the-panini force-pushed the ds-ffi-fixes branch 2 times, most recently from cc383e4 to 628a83e Compare December 4, 2024 21:42
@danini-the-panini
Copy link
Contributor Author

Closing this since ffi/ffi#1130 was closed

tenderlove pushed a commit to ruby/fiddle that referenced this pull request Dec 11, 2024
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)

---------

Co-authored-by: Benoit Daloze <[email protected]>
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 <[email protected]>
@enebo enebo added this to the Invalid or Duplicate milestone Jan 20, 2025
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.

3 participants