Skip to content
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

Fix code generation with nested let rec of functions #8712

Merged
merged 6 commits into from Jun 5, 2019

Conversation

@stedolan
Copy link
Contributor

commented Jun 4, 2019

Here's another go at fixing #8681 (and #8699, which turned out to be the same bug), by making Cmmgen.expr_size and Bytegen.size_of_lambda correctly compute the size of blocks containing Infix_tag, and making the runtime caml_alloc/update_dummy functions correctly handle Infix_tag allocations. These functions previously gave incorrect answers in the presence of Infix_tag.

This PR contains no type system changes - the check for which recursive definitions are allowed is unchanged.

@gasche, @chambart: I've added both of you as authors of this to the Changes file, since I borrowed your test cases. I don't think this should mean you can't review, though!

Copy link
Member

left a comment

The general approach is nice, thank you.

If we hadn't found correctness issues in real-world code due to this interaction, I think people would have recommend to wait until some of the better proposals come to existence (a compilation pass in lambda-code, or a removal of Infix pointers, or both), but now there is some urgency and I think that the proposed approach is both good enough and simple enough to be desirable.

The general idea is to have a new caml_alloc_dummy_infix function that initializes the dummy value with an Infix tag, and then have the existing caml_update_dummy function recognize the infix tag and do the right thing in that case. (No need for a new update_dummy_infix function with that approach.)

I wonder if there is a risk of caml_update_dummy being called several times on the same dummy block, with different or identical offsets. There is no provision against double-initializations in your code, so I guess that they could happen (and the final result would be fine), but I'm not sure it's good to have them -- I prefer to think of values as going from unitialized to initialized, and then not being written to anymore. Can those really happen? What should we do to avoid them?

runtime/alloc.c Outdated Show resolved Hide resolved
runtime/alloc.c Outdated Show resolved Hide resolved
asmcomp/cmmgen.ml Outdated Show resolved Hide resolved
@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I wonder if there is a risk of caml_update_dummy being called several times on the same dummy block, with different or identical offsets. There is no provision against double-initializations in your code, so I guess that they could happen (and the final result would be fine), but I'm not sure it's good to have them -- I prefer to think of values as going from unitialized to initialized, and then not being written to anymore. Can those really happen? What should we do to avoid them?

I agree double-initialisations are bad, but I'm fairly sure they can't happen here. There's exactly one caml_update_dummy call generated for each call to caml_alloc_dummy*, by the code in Bytegen.comp_expr / Cmmgen.transl_letrec.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

I would need to look at generated code to get a better sense of what happens in some cases. I agree with your idea that there is a single update_dummy per alloc_dummy function, but I still suspect there may be something fishy here. Could it be the case, instead, that a single shared block gets duplicated by being patched into several dummy blocks? Is there any problem with that? (This could be very strange, for example, if we had mutable closure fields, but I don't think this is the case today.)

To get a feeling for what the generated code looks like in these situations, I wrote the following program:

let bar =
  let rec f = function
    | 0 -> 0
    | n -> g (n - 1)
  and g = function
    | 0 -> f 10
    | n -> f (n - 1)
  in
  let rec foof = f
  and goof = g
  in (foof, goof)

but native-compiling this in your branch current crashes with:

Fatal error: exception File "asmcomp/cmmgen.ml", line 947, characters 13-19: Assertion failed
@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Nice find, thanks! I've added your example as a test.

My assumption was that Uoffset (selecting a particular Infix_tag object from within a closure block) can only be applied to a mutually recursive block of functions being defined. This is incorrect: it can also be applied to a previously-defined mutually recursive block of functions, not part of the current let rec.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Could it be the case, instead, that a single shared block gets duplicated by being patched into several dummy blocks? Is there any problem with that? (This could be very strange, for example, if we had mutable closure fields, but I don't think this is the case today.)

Thanks, I understand this concern better now. This can in fact happen, and it seems it could happen before this patch. Here's some exciting code:

# type t = { mutable a : t; mutable b : t };;
type t = { mutable a : t; mutable b : t; }
# let rec r = let rec x = { a = r; b = x } in x;;
val r : t = {a = <cycle>; b = {a = <cycle>; b = <cycle>}}
# r == r.a;;
- : bool = true
# r == r.b;;
- : bool = false
@gasche
gasche approved these changes Jun 4, 2019
Copy link
Member

left a comment

I have reviewed this patch and believe it is correct. I think we should merge it, because it fixes real wrong-code generation issues, and backport it in the 4.09 release branch.

There is some design space in the runtime support. The current proposal uses a new infix-aware dummy-alloc primitive, and makes the existing dummy-update primitive infix-aware. We could, alternatively:

  1. have used a new dummy-update primitive just for infix, without modifying the existing dummy-update primitive -- except to assert that neither the new value nor the dummy value have infix tags.
  2. have kept a single non-infix-aware dummy-alloc primitive: at update time, it is enough to detect that the new value has an infix tag to perform the infix-aware update.

Note that the runtime support, already in the proposed patch and "even more" with alternative (2) above, relies on the invariant that caml_update_dummy will encounter an Infix value only if that value is part of the current value-letrec block (so it must be a local shared closure of one of the recursive members). This invariant comes from the compilation strategy, but it is subtle and not clearly specified there. I believe that it holds due to the way letrec compilation is always initiated in an empty environment -- letrec bindings "from before" are always assigned size RHS_nonrec, and thus do not result in dummy creation and update.

runtime/alloc.c Show resolved Hide resolved
@damiendoligez damiendoligez added this to the 4.09 milestone Jun 4, 2019
(match expr_size env exp with
| RHS_block blocksize -> RHS_infix { blocksize; offset }
| RHS_nonrec -> RHS_nonrec
| _ -> assert false)

This comment has been minimized.

Copy link
@chambart

chambart Jun 5, 2019

Contributor

You could also handle
RHS_infix { blocksize; offset = offset' } -> RHS_infix { blocksize; offset = offset + offset' }
I'm not certain that I can produce an example where Uoffsets are chained, but in theory, flambda could produce it through convoluted sequence of unrolling.

@gasche gasche merged commit a8b29db into ocaml:trunk Jun 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gasche added a commit that referenced this pull request Jun 5, 2019
Fix code generation with nested let rec of functions

(cherry picked from commit a8b29db)
@yallop

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Here's some exciting code

I haven't looked into this, but it seems worth noting that this is a recent change: the exciting code was rejected pre-4.06.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.