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

Add Ctypes.string_start. #143

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@whitequark
Contributor

whitequark commented Apr 12, 2014

Ctypes.string_start works like Ctypes.bigarray_start--it returns
a pointer to the start of the string. Existence of this function
is motivated by the C APIs which mutate strings passed to them,
but do not capture the pointers.

In particular, I've worked on wrappers for such libraries as libsodium and libzmq, and they both would benefit from this API.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 12, 2014

Member

Thanks for the patch. This would be very useful functionality, but it's difficult to implement safely.

As your docstring for string_start suggests, the difficulty arises from the fact that strings are under control of the OCaml runtime, which can move them about unexpectedly. Unfortunately, even allocating a value to hold the pointer is enough to trigger a garbage collection, invalidating the pointer before it's even used.

# for i = 1 to max_int do
    if Ctypes.(!@ (string_start (String.copy "foo"))) <> 'f'
    then failwith (Printf.sprintf "Failed on iteration %d" i)
  done;;
      Exception: Failure "Failed on iteration 32762".
Member

yallop commented Apr 12, 2014

Thanks for the patch. This would be very useful functionality, but it's difficult to implement safely.

As your docstring for string_start suggests, the difficulty arises from the fact that strings are under control of the OCaml runtime, which can move them about unexpectedly. Unfortunately, even allocating a value to hold the pointer is enough to trigger a garbage collection, invalidating the pointer before it's even used.

# for i = 1 to max_int do
    if Ctypes.(!@ (string_start (String.copy "foo"))) <> 'f'
    then failwith (Printf.sprintf "Failed on iteration %d" i)
  done;;
      Exception: Failure "Failed on iteration 32762".
@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 12, 2014

Contributor

@yallop Oh. That would explain the intermittent failures I'm seeing.

How about a view that, when handled by ctypes, evaluates to the address of the string? I think the relevant codepaths don't include allocation (but I may be wrong).

Contributor

whitequark commented Apr 12, 2014

@yallop Oh. That would explain the intermittent failures I'm seeing.

How about a view that, when handled by ctypes, evaluates to the address of the string? I think the relevant codepaths don't include allocation (but I may be wrong).

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 12, 2014

Contributor

On a second thought, that probably wouldn't work, because the very action of evaluating a view reader would cause allocation.

What I really need is a way to invoke String_val(...) after transition from OCaml world to C world. I need to take a closer look at ctypes to understand whether this would be possible

Contributor

whitequark commented Apr 12, 2014

On a second thought, that probably wouldn't work, because the very action of evaluating a view reader would cause allocation.

What I really need is a way to invoke String_val(...) after transition from OCaml world to C world. I need to take a closer look at ctypes to understand whether this would be possible

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 12, 2014

Contributor

@yallop It took a fair amount of headscratching, but I fixed it.

Key points of my solution:

  • The central idea is that I force OCaml to track the values by itself, updating all references whenever it moves them, right until the point at which no allocations are possible.
  • I've added a phantom type 'a nocapture and a corresponding GADT variant. Currently the GADT variant does not store further type information. I wasn't sure how to better represent it.
  • I've also added a new kind of pointers, so-called movable ones. For movable pointers, no raw_ptr is stored, but only pmanaged. Thus, there is no value hidden from GC.
  • While collecting the arguments for the call, regular arguments are stored directly into the callbuffer, but the movable ones are collected in an OCaml array. After collecting all arguments, ctypes_call traverses the array and puts the proper pointers into callbuffer. At this point no OCaml code is executed and hence no movement can occur.
  • Movability is transitive with respect to coercion.
  • ctypes_call uses GC tags in order to determine what to do with the block. Currently it only handles strings, but can likely be easily extended to handle arrays.

Test case:

open Ctypes
open Foreign

let puts = foreign "puts" (ptr string_nocapture @-> returning void)
let _ = puts (string_to_nocapture "hello world")

let printf = foreign "printf" (ptr string_nocapture @-> ptr string_nocapture
                               @-> ptr string_nocapture @-> returning void)
let _ = printf (string_to_nocapture "formatting: %s -> %s\n")
               (string_to_nocapture "chiri")
               (string_to_nocapture "chan")

let puts' = foreign "puts" (ptr char @-> returning void)
let coercer = coerce (ptr string_nocapture) (ptr char)
let _ = puts' (coercer (string_to_nocapture "hello world"))

Verification: the following doesn't crash.

let printf = foreign "printf" (string @-> ptr string_nocapture @-> returning void)
for i = 1 to max_int do
  printf "%s" (string_to_nocapture (String.copy "q"))
done
Contributor

whitequark commented Apr 12, 2014

@yallop It took a fair amount of headscratching, but I fixed it.

Key points of my solution:

  • The central idea is that I force OCaml to track the values by itself, updating all references whenever it moves them, right until the point at which no allocations are possible.
  • I've added a phantom type 'a nocapture and a corresponding GADT variant. Currently the GADT variant does not store further type information. I wasn't sure how to better represent it.
  • I've also added a new kind of pointers, so-called movable ones. For movable pointers, no raw_ptr is stored, but only pmanaged. Thus, there is no value hidden from GC.
  • While collecting the arguments for the call, regular arguments are stored directly into the callbuffer, but the movable ones are collected in an OCaml array. After collecting all arguments, ctypes_call traverses the array and puts the proper pointers into callbuffer. At this point no OCaml code is executed and hence no movement can occur.
  • Movability is transitive with respect to coercion.
  • ctypes_call uses GC tags in order to determine what to do with the block. Currently it only handles strings, but can likely be easily extended to handle arrays.

