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

"module type of" + recursive modules + functors segfault in bytecode interpreter (4.07 regression) #7787

Closed
vicuna opened this issue Apr 27, 2018 · 12 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Apr 27, 2018

Original bug ID: 7787
Reporter: @mshinwell
Assigned to: @trefis
Status: resolved (set by @trefis on 2018-05-01T08:47:36Z)
Resolution: fixed
Priority: normal
Severity: major
Version: 4.07.0+dev/beta2/rc1/rc2
Target version: 4.07.0+dev/beta2/rc1/rc2
Category: middle end (typedtree to clambda)
Monitored by: @hhugo

Bug description

In 4.07 the following code causes a segfault in the bytecode interpreter, trying to read a field of the unit value, unless the "remove_aliases" attribute is added. This appears to work with 4.06.

Thanks to hheuzard and xclerc for help investigating this.

module O (T : sig
module N : sig
val foo : int -> int
end
end) = struct
open T

let go () =
N.foo 42 (* finding N (from T) goes wrong *)
end

module T = struct
module N = struct
let foo x = x + 3
end
end;;

(* Incidentally, M isn't used in T2, but it doesn't seem to fail if
it's just "module M" and "module T2" separately )
module rec M : sig
val go : unit -> int
end = O (T2)
and T2 : sig
include module type of struct include T end (
[@remove_aliases] *)
end = struct
include T
end;;

let _ = ignore (M.go ())

Steps to reproduce

Compile the above program with ocamlc and run it.

File attachments

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @mshinwell

The lambda code (for the version without the attribute) is:

(setglobal Test!
(let
(O/1002 =
(module-defn(O/1002) test.ml(1):0-157
(function T/1054 is_a_functor
(let
(go/1006 =
(function param/1008 (apply (field 0 (field 0 T/1054)) 42)))
(makeblock 0 go/1006))))
T/1009 =
(module-defn(T/1009) test.ml(12):159-228
(let
(N/1010 =
(module-defn(N/1010) test.ml(13):179-224
(let (foo/1011 = (function x/1012 (+ x/1012 3)))
(makeblock 0 foo/1011))))
(makeblock 0 N/1010)))
M/1013 =
(apply (field 0 (global CamlinternalMod!)) [0: "test.ml" 22 6]
[0: [0: 0]])
T2/1014 =
(apply (field 0 (global CamlinternalMod!)) [0: "test.ml" 25 6]
[0: [0: [1: 0]]]))
(seq
(apply (field 1 (global CamlinternalMod!)) [0: [0: 0]] M/1013
(module-defn(M/1013) test.ml(20):354-408 (apply O/1002 T2/1014)))
(apply (field 1 (global CamlinternalMod!)) [0: [0: [1: 0]]] T2/1014
(module-defn(T2/1014) test.ml(23):409-520
(makeblock 0 (field 0 T/1009))))
(ignore (apply (field 0 M/1013) 0))
(makeblock 0 O/1002 T/1009 M/1013 T2/1014))))

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @mshinwell

trefis confirms this segfaults with ocamlopt too.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @gasche

This looks like a bug for Leo, related to #1652

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @trefis

I'm wondering if #1610 might not have a positive impact here.
I'm currently in the middle of rebasing it and I'll report back once I have tried it out.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @trefis

Confirmed: #1610 fixes the issue.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @gasche

trefis: given that #1610 has a large conflict with the current trunk, maybe it would be useful to push your rebase somewhere for easier testing? (Maybe we could open another PR, or maybe you can get Leo to update his.)

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @trefis

I pushed the rebased patch here: trefis@5f1036f (I'll let Leo do a proper rebase)

For the record, I'm attaching the -dlambda output ocamlc without 1652 nor 1610 (that's remove-aliases.lambda), with only 1652 (broken-dont-remove-aliases) and with both (fixed-dont-remove-aliases).
The first and last one are the same, only the second one differs.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @mshinwell

Let's add this example to the testsuite in #1610 too...

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @trefis

Spoke too quickly, I had tested the rebase of gpr1610 with the [@remove_aliases] uncommented.

If I remove it the example is still broken; gpr1610 actually has no impact.

Sorry.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @trefis

I think I've got a fix for this, I'll create a GPR shortly.

@vicuna
Copy link
Author

vicuna commented Apr 27, 2018

Comment author: @trefis

#1743

@vicuna
Copy link
Author

vicuna commented May 1, 2018

Comment author: @trefis

#1743 has been merged into trunk and cherry-picked to 4.07.

@vicuna vicuna closed this as completed May 1, 2018
@vicuna vicuna added this to the 4.07.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants