Skip to content

Generate bindings using ctypes-stubs instead of ctypes-foreign (Minimize the use of libffi)#109

Open
pirbo wants to merge 8 commits into
dbuenzli:masterfrom
pirbo:pirbo-rewords
Open

Generate bindings using ctypes-stubs instead of ctypes-foreign (Minimize the use of libffi)#109
pirbo wants to merge 8 commits into
dbuenzli:masterfrom
pirbo:pirbo-rewords

Conversation

@pirbo

@pirbo pirbo commented Mar 3, 2025

Copy link
Copy Markdown
Contributor

Depends on #108

Pros:

  • (Independently from removing 1 definition to silence the warning from the C compiler that says this is a deprecated function that shouldn't be use (which we can very well reintroduce at the price of living with this warning)), tsdl.mli is not touched at all, so compatibility is fully preserved
  • Removes the overhead of libffi (but for 1 case, see below)
  • Contradict me if you disagree but I would claim that maintenance/extension is as easy as it was if not even a bit simplified because this binding methods does a bit more checks statically so we will be informed by the compiler about some categories of changes if they occurs on SDL side...
  • I struggled having the build system right but now that it is, I don't think it's too bad.

Meh:

  • There is so many cut/paste/reexport that we rely on the type-checkers to be sure nothing has been neither forgotten nor mis-exported. Hopefully, this method of binding is suppose to check more aspects statically...
  • It does not get fully rid of libffi. 1 single occurrence remains:

You cannot do without libffi (the ~thread_registration:true ~runtime_lock:true of

let as_callback =
  Foreign.funptr_opt ~thread_registration:true ~runtime_lock:true C.Types.as_callback_type

in audio_specs

Cons:

  • See below, I put as comments in order to quote them the definitions I'm unhappy with.

The direction taken by the discussion convinces me more and more to take the plunge but still, I repeat, this PR was open as a request for comments now that I've spent on these hours working on it...

For the one interested (because it was the first version of this PR description so I don't erase it but I now think that we don't care), here is my story for drowning into this mess:

  • I was experimenting with compiling this library using the "mingw" backend on windows (I know work has been done to compile it using the "msvc" backend and there exist a world where it works (which is no the vanilla opam-repository world (wherectypes is not compatible with msvc so no way to get tsdl)))
  • It was a complete failure (because symbols that gcc don't see any static use of are erased (while libffi will use them dynamically here) unless you explicitly put the no-as-needed flag which (as far as I understand) does not exist in mingw gcc/ld/...)
  • what seems to work with mingw is ctypes bindings built using "stubs" and well, actually, if doable, getting rid of libffi is a win for everybody independently of being on windows, isn't it? And migrating to this binding generation method is a matter of fighting the build system and a LOT of copy/paste, right?
  • let's give it a try...

Conclusion: See above I still don't have a successful build under mingw windows port... Not because of missing definitions this time but because of the ocaml compiler that hardwired the argument -municode when calling the C compiler while SDL would need -mwindows...

@dbuenzli

dbuenzli commented Mar 3, 2025

Copy link
Copy Markdown
Owner

Do you envision to actually switch one day?

Can you remind me exactly why we want to get rid of libffi ? I remember being mentioned that it's not supported on Windows but that's not what I read here.

Something that would drive me to switch would be users of the binding complaining that libffi is stealing to many cycles for their use. But then I wonder if case arising we could take an incremental approach to that.

  • I'm not sure about the build system at all. At the very least, it's very ugly but it might even be wrong. I just hammered until having the tests passing.

They look rather minimal. IIRC I suggested you could simply try to use dune. Would it work more seamlessly or would you run into same problems (the mingw things) ?

  • You don't get rid of libfii! There is one thing you cannot do without libffi (the ~thread_registration:true ~runtime_lock:true of
let as_callback =
  Foreign.funptr_opt ~thread_registration:true ~runtime_lock:true C.Types.as_callback_type

Well I suppose nothing prevents you from doing one manual (non-ctypes) binding on the side for that one no ?

@dbuenzli

dbuenzli commented Mar 3, 2025

Copy link
Copy Markdown
Owner

Now perhaps more heavy users than I am like @sanette or @zoggy have more opinions on that.

@sanette

sanette commented Mar 3, 2025

Copy link
Copy Markdown
Contributor

I'm afraid my experience with Windows is way too old to be of interest now, sorry.

Back in the times I was able to code some SDL games in Windows with vanilla ocaml+cygwin, but this was not with tsdl, but ocamlsdl (now https://github.com/fccm/OCamlSDL2)

@dbuenzli

dbuenzli commented Mar 3, 2025

Copy link
Copy Markdown
Owner

@sanette the opinion I'm seeking is more about having ctypes-generated bindings vs libffi baked ones.

@zoggy

zoggy commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

Preamble: I just don't care about Windows (or MacOS or any other proprietary OS), and I even prefer if it does not work on these OS :-)

Regarding my use of TSdl in Stk, I don't think the libffi performance is a problem (I did not know there was such a difference between libffi and ctypes stubs), so event if I would be interested in comparing both, I have no real opinion. As it seems like a quite big work, which could introduce bugs, I can help for testing if needed.

@sanette

sanette commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

I'm really not a specialist of this question. Theoretically, a library like SDL which focuses on performance would benefit from static ctypes stubs, so if this is feasible, that seems a good idea. However, this means also working on tsdl_ttf and tsdl_image. And the cost of maintenance is likely to be higher than what it is right now. Also, maintaining compat with ocaml 4 branch, which is necessary IMHO, might not be so obvious: I made some quick experiments to install https://github.com/pirbo/tsdl.git#pirbo-rewords, it works nicely with ocaml 5.3.0 but fails with 4.14.0:

[ERROR] The compilation of tsdl.1.1.0 failed at "ocaml pkg/pkg.ml build --dev-pkg true".

#=== ERROR while compiling tsdl.1.1.0 =========================================#
# context     2.2.1 | linux/x86_64 | ocaml-base-compiler.4.14.0 | pinned(git+https://github.com/pirbo/tsdl.git#pirbo-rewords#a1eed9aaf19499ef2de32523d7d2582b7ab92353)
# path        ~/.opam/4.14.0/.opam-switch/build/tsdl.1.1.0
# command     ~/.opam/opam-init/hooks/sandbox.sh build ocaml pkg/pkg.ml build --dev-pkg true
# exit-code   1
# env-file    ~/.opam/log/tsdl-34217-201d00.env
# output-file ~/.opam/log/tsdl-34217-201d00.out
### output ###
# /home/essai/.opam/4.14.0/lib/ocaml/caml/compatibility.h:71:63: error: ‘struct SDL_AudioSpec’ has no member named ‘caml_callback’; did you mean ‘callback’?
# [...]
# support/types_stubs_gen.c:22:47: note: in expansion of macro ‘callback’
#    22 |                offsetof(struct SDL_AudioSpec, callback));
#       |                                               ^~~~~~~~
# Command exited with code 2.
# pkg.ml: [ERROR] cmd ['ocamlbuild' '-use-ocamlfind' '-classic-display' '-j' '4' '-tag' 'debug'
#      '-build-dir' '_build' 'opam' 'pkg/META' 'CHANGES.md' 'LICENSE.md'
#      'README.md' 'src/tsdl.a' 'src/tsdl.cmxs' 'src/tsdl.cmxa' 'src/tsdl.cma'
#      'src/tsdl.cmx' 'src/tsdl.cmi' 'src/tsdl.mli' 'src/dlltsdl_stubs.so'
#      'src/libtsdl_stubs.a' 'src/top/tsdl_top.a' 'src/top/tsdl_top.cmxs'
#      'src/top/tsdl_top.cmxa' 'src/top/tsdl_top.cma' 'src/top/tsdl_top.cmx'
#      'src/tsdl_top_init.ml' 'doc/index.mld' 'test/min.ml' 'test/minc.c']: exited with 10

@pirbo

pirbo commented Mar 4, 2025

Copy link
Copy Markdown
Contributor Author
  • I will rewrite the description of the PR to say first what it is (which is indeed completely unrelated to Windows©) then the points of attention I think reviewer should watch before explaining in which context I've done it (Window 💲 ©)
  • Using dune as the build system (which I did during the development) suppose the diff Port build system to dune pirbo/tsdl#1 . Now that I've retrieved the ocamlbuild incantation, I don't have a strong opinion. (Well, if we are certain this build system is correct, I'm now wondering if tsdl.mllib is OK: shouldn't the generated modules Type_description Async_functions_description Functions_description and Types_generated appear explicitly here? I'm confused) In any case, it is orthogonal to the fact that it compiles and work using mingw.
  • libffi on windows (to the best of my understanding):
    • what works is dynamically "dlopen"ing a dll (and therefore having at runtime the machinery to find the dll on the disk) to then look for definition into it.
    • What we never manage to make work is (what ctypes-foreign rely on:) having the C compiler find statically at compile time the dll and links it to the binary once for all so that at runtime libffi "introspect the loaded executable" and finds definition into it...(sorry not rigorous terminology)

@pirbo

pirbo commented Mar 4, 2025

Copy link
Copy Markdown
Contributor Author

The OCaml 4.14.1 compatibility raised by @sanette is a big deal indeed!

In OPAM_ROOT/4.14.1/lib/ocaml/caml/compatibility.h, there is

   71 | #define callback CAML_DEPRECATED("callback", "caml_callback") caml_callback

So the preprocessor "happily" rewrites the mention of the field of SDL_AudioSpec named callback into caml_callback 🤦 That's an interresting one...

@sanette

sanette commented Mar 4, 2025

Copy link
Copy Markdown
Contributor
  • shouldn't the generated modules Type_description Async_functions_description Functions_description and Types_generated appear explicitly here?

I think so, yes, because if I try to compile bogue I get:

[ERROR] The compilation of bogue.20250224 failed at "dune build -p bogue -j 3 @install".

#=== ERROR while compiling bogue.20250224 =====================================#
# context     2.2.1 | linux/x86_64 | ocaml-base-compiler.5.3.0 | https://opam.ocaml.org#c940a5f1a97ea0abf72b5f8a3319f15551c41337
# path        ~/.opam/5.3.0/.opam-switch/build/bogue.20250224
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p bogue -j 3 @install
# exit-code   1
# env-file    ~/.opam/log/bogue-71484-961171.env
# output-file ~/.opam/log/bogue-71484-961171.out
### output ###
# (cd _build/default && /home/essai/.opam/5.3.0/bin/ocamlopt.opt -w -40 -g -o examples/examples.exe /home/essai/.opam/5.3.0/lib/xdg/xdg.cmxa -I /home/essai/.opam/5.3.0/lib/xdg /home/essai/.opam/5.3.0/lib/ocaml/str/str.cmxa /home/essai/.opam/5.3.0/lib/stdlib-shims/stdlib_shims.cmxa /home/essai/.opam/5.3.0/lib/integers/integers.cmxa -I /home/essai/.opam/5.3.0/lib/integers /home/essai/.opam/5.3.0/[...]
# File "_none_", line 1:
# Error: No implementation provided for the following modules:
#          "Async_functions_generated" referenced from "Tsdl" (/home/essai/.opam/5.3.0/lib/tsdl/tsdl.cmxa)
#          "Types_generated" referenced from "Tsdl" (/home/essai/.opam/5.3.0/lib/tsdl/tsdl.cmxa)
#          "Async_function_description" referenced from "Tsdl" (/home/essai/.opam/5.3.0/lib/tsdl/tsdl.cmxa)
#          "Functions_generated" referenced from "Tsdl" (/home/essai/.opam/5.3.0/lib/tsdl/tsdl.cmxa)
#          "Function_description" referenced from "Tsdl" (/home/essai/.opam/5.3.0/lib/tsdl/tsdl.cmxa)

@pirbo pirbo force-pushed the pirbo-rewords branch 3 times, most recently from 999d648 to 2f1ab70 Compare March 5, 2025 08:53
@pirbo

pirbo commented Mar 5, 2025

Copy link
Copy Markdown
Contributor Author
  • shouldn't the generated modules Type_description Async_functions_description Functions_description and Types_generated appear explicitly here?

I think so, yes, because if I try to compile bogue I get:
[...]

Should be better now. At least, works for me :) (And it is actually less changes in the build system so this is good!)

Comment on lines +1200 to +1206
type _button_bind
let button_bind : _button_bind structure typ =
structure "SDL_GameControllerButtonBind"
let button_bind_bind_type = field button_bind "bindType" int
let button_bind_value1 = field button_bind "value1" int (* simplified enum *)
let button_bind_value2 = field button_bind "value2" int
let () = seal button_bind

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This struct is defined here and not in Type_description because it contains an anonymous struct as a field (https://wiki.libsdl.org/SDL2/SDL_GameControllerButtonBind) and ctypes-stubs does not handle that...

Comment on lines +25 to +61
let framebuffer_acceleration =
F.foreign_value "SDL_HINT_FRAMEBUFFER_ACCELERATION" (array 29 char)
let idle_timer_disabled =
F.foreign_value "SDL_HINT_IDLE_TIMER_DISABLED" (array 28 char)
let mouse_focus_clickthrough =
F.foreign_value "SDL_HINT_MOUSE_FOCUS_CLICKTHROUGH" (array 29 char)
let orientations =
F.foreign_value "SDL_HINT_ORIENTATIONS" (array 21 char)
let render_driver =
F.foreign_value "SDL_HINT_RENDER_DRIVER" (array 18 char)
let render_opengl_shaders =
F.foreign_value "SDL_HINT_RENDER_OPENGL_SHADERS" (array 26 char)
let render_logical_size_mode =
F.foreign_value "SDL_HINT_RENDER_LOGICAL_SIZE_MODE" (array 29 char)
let render_scale_quality =
F.foreign_value "SDL_HINT_RENDER_SCALE_QUALITY" (array 25 char)
let render_vsync =
F.foreign_value "SDL_HINT_RENDER_VSYNC" (array 17 char)

let no_signal_handlers =
F.foreign_value "SDL_HINT_NO_SIGNAL_HANDLERS" (array 23 char)
let thread_stack_size =
F.foreign_value "SDL_HINT_THREAD_STACK_SIZE" (array 22 char)
let window_frame_usable_while_cursor_hidden =
F.foreign_value
"SDL_HINT_WINDOW_FRAME_USABLE_WHILE_CURSOR_HIDDEN" (array 44 char)

let audio_resampling_mode =
F.foreign_value "SDL_HINT_AUDIO_RESAMPLING_MODE" (array 26 char)
let mouse_normal_speed_scale =
F.foreign_value "SDL_HINT_MOUSE_NORMAL_SPEED_SCALE" (array 29 char)
let mouse_relative_speed_scale =
F.foreign_value "SDL_HINT_MOUSE_RELATIVE_SPEED_SCALE" (array 31 char)
let touch_mouse_events =
F.foreign_value "SDL_HINT_TOUCH_MOUSE_EVENTS" (array 23 char)
let mouse_touch_events =
F.foreign_value "SDL_HINT_MOUSE_TOUCH_EVENTS" (array 23 char)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm very unhappy with hardwiring the length of the strings here but I cannot find an other to bind them...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why dont you define a wrapper fn for F.foreign_value?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: I also believe that some of your lengths are not correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay but I'm very interested by understanding better this because, as I said, I don't know exactly what I'm doing and should do here. What do you mean? How have you detected that?/What raised suspicions of that? (On which platform/SDL version are you? (I really hope this is not a question of having these definitions move... I'm on debian testing with SDL 2.32.2).

On my side, if I try to put a different length to any of these definitions. For example

diff --git a/src/function_description.ml b/src/function_description.ml
index 09dcff1..a142045 100644
--- a/src/function_description.ml
+++ b/src/function_description.ml
@@ -39,7 +39,7 @@ module Functions (F : FOREIGN) = struct
     let render_scale_quality =
       F.foreign_value "SDL_HINT_RENDER_SCALE_QUALITY" (array 25 char)
     let render_vsync =
-      F.foreign_value "SDL_HINT_RENDER_VSYNC" (array 17 char)
+      F.foreign_value "SDL_HINT_RENDER_VSYNC" (array 42 char)
 
     let no_signal_handlers =
       F.foreign_value "SDL_HINT_NO_SIGNAL_HANDLERS" (array 23 char)

Immediately, the compilation fails with

src/functions_stubs.c: In function ‘sdl2_stubs_17_SDL_HINT_RENDER_VSYNC’:
src/functions_stubs.c:93:22: error: initialization of ‘char (*)[42]’ from incompatible pointer type ‘char (*)[17]’ [-Wincompatible-pointer-types]
   93 |    char(* x38)[42] = &SDL_HINT_RENDER_VSYNC;
      |                      ^

That's what convinced me I was kind of """safe"""...

Comment thread src/function_description.ml Outdated
Comment thread src/function_description.ml Outdated
Comment thread src/type_description.ml
Comment on lines +378 to +385
module Vulkan = struct
type _surface
type surface = _surface Ctypes_static.structure Ctypes_static.ptr

let raw_surface : _surface Ctypes_static.structure F.typ =
F.structure "VkSurfaceKHR_T"
let surface : surface F.typ = F.ptr raw_surface
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Require <SDL_Vulkan.h> which makes me wonder if it is an independent .h because it's not always available and therefore should be optionally bound (how?).
did not complain that it was missing under mingw windows though... Maybe worth trying under MacOS/with msvc...

@pirbo pirbo marked this pull request as ready for review March 5, 2025 10:15
@pirbo

pirbo commented Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

The OCaml 4.14.1 compatibility raised by @sanette is a big deal indeed!

Fixed

Comment thread src/type_description.ml
Comment thread src/type_description.ml Outdated
Comment thread src/type_description.ml Outdated
@sanette

sanette commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

great! I made some test with boguex 50 which makes thousands of calls to SDL functions per frame, and while it used to draw about 140-170 circles per frame, now I get about 230 circles per frame!

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.

4 participants