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

Segfault with improper use of let-rec #6939

Closed
vicuna opened this issue Jul 27, 2015 · 11 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Jul 27, 2015

Original bug ID: 6939
Reporter: @alainfrisch
Status: closed (set by @xavierleroy on 2017-02-16T14:18:21Z)
Resolution: fixed
Priority: normal
Severity: crash
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: ~DO NOT USE (was: OCaml general)
Related to: #5476 #6738
Monitored by: @gasche @diml @yallop

Bug description

let rec x = [| x |]; 1. in ()

is current accepted and compiled to this cmm code:

(function camlFoo__entry ()
(let
(x/1203 0
x/1204
(seq (alloc 1278 (load float64u x/1203)) []
(load float64u "camlFoo__1")))
[])
1a)

which segfaults.

I'm in favor of rejecting such let-rec declarations instead of trying to fix the code generator.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

Comment author: @lpw25

That code appears to obey the "statically constructive" criteria, and the problem comes from the float array hack forcing array construction to actually destruct the argument.

Without removing the array hack or breaking the statically constructive criteria, it seems that the only solution is to specialise the treatment of dummy values which are known to be floats.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

Comment author: @lpw25

the problem comes from the float array hack forcing array construction to actually destruct the argument

Actually, to be fair to the float array hack, this problem happens with any unboxed representation. For example, with float records:

type foo = { x : float };;

type foo = { x : float; }

let rec x = {x}; 1.0;;

Process ocaml-toplevel segmentation fault

In the future types other than float may have unboxed representations, so special casing dummy float values may not be the best approach.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

Comment author: @alainfrisch

Ah, too bad, I would have loved this to be (yet another) argument against unboxed float arrays.

Seriously, is there any useful use of a sequence on the rhs of a let-rec binding?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

Comment author: @yallop

The translation of class initialization code can involve sequences on the rhs of a let-rec:

$ ocaml -dlambda
OCaml version 4.02.2

0a
0a

module M(X:sig end) = struct class c = object end end;;

(apply (field 1 (global Toploop!)) "M/1023"
(let (c_tables/1026 =a (makemutable 0 0a 0a 0a))
(function X/1016
(letrec
(c/1020
(seq
(if (field 0 c_tables/1026) 0a
(let
(c_init/1035 =
(function class/1030
(function env/1032
(function self/1031
(apply (field 23 (global CamlinternalOO!))
self/1031 class/1030)))))
(apply (field 19 (global CamlinternalOO!)) 0a c_init/1035
c_tables/1026)))
(makeblock 0 (apply (field 0 c_tables/1026) 0a)
(field 1 c_tables/1026) (field 0 c_tables/1026) 0a)))
(makeblock 0 c/1020)))))
module M : functor (X : sig end) -> sig class c : object end end

However, that's not really a good justification for supporting it in the source language. In fact, this seems like another good reason to move the let-rec well-formedness check to the type-checking phase (PR6738), where it can be performed on the typed tree instead of lambda.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

Comment author: @alainfrisch

The extra restriction I had in mind would be to reject any reference to the identifiers being bound on the lhs of a sequence. Could such a case be generated by the class initialization code (it's not the case in your example)?

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 2, 2015

Comment author: @alainfrisch

I bump the Severity field: the compiler generates segfaulting code!

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 2, 2015

Comment author: @alainfrisch

Same for:
let rec x = let u = [|y|] in 10. and y = 1.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 3, 2015

Comment author: @alainfrisch

Fix committed fab5144.

Keeping it open: it's probably worth reviewing (and documenting / changing) the current criteria in Translcore.check_recursive_lambda.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 3, 2015

Comment author: @alainfrisch

Improved fix: commit fa743fd.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 7, 2015

Comment author: @alainfrisch

Note, the commit rejects the following forms:

let rec x = [| x |]; 1. in ()
let rec x = let u = [|y|] in 10. and y = 1.

but according to the manual, they should be accepted. I'm not sure it is worth spending time allowing them, though.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 9, 2015

Comment author: @xavierleroy

Thanks for the fix. I'm OK with rejecting these borderline cases.

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