Syscall Sysv: Add support for strings in callback arguments

[jwijenbergh]

I am building bindings to a library that has callbacks with strings as arguments, therefore I added this in.

The test is very basic, so that might be improved, any ideas? Also callback_test.go is only run for Darwin.

I am still learning a bit about this code-base so any feedback would be helpful :^)

[TotallyGamerJet]

Strings can already be used in callbacks by using a *byte. The reason I didn't want to automatically convert *byte into a string here is because that hides the allocation. Sometimes the user doesn't need the C string copied into Go memory because it's static memory. See ObjC for examples. This is why I suggested #121 because it explicitly copies.

@hajimehoshi what do you think?

[jwijenbergh]

Strings can already be used in callbacks by using a *byte

This is indeed what I was doing

The reason I didn't want to automatically convert *byte into a string here is because that hides the allocation

Ah!

This is why I suggested #121 because it explicitly copies.

Right.

Makes sense if you want to close this

[hajimehoshi]

Thank you for the PR.

Yeah, I agree with @TotallyGamerJet . Implicit conversion sometimes causes troubles and we like explicit ways.

Let me close this.

[jwijenbergh]

Thanks for the reviews!

[jwijenbergh]

Just a question (sorry). Isn't this consistent with:

purego/func.go

Lines 90 to 102 in 6319229

	// All functions below call this C function:
	//
	// char *foo(char *str);
	//
	// // Let purego convert types
	// var foo func(s string) string
	// goString := foo("copied")
	// // Go will garbage collect this string
	//
	// // Manually, handle allocations
	// var foo2 func(b string) *byte
	// mustFree := foo2("not copied\x00")
	// defer free(mustFree)

For return val purego copies if the type is string, to get rid of that you use a *byte. My intuition was if we use this same convention for callbacks args then we're consistent. Or should registerfunc have the string copying for return value removed and only handle *byte as well (if a solution to #121 is merged)?

[hajimehoshi]

Good point.

In RegisterFunc, converting C's char* to Go's string can happn at return values and Go's string to C's char* at arguments. On the other hand, in NewCallback, converting C's char* to Go's string could happen at arguments and Go's string to C's char* at return values. So this is a little different. This requires another discussion.

[hajimehoshi]

@TotallyGamerJet What do you think? I have not found an issue to support this conversion for callbacks so far.

As we need to be careful about conversion and object lifetimes, I slightly tend not to support this, though I understand that consistency matters.

IMO, as C <-> Go string conversion is complicated, not supporting this even for RegisterFunc might be an option. This would be consistent in another way.

[jwijenbergh]

As we need to be careful about conversion and object lifetimes, I slightly tend not to support this, though I understand that consistency matters.

I also don't have a strong opinion on this BTW just putting it out there so not supporting this is completely fine with me. It's one more thing to maintain in purego. Maybe it's fine that purego is as small as possible regarding this

IMO, as C <-> Go string conversion is complicated, not supporting this even for RegisterFunc might be an option. This would be consistent in another way.

I think this would definitely make it more consistent, that does give some breakage with packages that now rely on it. But semver solves that partly

[TotallyGamerJet]

I agree that consistency is key. If I remember correctly the reason RegisterFunc got the automatic conversion is be a use it's super convenient since you mostly just want to convert a char* to a string in most cases. However, I wouldn't be opposed dropping that feature if an API was created to replace it

[hajimehoshi]

If I remember correctly the reason RegisterFunc got the automatic conversion is be a use it's super convenient since you mostly just want to convert a char* to a string in most cases.

I don't remember the background :) This was alredy introduced at the first commit to introduce RegisterFunc?: 7128aea

I agree this is super convenient, but I'm a little bit worried about the complexitiy e.g. if Go's string ends with \0, this can be converted to a cloned C string, that lives for the call.

And, I now realized this Go string -> C string conversion cannot be applied to a callback. In callback, wouldn't it be possible to return a Go string as a C string with the same rule? What would its lifetime be?

So we have to decide the specification if we want to apply a similar conversion to callbacks. At least, we cannot use the same rule, so we cannot make it consistent in terms of that unfortunately in any cases.

[TotallyGamerJet]

I agree this is super convenient, but I'm a little bit worried about the complexitiy e.g. if Go's string ends with \0, this can be converted to a cloned C string, that lives for the call.

In RegisterFunc if the Go string ends in a null terminated byte then it is not copied and just passed as is. Only if it doesn't is it copied into Go memory and kept alive for the call but the API makes no guarantee when it will be freed. This behavior is documented in #154

And, I now realized this Go string -> C string conversion cannot be applied to a callback. In callback, wouldn't it be possible to return a Go string as a C string with the same rule? What would its lifetime be?

True it becomes very unclear

So we have to decide the specification if we want to apply a similar conversion to callbacks. At least, we cannot use the same rule, so we cannot make it consistent in terms of that unfortunately in any cases.

Should we make an issue about this instead of commenting in this PR? Or just reuse #121?

[hajimehoshi]

Let's reuse #121