Test case:

open Ctypes
open Foreign

let puts = foreign "puts" (ptr string_nocapture @-> returning void)
let _ = puts (string_to_nocapture "hello world")

let printf = foreign "printf" (ptr string_nocapture @-> ptr string_nocapture
                               @-> ptr string_nocapture @-> returning void)
let _ = printf (string_to_nocapture "formatting: %s -> %s\n")
               (string_to_nocapture "chiri")
               (string_to_nocapture "chan")

let puts' = foreign "puts" (ptr char @-> returning void)
let coercer = coerce (ptr string_nocapture) (ptr char)
let _ = puts' (coercer (string_to_nocapture "hello world"))

Verification: the following doesn't crash.

let printf = foreign "printf" (string @-> ptr string_nocapture @-> returning void)
for i = 1 to max_int do
  printf "%s" (string_to_nocapture (String.copy "q"))
done
@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 12, 2014

Contributor

The parts that are not covered are:

  • cstubs. I've no idea how they work. There are a few nonexhaustive matches there.
  • sanity checking. Does it even make sense to have a string nocapture ptr ptr? I honestly don't know, too tired right now.
    Otherwise the code should be solid.
Contributor

whitequark commented Apr 12, 2014

The parts that are not covered are:

  • cstubs. I've no idea how they work. There are a few nonexhaustive matches there.
  • sanity checking. Does it even make sense to have a string nocapture ptr ptr? I honestly don't know, too tired right now.
    Otherwise the code should be solid.
@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 17, 2014

Member

Thanks, @whitequark; this is very interesting! I think your approach (distinguishing between movable and non-movable objects all the way through to ctypes_call) is the right way to deal with passing OCaml objects to C. I'd like to press forward with your patch, since writing directly into OCaml objects from C is something that there's often a need for (particularly for float arrays and strings).

It's worth noting up front that there's still a danger of objects being moved if the GC runs during a C call. There are at least two circumstances where this can happen:

  1. if the C code calls back into OCaml. For example, you might bind qsort or bsearch using ctypes then pass an OCaml function as the comparator. Calling back into OCaml to compare values could trigger a GC, so it wouldn't be safe to pass an OCaml value as the array argument.
  2. where the call into C releases the runtime lock, allowing OCaml code to run in a different thread. Although ctypes doesn't yet support this, it will one day.

I don't see that there's much we can do to avoid these difficulties, but we can at least make them clear in the documentation

On to the details. I like the behaviour for calls, but making OCaml values look like pointers (values of ptr) introduces some problems. You touch on this when you say

Does it even make sense to have a string nocapture ptr ptr?

I don't think it does make sense. Since values of ptr are intended to behave like C pointers, we should be able to place them in addressable storage. It wouldn't be safe to do that for references to OCaml values; they need to be kept in one of the places where the GC can find them. You've already introduced a new kind of "movable" pointer. I'd like to expose your distinction in the interface by using different types for movable and immovable pointers.

I've added a phantom type 'a nocapture and a corresponding GADT variant.

I'd like to rename nocapture to ocaml, in order to describe what the value is, rather than focusing on one particular property. Other naming tweaks: let's use string_start (the name you originally proposed) for the function that obtains a reference to an OCaml values, and change its return type from ... ptr to string ocaml for the reasons given above. This'd give us the following additions to the Ctypes signature:

type 'a ocaml
val ocaml_string : string ocaml typ
val ocaml_string_start : string -> string ocaml

Once we've made ptr and ocaml distinct types, there's the question of pointer arithmetic, which is useful for both, as you point out (f7f9d4a). There are two possibilities:

  1. introduce a second set of functions, corresponding to +@, -@, and pointer_diff, that work on ocaml values
  2. make +@, -@ and pointer_diff polymorphic in the kind of pointers

The first approach is straightforward, but clutters up the interface with very similar functions. The second involves making ptr and ocaml instantiations of some common type. For example, we might have

type ('a, 'b) pointer
type 'a ptr = ('a, [`C]) pointer
type 'a ocaml = ('a, [`OCaml]) pointer

I'm more in favour of this second approach, which is similar to what I've done for structs and unions.

Currently the GADT variant does not store further type information.
I wasn't sure how to better represent it.

Let's add a typed enumeration that describes the OCaml type.

type _ ocaml_type =
  String : string ocaml_type
| FloatArray : float array ocaml_type

I'm not sure if there are any types other than string and float array that are important to support. We can always revisit that later, though. Also, it's not essential to push support for float array through the code just yet.

The constructor for representing types of movable value would then take one of the ocaml_type tags:

type _ typ =
  ...
| OCaml : 'a ocaml_type -> 'a ocaml typ

For movable pointers, no raw_ptr is stored, but only pmanaged. Thus,
there is no value hidden from GC.

I like the distinction and the semantics, but using a bool gives us a representation that's not as tight as I'd like. There's nothing in the types to ensure that pmanaged is populated and raw_ptr is unavailable when movable is true. I'd prefer to use a representation that makes explicit exactly when fields are available, such as the following:

type 'a cptr = { reftype      : 'a typ;
                 raw_ptr      : Ctypes_raw.voidp;
                 pmanaged     : Obj.t option;
                 pbyte_offset : int; }
type (_, _) pointer = 
  CPointer of 'a cptr -> ('a, [`C]) pointer
| OCamlRef : int * 'a -> ('a, [`OCaml]) pointer

