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 bool type corresponding to the llvm i1/ C99 bool #220

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@artagnon
Contributor

artagnon commented Nov 9, 2014

The idea is to find a way to represent the llvm i1 type, since llvm
ExecutionEngine also uses libffi now. It's a single bit, but I thought
I'd try to coerce it from an i8 (char). This proof-of-concept requires
more work, especially in the area of tests.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 10, 2014

Member

The build problem should be fixed if you rebase against master.

Member

yallop commented Nov 10, 2014

The build problem should be fixed if you rebase against master.

@artagnon

This comment has been minimized.

Show comment
Hide comment
@artagnon

artagnon Nov 11, 2014

Contributor

@yallop Okay, I have a preliminary version now. The type-printing test passes, but I have no idea if it really works. How do I ask my Makefile to use this instead of the opam version? I tried prepending $ROOT/_build/src/ctypes to OCAMLPATH like I do for llvm. But it doesn't seem to work. Tips?

Contributor

artagnon commented Nov 11, 2014

@yallop Okay, I have a preliminary version now. The type-printing test passes, but I have no idea if it really works. How do I ask my Makefile to use this instead of the opam version? I tried prepending $ROOT/_build/src/ctypes to OCAMLPATH like I do for llvm. But it doesn't seem to work. Tips?

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 11, 2014

Member

You can pin the local copy using

opam pin add -k git /path/to/local/copy

Thanks for the proposal and implementation! I'm unusually busy at the moment, but I'll have some time to take a proper look in a few days.

Member

yallop commented Nov 11, 2014

You can pin the local copy using

opam pin add -k git /path/to/local/copy

Thanks for the proposal and implementation! I'm unusually busy at the moment, but I'll have some time to take a proper look in a few days.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 15, 2014

Member

I think the implementation is correct, but rather than introducing a new type Unsigned.bool we should use the builtin bool type on the OCaml to improve interoperability with other OCaml code.

Switching to the builtin bool type will probably make the implementation slightly simpler, since you can remove the additions to Unsigned altogether. OCaml has Bool_val/Val_bool macros which you can use in type_info_stubs.c

Some other consequences of switching to the standard bool type:

  • OCaml's bools are immediate, so we can mark bool as noalloc in Cstubs_analysis
  • We'll be able to write true or false in the tests and other code instead of Bool.of_int 0

It'd be nice to print bool values as "true" or "false" (which you can test in test-value_printing, if you're feeling so inclined).

Minor: Cstubs_public_name.ident_of_ml_prim needs an extra case.

Member

yallop commented Nov 15, 2014

I think the implementation is correct, but rather than introducing a new type Unsigned.bool we should use the builtin bool type on the OCaml to improve interoperability with other OCaml code.

Switching to the builtin bool type will probably make the implementation slightly simpler, since you can remove the additions to Unsigned altogether. OCaml has Bool_val/Val_bool macros which you can use in type_info_stubs.c

Some other consequences of switching to the standard bool type:

  • OCaml's bools are immediate, so we can mark bool as noalloc in Cstubs_analysis
  • We'll be able to write true or false in the tests and other code instead of Bool.of_int 0

It'd be nice to print bool values as "true" or "false" (which you can test in test-value_printing, if you're feeling so inclined).

Minor: Cstubs_public_name.ident_of_ml_prim needs an extra case.

@artagnon artagnon changed the title from attempt to add bool type corresponding to the llvm i1 to Add bool type corresponding to the llvm i1/ C99 bool Nov 19, 2014

@artagnon

This comment has been minimized.

Show comment
Hide comment
@artagnon

artagnon Nov 19, 2014

Contributor

@yallop Thanks for the guidance! How does the latest revision look? I'm still working on value printing.

Contributor

artagnon commented Nov 19, 2014

@yallop Thanks for the guidance! How does the latest revision look? I'm still working on value printing.

@artagnon

This comment has been minimized.

Show comment
Hide comment
@artagnon

artagnon Nov 19, 2014

Contributor

Ah, value printing started working after I did a make clean. The Makefile didn't resolve the dependencies correctly, for some reason.

Contributor

artagnon commented Nov 19, 2014

Ah, value printing started working after I did a make clean. The Makefile didn't resolve the dependencies correctly, for some reason.

@yallop

View changes

Show outdated Hide outdated src/cstubs/cstubs_generate_c.ml
@@ -93,6 +94,7 @@ struct
| Char -> immediater "Val_int" (int @-> returning value)
| Schar -> immediater "Val_int" (int @-> returning value)
| Uchar -> conser "ctypes_copy_uint8" (uint8_t @-> returning value)
| Bool -> conser "Val_bool" (bool @-> returning value)

This comment has been minimized.

@yallop

yallop Nov 20, 2014

Member

conser here should be immediater since Val_of_bool doesn't allocate.

@yallop

yallop Nov 20, 2014

Member

