Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion ml-proto/host/builtins.ml
Original file line number Diff line number Diff line change
@@ -1,9 +1,50 @@
open Source
open Eval
Copy link
Member

Choose a reason for hiding this comment

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

From a readthrough, the only use of the Eval module in this file is the Eval.memory_for_module. Can this open be removed?

open Memory
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this open may be similarly unneeded.

open Types
Copy link
Member

Choose a reason for hiding this comment

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

(Sorry for nit-picking these open directives; my experience with OCaml so far has been that having lots of opens leads to increased chances of confusion from name collisions or struct type literal inferences). Is this open needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. How do the rules work?

Copy link
Member

Choose a reason for hiding this comment

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

I assume it's just: if you're not using any of the things in the module, you don't need the open. I was hoping it would be just a matter of deleting the open, seeing if everything still compiles, and if so, go with it.

Copy link
Member

Choose a reason for hiding this comment

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

A good rule is to limit open to a small number of imports that are heavily used, e.g. those that define datatypes with many constructors (like Ast), but is best avoided in most other cases, because it quickly harms readability. (In general, I feel that we have already started overusing it.)


let print vs =
let print m vs =
List.iter Print.print_value (List.map (fun v -> Some v) vs);
None

let rec stdout_write_inner at mem offset count i =
let load_result = Memory.load_extend mem (Int64.add offset i) Mem8 ZX Int32Type in

begin
match load_result with
| Values.Int32 byte ->
print_char (Char.chr (Int32.to_int byte))
| _ ->
ignore (Error.error at "load_extend returned wrong type")
Copy link
Member

Choose a reason for hiding this comment

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

Would this be a use case where an assert would be appropriate? The type will only be wrong if Memory.load_extend produces a value with the wrong type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dunno. Don't know anything about assert.

end;

let next = Int64.succ i in
let should_iterate = (Int64.compare next count) < 0 in
Copy link
Member

Choose a reason for hiding this comment

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

Is this equivalent to using plain < on int64 types, which we use elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried < before it wasn't working, but I had a bunch of strange problems with this code. I'll see if < works.


if should_iterate then
ignore (stdout_write_inner at mem offset count next)
else
();
Copy link
Member

Choose a reason for hiding this comment

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

If I understand what's going on here, the else () here is redundant; is that right, or am I missing something subtle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was getting type errors before, but it might have been because I had a diagnostic print in there. It'll probably work if I remove the else.


and stdout_write at m vs =
if List.length vs != 2 then
Error.error at "stdio.write expects 2 arguments (offset, count)";

let mem = Eval.memory_for_module at m in
match vs with
| [Values.Int32 _offset; Values.Int32 _count] ->
let offset = (Int64.of_int32 _offset) in
let count = (Int64.of_int32 _count) in

set_binary_mode_out stdout false;
Copy link
Member

Choose a reason for hiding this comment

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

As you mentioned elsewhere, this API does the opposite of what it first looks like. Would you mind adding a comment about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

ignore (stdout_write_inner at mem offset count Int64.zero);
set_binary_mode_out stdout true;
None

| _ ->
ignore (Error.error at "stdio.write expected i32 offset, i32 count");
None

let match_import i =
let {Ast.module_name; func_name; func_params; func_result} = i.it in
if module_name <> "stdio" then
Expand All @@ -13,6 +54,10 @@ let match_import i =
if func_result <> None then
Error.error i.at "stdio.print has no result";
print
| "write" ->
if func_result <> None then
Error.error i.at "stdio.write has no result";
stdout_write i.at
| _ ->
Error.error i.at ("no stdio." ^ func_name ^ "\"")

Expand Down
13 changes: 10 additions & 3 deletions ml-proto/spec/eval.ml
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ let error = Error.error

type value = Values.value
type func = Ast.func
type import = value list -> value option
type host_params = {page_size : Memory.size}

module ExportMap = Map.Make(String)
type export_map = func ExportMap.t

type instance =
type import = instance -> value list -> value option
and instance =
{
funcs : func list;
imports : import list;
Expand Down Expand Up @@ -159,7 +159,7 @@ let rec eval_expr (c : config) (e : expr) =

| CallImport (x, es) ->
let vs = List.map (fun ev -> some (eval_expr c ev) ev.at) es in
(import c x) vs
(import c x) c.module_ vs

| CallIndirect (x, e1, es) ->
let i = int32 (eval_expr c e1) e1.at in
Expand Down Expand Up @@ -312,3 +312,10 @@ let host_eval e =
let host = {page_size = 1L} in
let m = {imports = []; exports; tables = []; funcs = [f]; memory = None; host} in
eval_func m f []

let memory_for_module at m =
match m.memory with
| Some mem ->
mem
| _ ->
error at "Module has no memory";
5 changes: 3 additions & 2 deletions ml-proto/spec/eval.mli
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@

type instance
type value = Values.value
type import = value list -> value option
type import = instance -> value list -> value option
type host_params = {page_size : Memory.size}

val init : Ast.module_ -> import list -> host_params -> instance
val invoke : instance -> string -> value list -> value option
(* raise Error.Error *)

(* This function is not part of the spec. *)
(* These functions are not part of the spec. *)
val host_eval : Ast.expr -> value option (* raise Error.Error *)
val memory_for_module : Source.region -> instance -> Memory.t
Binary file not shown.
13 changes: 13 additions & 0 deletions ml-proto/test/stdio_write.wast
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
(module
(import $write "stdio" "write" (param i32 i32))

(memory 4096 4096 (segment 0 "\89\50\4e\47\0d\0a\1a\0a\00"))

(func $write_png_header
(call_import $write (i32.const 0) (i32.const 9))
)

(export "write_png_header" $write_png_header)
)

(invoke "write_png_header")