Organise and simplify translation of primitives by lpw25 · Pull Request #1557 · ocaml/ocaml

lpw25 · 2018-01-03T15:46:22Z

Currently the translation of builtin externals in bytecomp is pretty disorganized. It is handled across multiple parts of translcore.ml and different builtins are handled in different places. This has lead to a number of silly little bugs, e.g. this one or this bug that is still on trunk:

# module M : sig
    val send : CamlinternalOO.obj -> CamlinternalOO.tag -> CamlinternalOO.t
  end = CamlinternalOO ;;
    Characters -1--1:
  module M : sig
  
Error: The external function `%send' is not available

This PR tries to improve the situation. It moves all the handling for primitives into a new translprim.ml file, and uses an internal type:

type prim =
  | Primitive of Lambda.primitive
  | Comparison of comparison * comparison_kind
  | Raise of Lambda.raise_kind
  | Raise_with_backtrace
  | Lazy_force
  | Loc of loc_kind
  | Send
  | Send_self
  | Send_cache

to represent the various builtin externals. The cases other than Primitive are the ones that require special handling. These have previously been handled in different ad-hoc ways but here are handled by a fairly uniform set of functions in translprim.ml.

gasche · 2018-01-03T15:58:27Z

That sounds like an excellent idea to me, although I still have to look at what you have done.

cc @bobot @nojb, who have been working on #1465

xclerc · 2018-01-03T16:48:31Z

I am unsure whether it should be done in this PR, but some cases of
Semantics_of_primitives.for_primitive seem to be wrong.
For instance, caml_format_{float,int}/caml_{int32,int64,nativeint}_fomat
do allocate. Similarly, reading from a big array might allocate, depending on
its kind.

bobot

It is a very good improvement. Some remarks from the point of view of someone who try to add a new primitive.

bobot · 2018-01-15T12:59:22Z

bytecomp/translprim.ml

+  | Send_cache, [obj; meth; cache; pos] ->
+      Lsend(Cached, meth, obj, [cache; pos], loc)
+  | Send_cache, _ ->
+      raise(Error(loc, Wrong_arity_builtin_primitive prim_name))


bobot · 2018-01-15T13:03:03Z

bytecomp/translprim.ml

+(**************************************************************************)
+
+(* Translation of primitives *)
+


Could you add some guide of how to add a new primitive? Something simple perhaps like:

add your primitive to prim or one of the sub-datatype, then to primitives_table, finally let the typer guide you to the missing cases.

I decided against adding a comment. I don't think it makes adding a new primitive noticeably easier and these kinds of comments tend not to get updated when things change, leaving things in a worse state than if they weren't there.

bobot · 2018-01-15T13:03:58Z

bytecomp/translprim.ml

+
+exception Error of Location.t * error
+
+(* Insertion of debugging events *)


It is not clear when and where to add debugging events, could you point to the documentation?

I don't think there is documentation, nor do I know the rules and invariants. I tried simplifying things when writing this patch, but it made some tests around backtraces fail, so instead I was just careful to keep the behaviour the same.

hhugo · 2018-04-02T16:56:10Z

@damiendoligez, can we consider this one for 4.07 or at least merge #1465

hhugo · 2018-04-06T20:13:17Z

@damiendoligez, @lpw25, can we have this PR in 4.07 ?

nojb · 2018-04-07T08:17:00Z

I have been studying this PR and will post a review at some point today.

nojb

I reviewed this PR.

I think it is a great improvement on the current state of affairs. I read the code carefully and am confident it is correct (modulo a small issue with the handling of the raise primitive). More importantly, I am confident that if there is a bug, it will be easy to find and fix thanks to the new code organization. This is in contrast with the current state of affairs where the handling of primitives is spread all over the place and many invariants are not tracked by the type system.

Some remarks about the code itself.

While the diff is large, a lot of it is due to moving code around and "type-directed" refactoring.

Conceptually, the PR does several things:

merge the code used by transl_primitive (which eta-expands a primitive) and transl_primitive_application (which translates a fully applied primitive). This cuts down on a lot of duplicated code, and fixes at least MPR#7674 and MPR#7666.
A more robust function to lookup primitives lookup_primitive instead of find_primitive. This would have produced a sensible error message for MPR#7666.
Clean up the insertion of debug events in the generated lambda code (functions wrap and wrap0). The current code here is so convoluted that it took me some time to convince myself that the new code (which is simpler) is actually equivalent.
Introduce a new type Translprim.prim which includes the usual Lambda.primitive but also other primitives that do not currently have a representation in the type system and are only recognized by their names. The "new" primitives are: comparison primitives, send, sendself, sendcache, and raise_with_backtrace. Having constructors corresponding to these primitives allows to use pattern matching and, in the case of comparisons, to handle them before specialization.
Some primitives that need special handling are lifted out of the Lambda.primitive type into the new Translprim.prim type. On the one hand pattern matching makes it obvious where special handling is needed. On the other hand, it also makes it clear that these primitives do not survive lambda-generation. The primitives affected are: raise, lazyforce, and loc_* primitives.

In an ideal world, having the PR split into smaller commits would make it easier to digest, but personally I don't feel it is reason enough to delay its integration.

nojb · 2018-04-07T07:26:20Z

bytecomp/translprim.ml

+  | Compare, Compare_int32s -> Pccall caml_int32_compare
+  | Compare, Compare_int64s -> Pccall caml_int64_compare
+
+let lamda_of_loc kind loc =


s/lamda/lambda/

nojb · 2018-04-07T07:32:13Z

bytecomp/translprim.ml

+  let prim = lookup_primitive loc p env path in
+  let has_constant_constructor = false in
+  let prim =
+    match specialize_primitive env ty ~has_constant_constructor prim with


Why not have specialize_primitive return prim instead of prim option?

It is needed for implementing #1566.

nojb · 2018-04-07T12:08:44Z

bytecomp/translprim.ml

+        | Raise_regular, Lvar argv when Hashtbl.mem try_ids argv ->
+            Raise_reraise
+        | _, _ ->
+            Raise_regular


Should probably be kind instead of Raise_regular, to avoid changing Raise_notrace case.

Good catch. Fixed.

lpw25 · 2018-04-09T12:03:08Z

I've rebased, addressed comments and added a changes entry.

lpw25 · 2018-04-09T12:48:46Z

The CI failure is unrelated to this patch (and has now been fixed on trunk).

shindere · 2018-04-09T13:19:51Z

Inria's CI fails on ARM.
File "translprim.mli", line 18, characters 19-39:
Error: Unbound module Typedtree

Is this just a matter of running make alldepend?

lpw25 · 2018-04-09T13:33:46Z

I ran make depend, and I can see the line:

bytecomp/translprim.cmi : typing/types.cmi typing/typedtree.cmi

in the commits, so this is a surprising error.

lpw25 · 2018-04-09T13:46:06Z

Ah, there is aparently now a ocamldoc/stdlib_non_prefixed/.depend file that also needs updating, and gets updated by make alldepend. Mark has pushed the fix to trunk.

shindere · 2018-04-09T13:52:16Z

Leo White (2018/04/09 13:46 +0000):

Ah, there is aparently now a `ocamldoc/stdlib_non_prefixed/.depend` file that also needs updating, and gets updated by `make alldepend`. Mark has pushed the fix to trunk.

Thanks guys!

hhugo · 2018-04-09T15:10:39Z

Thanks

nojb mentioned this pull request Jan 4, 2018

Fix handling of Printexc.raise_with_backtrace under partial application #1465

Closed

lpw25 mentioned this pull request Jan 12, 2018

Remove primitive coercions #1566

Closed

bobot reviewed Jan 15, 2018

View reviewed changes

bobot mentioned this pull request Apr 6, 2018

Print backtrace in case of uncaught exception ocaml/dune#679

Merged

nojb requested changes Apr 7, 2018

View reviewed changes

nojb added the high-priority label Apr 9, 2018

Organise and simplify translation of primitives

656aa42

lpw25 force-pushed the tidy-primitives branch from 6284170 to 656aa42 Compare April 9, 2018 12:00

Add Changes entry

5870c99

nojb approved these changes Apr 9, 2018

View reviewed changes

lpw25 merged commit a7ee6a0 into ocaml:trunk Apr 9, 2018

hhugo mentioned this pull request Apr 9, 2018

Printexc.raise_with_backtrace not supported ocsigen/js_of_ocaml#619

Closed

lpw25 mentioned this pull request Apr 10, 2018

Fix location primitives (e.g. __FILE__) #1715

Merged

This was referenced Mar 14, 2019

miscompilation of the "%raise_with_backtrace" primitive. #7666

Closed

Segmentation fault when passing extra arguments to Printexc.raise_with_partial_backtrace #7674

Closed

incorrect arity on primitives causes assertion failure #7846

Closed

		(**************************************************************************)

		(* Translation of primitives *)


		exception Error of Location.t * error

		(* Insertion of debugging events *)

Conversation

lpw25 commented Jan 3, 2018

Uh oh!

gasche commented Jan 3, 2018

Uh oh!

xclerc commented Jan 3, 2018

Uh oh!

bobot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobot Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpw25 Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhugo commented Apr 2, 2018

Uh oh!

hhugo commented Apr 6, 2018

Uh oh!

nojb commented Apr 7, 2018

Uh oh!

nojb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpw25 commented Apr 9, 2018

Uh oh!

lpw25 commented Apr 9, 2018

Uh oh!

shindere commented Apr 9, 2018

Uh oh!

lpw25 commented Apr 9, 2018

Uh oh!

lpw25 commented Apr 9, 2018

Uh oh!

shindere commented Apr 9, 2018 via email

Uh oh!

hhugo commented Apr 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bobot left a comment •

edited

Loading

bobot Jan 15, 2018 •

edited

Loading

lpw25 Apr 9, 2018 •

edited

Loading