conser here should be immediater since Val_of_bool doesn't allocate.

@yallop

View changes

Show outdated Hide outdated src/ctypes-foreign-base/ffi_type_stubs.c
@@ -53,6 +53,7 @@ static ffi_type *primitive_ffi_types[] = {
&ctypes_ffi_type_char, /* Char */
&ffi_type_schar, /* Schar */
&ffi_type_uchar, /* Uchar */
&ffi_type_uchar, /* Bool */

This comment has been minimized.

@yallop

yallop Nov 20, 2014

Member

Unfortunately libffi doesn't support passing bools, and C doesn't guarantee that bools are passed or represented as unsigned chars, so &ffi_type_uichar here should be NULL.

@yallop

yallop Nov 20, 2014

Member

Unfortunately libffi doesn't support passing bools, and C doesn't guarantee that bools are passed or represented as unsigned chars, so &ffi_type_uichar here should be NULL.

@yallop

View changes

Show outdated Hide outdated src/ctypes/unsigned.ml
@@ -230,12 +230,14 @@ external ulonglong_size : unit -> int = "ctypes_ulonglong_size"
module Size_t : S = (val pick ~size:(size_t_size ()))
module UChar : S = UInt8
module Bool : S = UInt8

This comment has been minimized.

@yallop

yallop Nov 20, 2014

Member

This is no longer needed.

@yallop

yallop Nov 20, 2014

Member

This is no longer needed.

@yallop

View changes

Show outdated Hide outdated src/ctypes/unsigned.ml
module UShort : S = (val pick ~size:(ushort_size ()))
module UInt : S = (val pick ~size:(uint_size ()))
module ULong : S = (val pick ~size:(ulong_size ()))
module ULLong : S = (val pick ~size:(ulonglong_size ()))
type uchar = UChar.t
type bool = Bool.t

This comment has been minimized.

@yallop

yallop Nov 20, 2014

Member

This is no longer needed.

@yallop

yallop Nov 20, 2014

Member

This is no longer needed.

@yallop

View changes

Show outdated Hide outdated src/ctypes/unsigned.mli
@@ -115,6 +115,9 @@ end
module UChar : S
(** Unsigned char type and operations. *)
module Bool : S
(** Bool type and operations. *)

This comment has been minimized.

@yallop

yallop Nov 20, 2014

Member

The Unsigned.Bool module is no longer needed.

@yallop

yallop Nov 20, 2014

Member

The Unsigned.Bool module is no longer needed.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 20, 2014

Member

Thanks for the update. I've added some final comments inline.

Member

yallop commented Nov 20, 2014

Thanks for the update. I've added some final comments inline.

@artagnon

This comment has been minimized.

Show comment
Hide comment
@artagnon

artagnon Nov 21, 2014

Contributor

Issues fixed now. I'm a little worried about the primitive_ffi_types -- if we put NULL for bool, will an llvm i1 passed via libffi be slurped up correctly on the OCaml side?

Contributor

artagnon commented Nov 21, 2014

Issues fixed now. I'm a little worried about the primitive_ffi_types -- if we put NULL for bool, will an llvm i1 passed via libffi be slurped up correctly on the OCaml side?

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 22, 2014

Member

Thanks, @artagnon. There's a test still failing; it looks like it just needs Boolremoved.

I'm not very familiar with the details of LLVM. Does it give some guarantees about how i1 is represented? If it's equivalent to C99's _Bool type then we can't reliably pass i1 values using libffi, at least without a configuration step to detect how _Bool is represented on each platform. On the other hand, if i1 is essentially an alias for int8_t then it might be reasonable to use a ctypes view to make i8 values look like bools on the OCaml side but int8_t values on the C side.

Defining a view requires conversion functions in both directions:

# let bool_of_int i = i <> 0 and int_of_bool b = if b then 1 else 0;;
val bool_of_int : int -> bool = <fun>
val int_of_bool : bool -> int = <fun>
# let i1 = view int8_t ~read:bool_of_int ~write:int_of_bool;;
val i1 : bool typ = int8_t

The top-level printer gives a clue to what's going on: the type of i1 (i.e. bool) is the OCaml type used to read and write values using the view, and the pretty-printed value (int8_t) shows the C representation type. Here's another example:

# i1 @-> i1 @-> returning void;;
- : (bool -> bool -> unit) fn = void(int8_t, int8_t)

That is, describing a function using the i1 view gives you a function of type bool -> bool -> unit on the OCaml side that calls a C function at type void(int8_t, int8_t) when invoked.

The view implementation doesn't need any changes to ctypes itself, since it only uses functions in the public API. I'd like to go ahead and merge this pull request, since having a _Bool type is independently useful.

Member

yallop commented Nov 22, 2014

Thanks, @artagnon. There's a test still failing; it looks like it just needs Boolremoved.

I'm not very familiar with the details of LLVM. Does it give some guarantees about how i1 is represented? If it's equivalent to C99's _Bool type then we can't reliably pass i1 values using libffi, at least without a configuration step to detect how _Bool is represented on each platform. On the other hand, if i1 is essentially an alias for int8_t then it might be reasonable to use a ctypes view to make i8 values look like bools on the OCaml side but int8_t values on the C side.

Defining a view requires conversion functions in both directions:

# let bool_of_int i = i <> 0 and int_of_bool b = if b then 1 else 0;;
val bool_of_int : int -> bool = <fun>
val int_of_bool : bool -> int = <fun>
# let i1 = view int8_t ~read:bool_of_int ~write:int_of_bool;;
val i1 : bool typ = int8_t

The top-level printer gives a clue to what's going on: the type of i1 (i.e. bool) is the OCaml type used to read and write values using the view, and the pretty-printed value (int8_t) shows the C representation type. Here's another example:

# i1 @-> i1 @-> returning void;;
- : (bool -> bool -> unit) fn = void(int8_t, int8_t)

That is, describing a function using the i1 view gives you a function of type bool -> bool -> unit on the OCaml side that calls a C function at type void(int8_t, int8_t) when invoked.

The view implementation doesn't need any changes to ctypes itself, since it only uses functions in the public API. I'd like to go ahead and merge this pull request, since having a _Bool type is independently useful.

Add bool type corresponding to the llvm i1/ C99 bool
The idea is to find a way to represet the llvm i1 type, since llvm
ExecutionEngine also uses libffi now. It is equivalent to the C99
bool. We use the OCaml native bool plus Val_bool/ Bool_val. The actual
memory representation of the i1 wastes 7 bits, since one quantum of
memory is 8 bits. This patch is mostly a nice-to-have, since we can
always represent the data as a char and coerce it into a bool
using (bool_of_int (Char.code (getf value lang_bool))) in the user
program.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
@artagnon

This comment has been minimized.

Show comment
Hide comment
@artagnon

artagnon Nov 22, 2014

Contributor

Sorry about that: fixed the tests now. You can merge it, but the commit message may be incorrect.

I'll do some research on how llvm represents i1 and get back to you. I assumed that C99 bool = llvm i1 = uint8; the language uses this type information to say what operation is valid and what isn't. I mean what other way is there to represent 1 bit? You essentially waste 7 bits and put it in a byte, right?

Contributor

artagnon commented Nov 22, 2014

Sorry about that: fixed the tests now. You can merge it, but the commit message may be incorrect.

I'll do some research on how llvm represents i1 and get back to you. I assumed that C99 bool = llvm i1 = uint8; the language uses this type information to say what operation is valid and what isn't. I mean what other way is there to represent 1 bit? You essentially waste 7 bits and put it in a byte, right?

yallop added a commit that referenced this pull request Nov 25, 2014

Add bool type corresponding to the C99 _Bool/bool.
[jdy: edited commit message to reflect the apparent distinction
 between C99 _Bool and LLVM i1.  See the discussion at:
   #220
 Original commit message below.]

Add bool type corresponding to the llvm i1/ C99 bool

The idea is to find a way to represet the llvm i1 type, since llvm
ExecutionEngine also uses libffi now. It is equivalent to the C99
bool. We use the OCaml native bool plus Val_bool/ Bool_val. The actual
memory representation of the i1 wastes 7 bits, since one quantum of
memory is 8 bits. This patch is mostly a nice-to-have, since we can
always represent the data as a char and coerce it into a bool
using (bool_of_int (Char.code (getf value lang_bool))) in the user
program.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 25, 2014

Member

I've merged the changeset and added a note to the commit message: d662070. Thanks for your contribution!

Regarding possible ways to represent 1 bit: implementations are obliged to waste at least 7 bits, since bools must be addressable, but I understand that they're at liberty to waste even more. I don't see anything in the standard that requires sizeof(_Bool) to be 1. I'd be happy to be mistaken on this point, though.

Member

yallop commented Nov 25, 2014

I've merged the changeset and added a note to the commit message: d662070. Thanks for your contribution!

Regarding possible ways to represent 1 bit: implementations are obliged to waste at least 7 bits, since bools must be addressable, but I understand that they're at liberty to waste even more. I don't see anything in the standard that requires sizeof(_Bool) to be 1. I'd be happy to be mistaken on this point, though.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 26, 2014

Member

I've added support for passing bools via libffi/Foreign in #229.

Member

yallop commented Nov 26, 2014

I've added support for passing bools via libffi/Foreign in #229.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 27, 2014

Member

Closing, since the patch is merged. Further pull requests are welcome if there's more needed to support LLVM's i1.

Member

yallop commented Nov 27, 2014

Closing, since the patch is merged. Further pull requests are welcome if there's more needed to support LLVM's i1.

@yallop yallop closed this Nov 27, 2014

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