Aside: one benefit of using a GADT here is that there's typically no additional matching overhead when OCaml knows enough about the types to tell which constructors are available. If you only match one case of a regular variant you'll get rather slow code (along with warnings about inexhaustivity). Here's an example:

type t =
  A of int
| B of float * string

let unA (A x) = x

Compiling with -dlambda shows the runtime tag inspection and the
Match_failure if the wrong argument is passed:

(let
  (unA/1011
     (function param/1015
       (switch param/1015
        case tag 0: (field 0 param/1015)
        default:
         (raise
           (makeblock 0 (global Match_failure/16g) [0: "/tmp/v.ml" 5 8])))))
  (makeblock 0 unA/1011))

If you add distinct type indices for each constructor then the type checker can tell that the function can never be passed a B:

type _ t =
  A : int -> [`A] t
| B : float * string -> [`B] t

let unA (A x) = x

The generated code doesn't inspect the tag, so field access performs the same as with records:

(let (unA/1011 (function param/1016 (field 0 param/1016)))
  (makeblock 0 unA/1011)))

Unfortunately, although there's no new branching overhead in most cases, there is an extra indirection, since both CPointer and its record argument are allocated blocks. We could avoid that by giving CPointer four individual arguments instead of a four-field record, with some loss of readability. If @alainfrisch's inline record patch patch is merged then we'll have the best of both worlds: we'll be able to name fields when constructing and destructing values without the overhead of an additional indirection.

Member

yallop commented Apr 17, 2014

Thanks, @whitequark; this is very interesting! I think your approach (distinguishing between movable and non-movable objects all the way through to ctypes_call) is the right way to deal with passing OCaml objects to C. I'd like to press forward with your patch, since writing directly into OCaml objects from C is something that there's often a need for (particularly for float arrays and strings).

It's worth noting up front that there's still a danger of objects being moved if the GC runs during a C call. There are at least two circumstances where this can happen:

  1. if the C code calls back into OCaml. For example, you might bind qsort or bsearch using ctypes then pass an OCaml function as the comparator. Calling back into OCaml to compare values could trigger a GC, so it wouldn't be safe to pass an OCaml value as the array argument.
  2. where the call into C releases the runtime lock, allowing OCaml code to run in a different thread. Although ctypes doesn't yet support this, it will one day.

I don't see that there's much we can do to avoid these difficulties, but we can at least make them clear in the documentation

On to the details. I like the behaviour for calls, but making OCaml values look like pointers (values of ptr) introduces some problems. You touch on this when you say

Does it even make sense to have a string nocapture ptr ptr?

I don't think it does make sense. Since values of ptr are intended to behave like C pointers, we should be able to place them in addressable storage. It wouldn't be safe to do that for references to OCaml values; they need to be kept in one of the places where the GC can find them. You've already introduced a new kind of "movable" pointer. I'd like to expose your distinction in the interface by using different types for movable and immovable pointers.

I've added a phantom type 'a nocapture and a corresponding GADT variant.

I'd like to rename nocapture to ocaml, in order to describe what the value is, rather than focusing on one particular property. Other naming tweaks: let's use string_start (the name you originally proposed) for the function that obtains a reference to an OCaml values, and change its return type from ... ptr to string ocaml for the reasons given above. This'd give us the following additions to the Ctypes signature:

type 'a ocaml
val ocaml_string : string ocaml typ
val ocaml_string_start : string -> string ocaml

Once we've made ptr and ocaml distinct types, there's the question of pointer arithmetic, which is useful for both, as you point out (f7f9d4a). There are two possibilities:

  1. introduce a second set of functions, corresponding to +@, -@, and pointer_diff, that work on ocaml values
  2. make +@, -@ and pointer_diff polymorphic in the kind of pointers

The first approach is straightforward, but clutters up the interface with very similar functions. The second involves making ptr and ocaml instantiations of some common type. For example, we might have

type ('a, 'b) pointer
type 'a ptr = ('a, [`C]) pointer
type 'a ocaml = ('a, [`OCaml]) pointer

I'm more in favour of this second approach, which is similar to what I've done for structs and unions.

Currently the GADT variant does not store further type information.
I wasn't sure how to better represent it.

Let's add a typed enumeration that describes the OCaml type.

type _ ocaml_type =
  String : string ocaml_type
| FloatArray : float array ocaml_type

I'm not sure if there are any types other than string and float array that are important to support. We can always revisit that later, though. Also, it's not essential to push support for float array through the code just yet.

The constructor for representing types of movable value would then take one of the ocaml_type tags:

type _ typ =
  ...
| OCaml : 'a ocaml_type -> 'a ocaml typ

For movable pointers, no raw_ptr is stored, but only pmanaged. Thus,
there is no value hidden from GC.

I like the distinction and the semantics, but using a bool gives us a representation that's not as tight as I'd like. There's nothing in the types to ensure that pmanaged is populated and raw_ptr is unavailable when movable is true. I'd prefer to use a representation that makes explicit exactly when fields are available, such as the following:

type 'a cptr = { reftype      : 'a typ;
                 raw_ptr      : Ctypes_raw.voidp;
                 pmanaged     : Obj.t option;
                 pbyte_offset : int; }
type (_, _) pointer = 
  CPointer of 'a cptr -> ('a, [`C]) pointer
| OCamlRef : int * 'a -> ('a, [`OCaml]) pointer

Aside: one benefit of using a GADT here is that there's typically no additional matching overhead when OCaml knows enough about the types to tell which constructors are available. If you only match one case of a regular variant you'll get rather slow code (along with warnings about inexhaustivity). Here's an example:

type t =
  A of int
| B of float * string

