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

CamlinternalLazy not flambda-proof? #7292

Closed
vicuna opened this Issue Jul 18, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Jul 18, 2016

Original bug ID: 7292
Reporter: @alainfrisch
Assigned to: @mshinwell
Status: closed (set by @xavierleroy on 2017-09-24T15:33:15Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.04.0 +dev / +beta1 / +beta2
Fixed in version: 4.04.0 +dev / +beta1 / +beta2
Category: back end (clambda to assembly)
Related to: #7301
Monitored by: braibant

Bug description

Compiling the following:

let _ = Lazy.force (lazy (fun _ -> assert false))

with flambda and O3 prints the following error message 9 times:

File "camlinternalLazy.ml", line 0, characters 0-0:
Warning 59: A potential assignment to a non-mutable value was detected
in this source file.  Such assignments may generate incorrect code
when using Flambda.

(Note that the error message refers to camlinternalLazy.ml while we are compiling another unit.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 18, 2016

Comment author: @alainfrisch

For now, we compile with this local change:

Index: inline_and_simplify.ml
===================================================================
--- inline_and_simplify.ml      (revision 95085)
+++ inline_and_simplify.ml      (working copy)
@@ -1017,6 +1017,7 @@
       | (Psetfield _ | Parraysetu _ | Parraysets _),
           _block::_, block_approx::_ ->
         if A.is_definitely_immutable block_approx then begin
+          if dbg.Debuginfo.dinfo_file <> "camlinternalLazy.ml" then (* LEXIFI, see OCaml #7292 *)
           Location.prerr_warning (Debuginfo.to_location dbg)
             Warnings.Assignment_to_non_mutable_value
         end;

I don't know if this is safe (i.e. if the "unsafe set_field" in camlinternalLazy cannot break anything), but it is better than disabling that warning globally. (Due to our local "lazy let rec" construction, we end up with a lot of "lazy functions".)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 20, 2016

Comment author: @alainfrisch

We get a segfault on a piece of code with flambda -O3; this code uses Lazy and the segfault disappears with simple changes such as (lazy x --> Lazy.from_val x) (where x is an identifier). So I'm wondering if the warning reported in this PR could be related. Could it be that "unsafe" mutations done in camlinternalLazy are really unsafe with flambda -O3?

More details: the 'x' refers in fact to a function, but hidden behind an abstract type. So for "lazy x", Translcore produces a forward block (because it doesn't know the definition of the abstract type and if it were "float", it could be compile the expression as "x"). For "Lazy.from_val x", flambda inlines the called function; since it knows that "x" refers to a closure, this removes the tag check in Lazy.from_val, and this produces simply a reference to "x".

(Optimization note: Perhaps in flambda mode, Translcore should compile "lazy x" to "Lazy.from_val x" instead of producing the forward block itself, in order to benefit from further optimization. Alternatively flambda should optimize away forward blocks pointing to values of non-float types.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 21, 2016

Comment author: @alainfrisch

Segfaulting program reduced and reported in #7301.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 25, 2016

Comment author: @mshinwell

I am looking at this

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 25, 2016

Comment author: @mshinwell

Please see #713

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 31, 2016

Comment author: @alainfrisch

And #714

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Aug 1, 2016

Comment author: @mshinwell

Fixed for 4.04

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.