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
Comments
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. |
Comment author: @lpw25
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. |
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? |
Comment author: @yallop The translation of class initialization code can involve sequences on the rhs of a let-rec: $ ocaml -dlambda 0a module M(X:sig end) = struct class c = object end end;;(apply (field 1 (global Toploop!)) "M/1023" 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. |
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)? |
Comment author: @alainfrisch I bump the Severity field: the compiler generates segfaulting code! |
Comment author: @alainfrisch Same for: |
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. |
Comment author: @alainfrisch Improved fix: commit fa743fd. |
Comment author: @alainfrisch Note, the commit rejects the following forms: let rec x = [| x |]; 1. in () but according to the manual, they should be accepted. I'm not sure it is worth spending time allowing them, though. |
Comment author: @xavierleroy Thanks for the fix. I'm OK with rejecting these borderline cases. |
Not clear where the reference to ocaml/ocaml#6939 comes from, as that issue is about unboxed float arrays. See https://forum.rescript-lang.org/t/recursive-type-usage-problem/3717
Not clear where the reference to ocaml/ocaml#6939 comes from, as that issue is about unboxed float arrays. See https://forum.rescript-lang.org/t/recursive-type-usage-problem/3717
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.
The text was updated successfully, but these errors were encountered: