[vim/vim] Fix Ruby 4.0 dynamic builds correctly by inlining rb_check_typeddata (PR #19060)
Yee Cheng Chin
Ruby 4.0 broke Vim compilation in dynamic builds. That's because the function rb_check_typeddata is now used in an inline function defined in Ruby headers, which causes it to link against the lib statically rather than using the one we load in dynamically
(dll_rb_check_typeddata) as we only remap it later (after the Ruby header include).
A previous fix (v9.1.2036) did a wrong fix by stubbing in the actual inline function rbimpl_check_typeddata instead. This does not work because the inline function is not part of the dynamic lib and therefore it's not possible to load it in as such. With that patch, Vim would crash when this function is used as the function pointer is null.
Fix this properly by reverting the previous change, and simply move the rb_check_typeddata dll remapping higher to where we already handle this situation when dealing with functions used by other inline functions.
Fix #18884
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19060
Commit Summary
- e7c2d87 Fix Ruby 4.0 dynamic builds correctly by inlining rb_check_typeddata
File Changes
(1 file)
- M src/if_ruby.c (12)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin pushed 1 commit.
- 32a8986 Fix Ruby 4.0 dynamic builds correctly by inlining rb_check_typeddata
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19060)thank!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19060)Did this v9.1.2039 or v9.1.2036 break ruby/dyn on Windows?
I am seeing a failure now with the daily appveyor vim-win32-installer builds: https://ci.appveyor.com/project/chrisbra/vim-win32-installer/builds/53344146/job/xdth9gi786qlm36i
Failures:
From test_ruby.vim:
Found errors in Test_ruby_Vim_buffer_count():
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_ruby_Vim_buffer_count line 3: Expected 3 but got 0
command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_ruby_Vim_buffer_count line 5: Expected 1 but got 0
Found errors in Test_ruby_Vim_buffer_get():
Caught exception in Test_ruby_Vim_buffer_get(): Vim(call):NoMethodError: undefined method `name' for nil:NilClass @ command line..script C:/projects/vim-win32-installer/vim/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_ruby_Vim_buffer_get, line 5
TEST FAILURE
NMAKE : fatal error U1077: 'if exist test.log ( echo TEST FAILURE & exit /b 1 ) else ( echo ALL DONE )' : return code '0x1'
Stop.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#19060)Let met take a look.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#19060)Is there a way for me to trigger the appveyor build? Or probably not since it's not a free thing?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19060)You could add a patch into the patch directory and create a PR. I think this should trigger a build over there
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19060)Okay I found it the culprit. It wasn't directly related to your patch. Including ruby.h (on 64-bit Windows) now leaks the HAVE_FSYNC macro. This causes BV_COUNT in if_ruby.c to be 96 instead of 95 in all other C files. Since buf_T contains an array of sctx_T sized by BV_COUNT, and each sctx_T is 16 bytes, the offset for every field after that array shifted by exactly 16 bytes.
This meant the Ruby module was looking at the wrong memory address when checking if a buffer was listed, which means the loop in buffer_c_count() and buffer_s_aref() returning zero/Qnil. It was a nightmare to debug because my 64bit Windows development machine just broke before Christmas and quite honestly all those if_*.c interfaces are black magic to me, so it took me a while to understand all the related code.
Don't ask me, why this triggered only after I included your patch 🤷
The fix should look like this:
diff --git a/src/if_ruby.c b/src/if_ruby.c index 60ca11188..66a7c469b 100644 --- a/src/if_ruby.c +++ b/src/if_ruby.c @@ -179,6 +179,9 @@ #ifdef HAVE_DUP # undef HAVE_DUP #endif +#ifdef HAVE_FSYNC +# undef HAVE_FSYNC +#endif // Avoid redefining TRUE/FALSE in vterm.h. #ifdef TRUE
I'll create a PR soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#19060)Right. Sorry I was a little busy last couple weeks and didn't get around to looking at this.
Windows does not have fsync, so I was a little confused why Ruby sets this flag. Seems like they just have a mock to substitute with _commit and pretend to have it available (http://github.com/ruby/ruby/commit/161e19ded98). It's kind of annoying that they expose all these HAVE_* ifdefs so we have to manually deal with the conflict. That said, if you look at the commit that added the mock to Ruby, it's decades old.
As for the trigger for this build failure, I'm guessing it's 4d5b303 (v9.1.2024). This change is the one that added a per-buffer fsync setting hence exposing this latent bug (that has been sitting for years) as now a mismatched HAVE_FSYNC flag will cause the build issue. This commit happens to coincide with Ruby 4's release. I wonder if the previous "passing" win32 installers were silently failing on Ruby? Maybe Ruby didn't work at all, which somehow caused the build to succeed by turning it off, and when I fixed Ruby it now tries to build and fail? That's the best guess I have.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19060)As for the trigger for this build failure, I'm guessing it's 4d5b303 (v9.1.2024).
Ah, so the failure started actually when importing that patch in the vim-win32-installer. The change to if_ruby was just a coincident and made me think this was the reason for the failure.
I think the worst part is each of them works a bit differently from each other. if_ruby is the only one where we do #include "vim.h" so far down.
Yes, annoyingly. I noticed this to. But moving it to the beginning will break the build again. Tried that and did not feel like fixing all the wrong defines for the ruby headers.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.