Crystal: Lib callbacks work incorrectly when structs are involved

[oprypin]

name: test
version: 0.1.0
dependencies:
  crsfml:
    github: blaxpirit/crsfml
    version: "< 2.4"

require "crsfml/system_lib"
require "crsfml/graphics_lib"

shape = CSFML.shape_create(
  ->(data: Void*) { 3u64 },
  ->(i: UInt64, data: Void*) { CSFML::Vector2f.new(x: 13f32, y: 37f32) },
  Pointer(Void).null
)
CSFML.shape_update(shape)
p CSFML.shape_get_point(shape, 1)
#< CSFML::Vector2f(@x=13, @y=0)
# (expected 13, 37)

[asterite]

Ouch. That probably is because we are not respecting the X86_64 ABI in Proc calls. Which probably means we should apply that ABI logic everywhere (right now we are doing that only in lib calls).

[oprypin]

I got bitten by this again :frowning:

Looks like structs mess up not only the callback return, but also arguments passed to callback.

require "crsfml/system_lib"
require "crsfml/audio_lib"

ss = CSFML.sound_stream_create(
  ->(chunk: CSFML::SoundStreamChunk*, userdata: Void*) {
    0
  },
  ->(time: CSFML::Time, userdata: Void*) {
    puts "called with #{userdata}"
  },
  1, 44100,
  Pointer(Void).new(0x123)
)
CSFML.sound_stream_play(ss)
#< called with Pointer(Void).null
# (expected Pointer(Void)@0x123)

Here the first callback receives userdata correctly, because it has no structs in its arguments.
But the 2nd one receives 0 as userdata, because it receives a struct (with one member)

[asterite]

@BlaXpirit I'll try to fix this soon. These bugs are the worst, because they are silent.

[oprypin]

@asterite Any chance for a fix in the near future?

[asterite]

A simple way to reproduce this:

# foo.cr
@[Link(ldflags: "#{__DIR__}/mylib.o")]
lib LibMylib
  struct Struct
    x : Int32
    y : Int32
  end

  alias Callback = Struct -> Struct

  fun foo(callback : Callback) : LibMylib::Struct
end

x = LibMylib.foo(->(s) {
  p s
  s.x, s.y = s.y, s.x
  s
})
p x

# mylib.c
struct Struct {
  int x;
  int y;
};

struct Struct foo(struct Struct (*callback)(struct Struct)) {
  struct Struct s;
  s.x = 1;
  s.y = 2;
  return callback(s);
}

Then:

$ clang -c -o mylib.o mylib.c
$ crystal foo.cr 
LibMylib::Struct(@x=70884144, @y=1)
LibMylib::Struct(@x=1, @y=0)

I'm thinking maybe this can be solved by creating a wrapper fun for lib callbacks that would adjust the arguments and return values. I'll try...

[oprypin]

I don't see why any kind of wrapper is necessary.

[asterite]

C ABI expects structs to be passed and returned in some annoying ways. For example if a struct is big (more than 8 bytes, I think), then it needs to be returned in a pointer, and you need to pass that pointer as the first argument in the function. If a struct is small, it must be extended to a word size, when passed as an argument (or something like that).

Crystal code doesn't respect this ABI, because Crystal code interacts with other code written in Crystal, so there's no ABI to follow. However, it needs to respect this ABI when interfacing with C.

When we codegen lib function calls, at that moment, we generate code specific for the C ABI.

(The truth is, we found out about this C ABI, and how LLVM doesn't automatically generate correct C ABI code, after a long time, when we had a lot of code written without an ABI in mind. We though LLLVM would abstract that from us, but unfortunately this logic has to be repeated in every compiler... For example Rust does this too)

However, a Crystal callback (a Proc) doesn't know if it's going to be passed to C or used in Crystal code. One solution is to generate all Procs respecting C's ABI. But because you can take a pointer to any method, it means that we would need to respect the C ABI everywhere, which is a huge change to do right now. Alternatively, I'm thinking maybe a wrapper can be passed, but I'm not sure that can work.

[oprypin]

OK, thank you for information. Please don't focus your attention here if this is such a big task.
I can work around this.

[asterite]

Why it's not a blocker? How can you workaround it?

[oprypin]

Now I'm making my own C++ → C wrapper in addition to a C → Crystal wrapper (this bug was not the main reason for this), so I can define the functions however I want, unwrapping structs into separate arguments; also instead of returns I can pass pointers to be filled.
So now it's just a mild inconvenience.

[asterite]

@BlaXpirit More fixes! Could you try this again? I tried this on the project you gave me and uncommenting the code, plus running all specs (even with the pending ones) work.

[oprypin]

@asterite I think that's all I have

[asterite]

@BlaXpirit You mean that it worked?

[oprypin]

@asterite, yes, the specs worked, just like for you, and I don't have any other tests to try it with.

Thank you very much!