let unA (A x) = x

Compiling with -dlambda shows the runtime tag inspection and the
Match_failure if the wrong argument is passed:

(let
  (unA/1011
     (function param/1015
       (switch param/1015
        case tag 0: (field 0 param/1015)
        default:
         (raise
           (makeblock 0 (global Match_failure/16g) [0: "/tmp/v.ml" 5 8])))))
  (makeblock 0 unA/1011))

If you add distinct type indices for each constructor then the type checker can tell that the function can never be passed a B:

type _ t =
  A : int -> [`A] t
| B : float * string -> [`B] t

let unA (A x) = x

The generated code doesn't inspect the tag, so field access performs the same as with records:

(let (unA/1011 (function param/1016 (field 0 param/1016)))
  (makeblock 0 unA/1011)))

Unfortunately, although there's no new branching overhead in most cases, there is an extra indirection, since both CPointer and its record argument are allocated blocks. We could avoid that by giving CPointer four individual arguments instead of a four-field record, with some loss of readability. If @alainfrisch's inline record patch patch is merged then we'll have the best of both worlds: we'll be able to name fields when constructing and destructing values without the overhead of an additional indirection.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 26, 2014

Contributor

@yallop I think this wouldn't work. In your scheme, it's not possible to write a coercion from string ocaml to char ptr, which is exactly what I want for the ocaml-sodium API, where I need to be able to pass pointers either to strings or to character arrays.

Contributor

whitequark commented Apr 26, 2014

@yallop I think this wouldn't work. In your scheme, it's not possible to write a coercion from string ocaml to char ptr, which is exactly what I want for the ocaml-sodium API, where I need to be able to pass pointers either to strings or to character arrays.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 26, 2014

Member

I wonder if it's possible to avoid the to_ptr coercion altogether by changing the types used in the ocaml-sodium foreign bindings. Although it may be safe to coerce to ptr in this case, pointers to movable objects have unpredictable behaviour in general, so I'd prefer not to make the coercion operation available.

(Note that it's possible to have separate parallel foreign bindings to the same functions for bigarrays and for strings. Perhaps you could add a ctypes typ member to the Storage signature and use it to parameterize the foreign bindings, along these lines:

module C(S : Storage) =
struct
    let prefix      = "crypto_verify"

    let verify_type = S.typ @-> S.typ @-> returning int
    let verify_16   = foreign (prefix^"_16") verify_type
    let verify_32   = foreign (prefix^"_32") verify_type
end

)

Member

yallop commented Apr 26, 2014

I wonder if it's possible to avoid the to_ptr coercion altogether by changing the types used in the ocaml-sodium foreign bindings. Although it may be safe to coerce to ptr in this case, pointers to movable objects have unpredictable behaviour in general, so I'd prefer not to make the coercion operation available.

(Note that it's possible to have separate parallel foreign bindings to the same functions for bigarrays and for strings. Perhaps you could add a ctypes typ member to the Storage signature and use it to parameterize the foreign bindings, along these lines:

module C(S : Storage) =
struct
    let prefix      = "crypto_verify"

    let verify_type = S.typ @-> S.typ @-> returning int
    let verify_16   = foreign (prefix^"_16") verify_type
    let verify_32   = foreign (prefix^"_32") verify_type
end

)

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 26, 2014

Contributor

@yallop This would require me to duplicate all code inside the functor, not just that which differs depending on storage. I very much dislike this solution as too heavy.

Contributor

whitequark commented Apr 26, 2014

@yallop This would require me to duplicate all code inside the functor, not just that which differs depending on storage. I very much dislike this solution as too heavy.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 26, 2014

Member

My difficulty is as follows: although the coercion from OCaml strings to ptr supports the operations you need in this case (arithmetic and passing the ptr to C functions), it doesn't support ptr operations in general (e.g. dereferencing the ptr, storing it as a struct member, turning it into an array or bigarray, etc.). I'd therefore prefer to keep it as a separate type, although perhaps one that has some operations in common with ptr.

It looks like many operations in ocaml-sodium are already defined inside functors parameterized by Storage.S. Would it really make things so much heavier to move a few more definitions into those functors?

Member

yallop commented Apr 26, 2014

My difficulty is as follows: although the coercion from OCaml strings to ptr supports the operations you need in this case (arithmetic and passing the ptr to C functions), it doesn't support ptr operations in general (e.g. dereferencing the ptr, storing it as a struct member, turning it into an array or bigarray, etc.). I'd therefore prefer to keep it as a separate type, although perhaps one that has some operations in common with ptr.

It looks like many operations in ocaml-sodium are already defined inside functors parameterized by Storage.S. Would it really make things so much heavier to move a few more definitions into those functors?

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark Apr 26, 2014

Contributor

Definitely. There's no reason for e.g. random_keypair to be in a module parameterized by Storage, because it's not actually different and it's confusing.

That being said, I may be able to split each C module in two, one of them being parameterized.

Contributor

whitequark commented Apr 26, 2014

Definitely. There's no reason for e.g. random_keypair to be in a module parameterized by Storage, because it's not actually different and it's confusing.

That being said, I may be able to split each C module in two, one of them being parameterized.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 3, 2014

Contributor

@yallop I've encountered another hurdle. In +@, one needs to know the size of the element. But, ('a, [`OCaml]) pointer does not have this information.

In principle it is possible to detect it on the C side based on the tag, and this is what I'm going to implement so far, but I do not like this solution too much.

For similar reasons, it's not possible to pass an 'a ocaml to reference_type.

Contributor

whitequark commented May 3, 2014

@yallop I've encountered another hurdle. In +@, one needs to know the size of the element. But, ('a, [`OCaml]) pointer does not have this information.

