-
-
Notifications
You must be signed in to change notification settings - Fork 925
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 fixes #4518
Fiddle fixes #4518
Conversation
This is a re-merge on a separate branch due to Wnidows breakage caused by the original fixes.
This is a re-commit on a new branch because of Windows breakage caused by jruby#4511.
Before these fixes if I run gem list (with -d) I see some goofiness:
So something is not loading properly somewhere and this may or may not end up being related since it is hard to tell where we are really calling FFI vs calling Fiddle which is calling FFI. Here is the output with the patches applied:
Mostly this looks like a single repeated error. |
Hi @headius and @enebo. Thank you for taking the time to review my changes. I will begin immediately to resolve the issues on Windows. Following that, I will look into fixing the FFI/Fiddle function issue from #4510, along with as much else I can. I assume |
@jellymann Yes, it works without your changes, so something must have gone awry. @enebo above pointed out that there are some errors happening internally related to FFI, but those may not be related to the new failures that happen after the patch. I now also have a Windows VM I can use. Let us know if you need some assistance, and we will look forward to some additional patches! Logistically, you can either create a new PR to headius/jruby@fiddle_fixes, or just make a new PR against jruby/jruby@master containing the whole shebang. Thank you for your help! |
Previous commit broke this functionality by only allowing Fiddle::Pointer types.
This should be supported in future. Currently, attempting to convert IO to pointer causes a Segfault. Raising to avoid this for now.
@headius @enebo I have made a couple of PRs in to this branch to resolve a couple of things:
|
Accidentally left them in in last commit
Previously, `Fiddle::Pointer`s passed to `Fiddle::Function#call` were actually being silently converted to `FFI::Pointer`s because `FFI::Function#call` calls `#to_ptr`, which coincidentally (or not) returns the underlying `FFI::Pointer`. I now convert these pointers explicitely and remove the `#to_ptr` method.
@jellymann Looks great! I've merged your PRs in. @enebo You have a dev env set up for Windows, want to give the branch a quick check? |
I just got a build going myself and I'm seeing a new error when I attempt to
I'll see if I can find the problem. |
It looks like that error was just our Fiddle being a bit more strict about a missing closing parenthesis. However, fixing that (patch below) it still fails to bind this function from the wsock32 library. diff --git a/lib/ruby/stdlib/win32/resolv.rb b/lib/ruby/stdlib/win32/resolv.rb
index 183d5d38da..72a685a722 100644
--- a/lib/ruby/stdlib/win32/resolv.rb
+++ b/lib/ruby/stdlib/win32/resolv.rb
@@ -264,7 +264,7 @@ else
extend Importer
dlload "wsock32.dll"
end
- WsControl = WSock32.extern "int WsControl(int, int, void *, void *, void *, void *", :stdcall
+ WsControl = WSock32.extern "int WsControl(int, int, void *, void *, void *, void *)", :stdcall
WSAGetLastError = WSock32.extern "int WSAGetLastError(void)", :stdcall
MAX_TDI_ENTITIES = 512 Here's the errors with debugging turned on in RubyGems. Looks like there's still several other problems loading the needed functions on Windows.
|
Something else I believe is wrong here....WsControl is for windows 98 support using winsock. I suspect we should not be loading this at all... |
Ok I got this to work again but I know I reverted one of your (@jellyman) fixes: diff --git a/lib/ruby/stdlib/fiddle/jruby.rb b/lib/ruby/stdlib/fiddle/jruby.rb
index f69a6fc..d3d9ef6 100644
--- a/lib/ruby/stdlib/fiddle/jruby.rb
+++ b/lib/ruby/stdlib/fiddle/jruby.rb
@@ -97,12 +97,12 @@ module Fiddle
private
def make_pointer(arg, type)
- return arg if type != TYPE_VOIDP
+ return arg #if type != TYPE_VOIDP
Pointer[arg]
end
def make_native(ptr, type)
- return ptr if type != TYPE_VOIDP
+ return ptr #if type != TYPE_VOIDP
ptr.ffi_ptr
end
end So the issue is that if you run this: require 'win32/importer'
module Kernel32
extend Importer
dlload "kernel32"
end
getv = Kernel32.extern "int GetVersionExA(void *)", :stdcall
info = [ 148, 0, 0, 0, 0 ].pack('V5') + "\0" * 128
require 'pp'
pp info
getv.call(info)
pp info
pp info.unpack('V5')
#info.unpack('V5')[4] == 2 # VER_PLATFORM_WIN32_NT You will see that the param to GetVersionExA is an out parameter of void * but you make a new pointer so the original string loses the out data. |
@enebo I can't seem to see what the issue is with the above code. I ran it on my Windows machine and it runs exactly the same as it does on MRI.
|
@headius I noticed you've merged my PRs on your fork, but how come the changes aren't being reflected in this PR? Did I do something wrong? |
@headius Also, I cannot reproduce the failing I'm running Windows 7 SP1 64-bit. Perhaps if you are running a newer/different version of Windows they maybe removed the WsControl function? |
@jellymann This is confusing both @headius and I got the same error on his branch with win7 and win10 (respectively). It was going down the wrong branch because of the snippet I highlighted. The out param data was identical to the before puts (pp) so the comparison with 2 in that 5 element was seeing 0. My guess based on what I commented out is that ptr.ffi_ptr actually allocates another pointer or something like that? |
@enebo Can you re-test this on Windows? |
Fix: using strings as out arguments in Fiddle
I've merged headius#3 into my branch there. |
With the head of this branch I see:
|
I am not sure what is up. Should ffi/libc actually be loading? I do not think some of this stuff exists on windows. Can someone else try checking out head of headius fiddle-fixes and see what they see. This still sort of feels like it is going down the wrong path. |
@headius @jellymann ah I should point out in my output above I commented out sys_errlist in libc because it could not find it...once it was commented out it just could not find the next field 'sys_nerr'. So your output will be slightly different. |
@enebo Perhaps they were removed in a later version of windows. I only have Windows 7 available to me, and those functions exist. |
@jellymann wow I did just build a new Win 10 box so it is possible but I sure hope not. Also I am thinking MRI would not work either then (at least FFI). I will see if I can figure more out I guess. |
@enebo The reason I pulled in LibC is to get a handle on the I think MRI works because it doesn't load LibC like this at startup, since it uses it's own malloc/free functions that are linked at compile time. JRuby obviously can't do that since it's not C 😅 |
@jellymann do you think you could roll something which only binds those two functions? Also I don't grok why this works on win7 but not win10. Something seems strange there. Windows does occasionally remove stuff but I am surprised in that it all seems removed. |
While working on an FFI issue on MRI I stumbled across several of the issues that are addressed in this PR. I was just about to prepare a similar PR with a subset of these changes, but then noted this one. It would be great, if this PR could be finished! To the LibC issue: many of the functions are not available on Windows (unless on cygwin). However isn't it better to implement |
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed with Ruby-2.3+ and on JRuby (provided jruby/jruby#4518 will be merged).
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
@headius @enebo Hi All, sorry I haven't been active on this lately. I had to sell my PC a few months ago, but I have recently aquired a new one which is running Windows 10 this time. I pulled down my I'm also experiencing some new issues on my side, for example I cannot run the MRI specs unless I run them in Windows 7 compatibility mode, otherwise I get a exit code error In other news, I was able to successfully run some of the examples from Mittsu on JRuby without too much tweaking. There still seems to be a discrepancy between JRuby and MRI with regard to what can and cannot be implicitly converted to a pointer. JRuby/JFFI seem to only support Nil, String and MemoryIO, whereas MRI can convert integers and Fiddle::Closures as well. Not sure what else there is, but I will start investigating. Maybe we need to define more PointerParameterStrategy implementations for the various types? |
@jellymann PR sure! Exit code error: I have not seen that but we can work with you to look into it. Running mittsu: excellent! We have had few if any changes to fiddle since your earlier PRs but we definitely want to align things with MRI. Let's do it! cc @tenderlove since he wrote fiddle and may be able to offer some advice. |
This was causing `gem list` to fail. Apparently, there are quite a number of libc functions and variables that are not available on Windows.
Even though they're wrapped in begin/recue blocks (since they're also known to not be available in OSX), I'm avoiding them if Windows so that the extra debug noise doesn't show up. If not, why not?
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
Fiddle Fixes for Windows
Pinging this one after 7 years of neglect. 😭 I merged in headius#4 just now... sorry we lost track of this so many years ago. If you are still out there @danini-the-panini we'd love to revisit this changes in the fiddle gem, where we are working to import the JRuby logic: ruby/fiddle#147 |
@danini-the-panini That's great to hear! The FFI-based fiddle has been incorporated into the fiddle gem officially, and will provide fiddle functionality for both JRuby and TruffleRuby going forward! If you get some cycles to return to this work, definitely go to the source: https://github.com/ruby/fiddle. For now I will close this PR, since it should be rehomed on the fiddle repository anyway. Any fixes you can provide will be greatly appreciated! I will be standing by to help! |
In #4511, @jellymann provided some nice improvements to our "badly broken" implementation of Fiddle. Initially these changes looked ok, but then it turned out they broke logic on Windows that in turn caused basic things like
gem list
to fail.@enebo will comment with his findings, and we will work with @jellymann to get things in order.