In principle it is possible to detect it on the C side based on the tag, and this is what I'm going to implement so far, but I do not like this solution too much.

For similar reasons, it's not possible to pass an 'a ocaml to reference_type.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 3, 2014

Member

It's possible to delay looking up the size of the element until ctypes_call if you treat the first argument to OCamlRef as an element offset rather than a byte offset. For C pointers it's better to have the offsets be a byte offset, since it makes conversions between pointer types much simpler. However, for ('a, [`OCaml]) pointer there aren't any conversions defined so it's fine to delay determining the element size until you're actually in a call.

Supporting reference_type is a good idea (although it'll probably end up returning string rather than char etc., which is slightly different to the situation for C pointers). I suppose it would be possible to implement this for by inspecting the object tag, which should always be equal to Obj.string_tag or Obj.double_array_tag. However, that'd involve an unsafe cast, since there's no GADT available to refine the return type. It's probably better to simply store the ocaml_type in the OCamlRef and return that directly from reference_type.

Member

yallop commented May 3, 2014

It's possible to delay looking up the size of the element until ctypes_call if you treat the first argument to OCamlRef as an element offset rather than a byte offset. For C pointers it's better to have the offsets be a byte offset, since it makes conversions between pointer types much simpler. However, for ('a, [`OCaml]) pointer there aren't any conversions defined so it's fine to delay determining the element size until you're actually in a call.

Supporting reference_type is a good idea (although it'll probably end up returning string rather than char etc., which is slightly different to the situation for C pointers). I suppose it would be possible to implement this for by inspecting the object tag, which should always be equal to Obj.string_tag or Obj.double_array_tag. However, that'd involve an unsafe cast, since there's no GADT available to refine the return type. It's probably better to simply store the ocaml_type in the OCamlRef and return that directly from reference_type.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 3, 2014

Contributor

@yallop It's actually not required, as build_ccallspec has access to the type of the argument.

I have made the changes you have suggested in full; including the float array part. I am only now testing them on ocaml-sodium, but please take a look nevertheless, as changes to ocaml-sodium may take a while.

Contributor

whitequark commented May 3, 2014

@yallop It's actually not required, as build_ccallspec has access to the type of the argument.

I have made the changes you have suggested in full; including the float array part. I am only now testing them on ocaml-sodium, but please take a look nevertheless, as changes to ocaml-sodium may take a while.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 3, 2014

Contributor

On IRC, @gasche suggested that the usage of polymorphic variants is needless here, and it would be better to use fully abstract phantom types instead.

Contributor

whitequark commented May 3, 2014

On IRC, @gasche suggested that the usage of polymorphic variants is needless here, and it would be better to use fully abstract phantom types instead.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 3, 2014

Contributor

@yallop The ocaml-sodium testsuite now passes with new ctypes interface. It's probably safe to merge the PR, provided you have no other concerns.

Contributor

whitequark commented May 3, 2014

@yallop The ocaml-sodium testsuite now passes with new ctypes interface. It's probably safe to merge the PR, provided you have no other concerns.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 6, 2014

Contributor

I have additionally added support for the bytes type in >=4.02. Backwards compatibility is taken of by trunk ocamlfind.

Note that this would need that opam package depend on base-bytes, when that is available in opam. /cc @AltGr

Contributor

whitequark commented May 6, 2014

I have additionally added support for the bytes type in >=4.02. Backwards compatibility is taken of by trunk ocamlfind.

Note that this would need that opam package depend on base-bytes, when that is available in opam. /cc @AltGr

@yallop

View changes

Show outdated Hide outdated src/ctypes-foreign-base/ffi_call_stubs.c
int arg_idx;
for(arg_idx = 0; arg_idx < Wosize_val(callback_val_arr); arg_idx++) {
value arg_tuple = Field(callback_val_arr, arg_idx);
if(!arg_tuple) continue;

This comment has been minimized.

@yallop

yallop May 8, 2014

Member

Unfortunately this no longer works in 4.02, since caml_alloc_tuple now (05100e5) initializes fields to Val_unit, not to 0.

@yallop

yallop May 8, 2014

Member

Unfortunately this no longer works in 4.02, since caml_alloc_tuple now (05100e5) initializes fields to Val_unit, not to 0.

This comment has been minimized.

@whitequark

whitequark May 8, 2014

Contributor

I've considered several ways to fix this, but seeing as OCaml doesn't export its version in C headers, the simplest way is just to check for both. So, fixed.

@whitequark

whitequark May 8, 2014

Contributor

I've considered several ways to fix this, but seeing as OCaml doesn't export its version in C headers, the simplest way is just to check for both. So, fixed.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 8, 2014

Member

Thanks for the updates! I'm hoping to merge once I've finished working through various issues (e.g. convincing myself that stub generation will work, and writing some tests). I'll add comments inline in the meantime.

One general issue: at present it's possible to create struct/union types with OCaml values as fields, which isn't really a useful thing to be able to do. Reading or writing the field raises an exception:

let s = structure "s";;
let os = field s "os" ocaml_string;;
let () = seal s;;

let v = make s;;

setf v os (ocaml_string_start "foo");;
~> Exception: Static.IncompleteType.

getf v os;;
~> Exception: Assert_failure ("src/ctypes/memory.ml", 98, 19).

Making sizeof and alignment raise IncompleteType (or some similar exception) for OCaml types would address the problem, and is probably the right solution anyway.

Member

yallop commented May 8, 2014

Thanks for the updates! I'm hoping to merge once I've finished working through various issues (e.g. convincing myself that stub generation will work, and writing some tests). I'll add comments inline in the meantime.

One general issue: at present it's possible to create struct/union types with OCaml values as fields, which isn't really a useful thing to be able to do. Reading or writing the field raises an exception:

let s = structure "s";;
let os = field s "os" ocaml_string;;
let () = seal s;;

let v = make s;;

setf v os (ocaml_string_start "foo");;
~> Exception: Static.IncompleteType.

getf v os;;
~> Exception: Assert_failure ("src/ctypes/memory.ml", 98, 19).

Making sizeof and alignment raise IncompleteType (or some similar exception) for OCaml types would address the problem, and is probably the right solution anyway.

@yallop

View changes

Show outdated Hide outdated src/ctypes/memory.ml
CPointer {raw_ptr = rp; pbyte_offset = roff} ->
Raw.PtrType.(compare (add lp (of_int loff)) (add rp (of_int roff)))
| OCamlRef (loff, lobj), OCamlRef (roff, robj) ->
assert (lobj = robj); compare loff roff

This comment has been minimized.

@yallop

yallop May 8, 2014

Member

Since ptr_compare raises Assert_failure if the OCaml objects are not structurally equal, it doesn't conform to the typical requirements that comparison functions be total (e.g. in the Ord argument to Map.Make). Let's just leave ptr_compare restricted to C pointers, where it's possible to give a total ordering. We'll still have ptr_diff for OCaml references.

@yallop

yallop May 8, 2014

Member

Since ptr_compare raises Assert_failure if the OCaml objects are not structurally equal, it doesn't conform to the typical requirements that comparison functions be total (e.g. in the Ord argument to Map.Make). Let's just leave ptr_compare restricted to C pointers, where it's possible to give a total ordering. We'll still have ptr_diff for OCaml references.

@yallop

View changes

Show outdated Hide outdated src/ctypes/std_views.ml
@@ -5,34 +5,37 @@
* See the file LICENSE for details.
*)
let string_of_char_ptr {Static.raw_ptr; pbyte_offset} =
open Static

This comment has been minimized.

@yallop

yallop May 8, 2014

Member

I'd prefer to stick with local opens (Static.(...)) rather than open Static.

@yallop

yallop May 8, 2014

Member

I'd prefer to stick with local opens (Static.(...)) rather than open Static.

@yallop

View changes

Show outdated Hide outdated src/ctypes/memory.ml
let l = add lp (of_int loff)
and r = add rp (of_int roff) in
to_int (sub r l) / sizeof reftype
| OCamlRef (lo, _), OCamlRef (ro, _) -> ro - lo

This comment has been minimized.

@yallop

yallop May 8, 2014

Member

We should raise Invalid_argument if the OCamlRefs don't reference the same (==) object.

@yallop

yallop May 8, 2014

Member

We should raise Invalid_argument if the OCamlRefs don't reference the same (==) object.

@yallop

View changes

Show outdated Hide outdated src/ctypes/memory.ml
| Bigarray b -> Ctypes_bigarray.view b ?ref ~offset raw_ptr
| Abstract _ -> { structured = ptr }
| OCaml _ -> assert false

This comment has been minimized.

@yallop

yallop May 8, 2014

Member

IncompleteType (or some other appropriately-named exception such as Non_C_type) would be more appropriate than Assert_failure here. Assert_failure suggests an error in the implementation, not a user error.

@yallop

yallop May 8, 2014

Member

IncompleteType (or some other appropriately-named exception such as Non_C_type) would be more appropriate than Assert_failure here. Assert_failure suggests an error in the implementation, not a user error.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 8, 2014

Contributor

Fixed.

Let's merge this today.

Contributor

whitequark commented May 8, 2014

Fixed.

Let's merge this today.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 8, 2014

Member

There's a tricky issue with passing double arrays to C: some platforms have a strict requirement that doubles be 8-byte aligned, which isn't guaranteed to be the case for OCaml-managed objects. As far as I can see, this means that we simply can't pass float array values to C on those platforms. OCaml itself takes care to copy doubles to aligned memory ([1], [2]) on those platforms. For ctypes we have at least the following options, none of which is ideal.

  1. Use conditional compilation to disable ocaml_float_array on platforms with strict alignment requirements.
  2. Raise a runtime error when a float array is passed to C on a platform with strict alignment requirements. This is the approach taken by other libraries (e.g. lwt and the standard library) for features that are not available on particular platforms.
  3. Copy the array into aligned memory before passing it to C. This approach weakens the guarantees given by the interface (that the ocaml_ types don't involve copying), but the behaviour should at least be consistent across platforms. There's some similarity with OCaml's approach to floats in general: presumably accesses on platforms with strict alignment requirements are significantly slower, but the behaviour is otherwise the same.
Member

yallop commented May 8, 2014

There's a tricky issue with passing double arrays to C: some platforms have a strict requirement that doubles be 8-byte aligned, which isn't guaranteed to be the case for OCaml-managed objects. As far as I can see, this means that we simply can't pass float array values to C on those platforms. OCaml itself takes care to copy doubles to aligned memory ([1], [2]) on those platforms. For ctypes we have at least the following options, none of which is ideal.

  1. Use conditional compilation to disable ocaml_float_array on platforms with strict alignment requirements.
  2. Raise a runtime error when a float array is passed to C on a platform with strict alignment requirements. This is the approach taken by other libraries (e.g. lwt and the standard library) for features that are not available on particular platforms.
  3. Copy the array into aligned memory before passing it to C. This approach weakens the guarantees given by the interface (that the ocaml_ types don't involve copying), but the behaviour should at least be consistent across platforms. There's some similarity with OCaml's approach to floats in general: presumably accesses on platforms with strict alignment requirements are significantly slower, but the behaviour is otherwise the same.
@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 8, 2014

Contributor

I think we should go with 1). The whole point of float array ocaml is to have fast access; without OCaml's internal workaround, there would be no access at all, however ctypes already has copying.

Is there something for conditional compilation in ctypes?

Contributor

whitequark commented May 8, 2014

I think we should go with 1). The whole point of float array ocaml is to have fast access; without OCaml's internal workaround, there would be no access at all, however ctypes already has copying.

Is there something for conditional compilation in ctypes?

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 9, 2014

Member

Ok, I'm going to go ahead and merge, splitting the bytes support into a separate pull request, until the findlib support is more widely available.

I'll leave ocaml_float_array out of the public interface for the moment, since it's not yet fully supported in any case.

(There's nothing for conditional compilation in ctypes itself. Using optcomp is a possibility, but I'd rather just look at ARCH_ALIGN_DOUBLE during configuration and pick a suitable module interface if we go that route.)

Member

yallop commented May 9, 2014

Ok, I'm going to go ahead and merge, splitting the bytes support into a separate pull request, until the findlib support is more widely available.

I'll leave ocaml_float_array out of the public interface for the moment, since it's not yet fully supported in any case.

(There's nothing for conditional compilation in ctypes itself. Using optcomp is a possibility, but I'd rather just look at ARCH_ALIGN_DOUBLE during configuration and pick a suitable module interface if we go that route.)

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli May 9, 2014

Contributor

If 1. entails configuration burden for ctypes client I don't see as a good thing. Personally I would prefer 3. with a boolean flag that allows us to detect if on this platform we are doing copying rather than passing the floats directly.

Contributor

dbuenzli commented May 9, 2014

If 1. entails configuration burden for ctypes client I don't see as a good thing. Personally I would prefer 3. with a boolean flag that allows us to detect if on this platform we are doing copying rather than passing the floats directly.

@whitequark

This comment has been minimized.

Show comment
Hide comment
@whitequark

whitequark May 9, 2014

Contributor

@dbuenzli @yallop Actually I've changed my mind. Since the fallback for broken passing of float array ocaml would clearly be just passing float array and letting ctypes do the copy, it makes sense to make this behavior built-in.

Contributor

whitequark commented May 9, 2014

@dbuenzli @yallop Actually I've changed my mind. Since the fallback for broken passing of float array ocaml would clearly be just passing float array and letting ctypes do the copy, it makes sense to make this behavior built-in.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 9, 2014

Member

Merged (with changes noted above): 3d3747f. Thanks for the great new feature, and for your patience!

Member

yallop commented May 9, 2014

Merged (with changes noted above): 3d3747f. Thanks for the great new feature, and for your patience!

@@ -275,13 +275,12 @@ value ctypes_prep_callspec(value callspec_, value abi_, value rtype)
/* Call the function specified by `callspec', passing arguments and return
values in `buffer' */
/* call : raw_pointer -> callspec -> (raw_pointer -> unit) ->
/* call : raw_pointer -> callspec -> (raw_pointer -> Obj.t array -> unit) ->

This comment has been minimized.

@gasche

gasche May 9, 2014

Isn't there a discrepancy between the comment here, which says raw_pointer -> Obj.t array -> unit, and the OCaml interface which as raw_pointer -> (Obj.t array * int) -> unit?

(I'm just trying to make sense of the code to understand the Val_unit initialization problem. On a different note orthogonal to this patch, why aren't the OCaml-exposed functions annotated with CAMLprim?)

@gasche

gasche May 9, 2014

Isn't there a discrepancy between the comment here, which says raw_pointer -> Obj.t array -> unit, and the OCaml interface which as raw_pointer -> (Obj.t array * int) -> unit?

(I'm just trying to make sense of the code to understand the Val_unit initialization problem. On a different note orthogonal to this patch, why aren't the OCaml-exposed functions annotated with CAMLprim?)

This comment has been minimized.

@whitequark

whitequark May 9, 2014

Contributor

There is. I forgot to update the comment. (As a side note, a very significant proportion of such annotations I have encountered is plain incorrect. There must be some better solution...)

@whitequark

whitequark May 9, 2014

Contributor

There is. I forgot to update the comment. (As a side note, a very significant proportion of such annotations I have encountered is plain incorrect. There must be some better solution...)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche May 9, 2014

Regarding the value initialization question: why don't you change the protocol of argument writers to also return the next available index, instead of trying to understand which values have been written after-the-fact? (besides, what happens if one argument really is equal to Val_true?) I haven't a good picture of what's happening yet, but it seems that the main place where this gets decided is ffi.ml:Make:write_arg, which should be amenable to such an interface.

(Is it possibe that a given argument writer would fill more than one argument (unboxed tuples)? If the stuff that you get from the user in fact only ever fills one argument (and you concatenate them in ffi.ml:Make/invoke), you could expose a different interface where the array is not shown, you take an array of functions that each return one argument, and you just have to check that the number of such functions coincides with the expected argument number.)

gasche commented May 9, 2014

Regarding the value initialization question: why don't you change the protocol of argument writers to also return the next available index, instead of trying to understand which values have been written after-the-fact? (besides, what happens if one argument really is equal to Val_true?) I haven't a good picture of what's happening yet, but it seems that the main place where this gets decided is ffi.ml:Make:write_arg, which should be amenable to such an interface.

(Is it possibe that a given argument writer would fill more than one argument (unboxed tuples)? If the stuff that you get from the user in fact only ever fills one argument (and you concatenate them in ffi.ml:Make/invoke), you could expose a different interface where the array is not shown, you take an array of functions that each return one argument, and you just have to check that the number of such functions coincides with the expected argument number.)

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 9, 2014

Member

@gasche: I agree that the protocol can be changed to avoid the need to check what's been written after-the-fact, and this is probably best in the long run. Rather than returning the next available index from the writers, though, we'll probably just compute everything in advance. There's a general policy in the code of making everything specializable, which accounts (in part!) for the unusual style in ffi.ml.

besides, what happens if one argument really is equal to Val_true?

There's no danger of that: all of the elements written are blocks, and this is actually enforced by the type signature for ctypes_call function --- see the second argument to the third argument(!),

Is it possibe that a given argument writer would fill more than one argument (unboxed tuples)?

It's not possible. The array is actually a secondary mechanism for handling the specific case of passing C-like OCaml values directly to C, so it only needs to handle values of types string, byte and float array. C-like arguments (i.e. everything else) are written directly into a block of memory outside the heap, so the OCaml value representation doesn't come into play.

Member

yallop commented May 9, 2014

@gasche: I agree that the protocol can be changed to avoid the need to check what's been written after-the-fact, and this is probably best in the long run. Rather than returning the next available index from the writers, though, we'll probably just compute everything in advance. There's a general policy in the code of making everything specializable, which accounts (in part!) for the unusual style in ffi.ml.

besides, what happens if one argument really is equal to Val_true?

There's no danger of that: all of the elements written are blocks, and this is actually enforced by the type signature for ctypes_call function --- see the second argument to the third argument(!),

Is it possibe that a given argument writer would fill more than one argument (unboxed tuples)?

It's not possible. The array is actually a secondary mechanism for handling the specific case of passing C-like OCaml values directly to C, so it only needs to handle values of types string, byte and float array. C-like arguments (i.e. everything else) are written directly into a block of memory outside the heap, so the OCaml value representation doesn't come into play.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 9, 2014

Member

I thought "we'll probably just compute everything in advance" deserved a little more explanation. When you use ctypes to make a call into C there are three distinct stages. Here's an example call:

let plus = foreign "plus" (int @-> int @-> returning int) in
  plus 1 2

The first stage corresponds to compilation, and depends only on the type signature of the function (int @-> int @-> returning int). Once we know the types of the arguments and return value we compute the layout of the call frame buffer immediately; there's no need to wait for the actual arguments. With the currently-released libffi-based backend for ctypes this is fairly involved, but if you trace through the code you'll see that that's what happens: once the function type is available, ctypes allocates a description of the frame and iterates over the arguments, computing the offsets of the call frame buffer (see also).

The second stage corresponds to linking, and depends on the name of the function ("plus"). With the libffi backend this stage is much simpler: it mostly just involves a call to dlsym to turn the name into an address.

The final stage involves actually calling the function by passing the arguments (plus 1 2). Most of the work has already been done by this point; we just need to allocate a call frame buffer, write the arguments into the buffer and make the call.

Since the feature under discussion in this pull request --- passing OCaml values into C --- is new, the code isn't so aggressive about pushing computations into the earliest possible stage. That'll probably change in the longer term, though, at which point the problem of determining which array elements have been initialized will disappear, since we won't be allocating space for elements that are never initialized.

Member

yallop commented May 9, 2014

I thought "we'll probably just compute everything in advance" deserved a little more explanation. When you use ctypes to make a call into C there are three distinct stages. Here's an example call:

let plus = foreign "plus" (int @-> int @-> returning int) in
  plus 1 2

The first stage corresponds to compilation, and depends only on the type signature of the function (int @-> int @-> returning int). Once we know the types of the arguments and return value we compute the layout of the call frame buffer immediately; there's no need to wait for the actual arguments. With the currently-released libffi-based backend for ctypes this is fairly involved, but if you trace through the code you'll see that that's what happens: once the function type is available, ctypes allocates a description of the frame and iterates over the arguments, computing the offsets of the call frame buffer (see also).

The second stage corresponds to linking, and depends on the name of the function ("plus"). With the libffi backend this stage is much simpler: it mostly just involves a call to dlsym to turn the name into an address.

The final stage involves actually calling the function by passing the arguments (plus 1 2). Most of the work has already been done by this point; we just need to allocate a call frame buffer, write the arguments into the buffer and make the call.

Since the feature under discussion in this pull request --- passing OCaml values into C --- is new, the code isn't so aggressive about pushing computations into the earliest possible stage. That'll probably change in the longer term, though, at which point the problem of determining which array elements have been initialized will disappear, since we won't be allocating space for elements that are never initialized.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop May 9, 2014

Member

@whitequark, @dbuenzli: agreed; falling back to a copy for float arrays on ARCH_ALIGN_DOUBLE platforms seems best.

Member

yallop commented May 9, 2014

@whitequark, @dbuenzli: agreed; falling back to a copy for float arrays on ARCH_ALIGN_DOUBLE platforms seems best.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche May 9, 2014

Terrific answer, thanks! You should absolutely copy what you just wrote and paste it in some documentation somewhere.

gasche commented May 9, 2014

Terrific answer, thanks! You should absolutely copy what you just wrote and paste it in some documentation somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment