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

value access to extensible variant tag binds module value instead (name clash) #7563

Closed
vicuna opened this issue Jun 22, 2017 · 7 comments
Closed
Assignees

Comments

@vicuna
Copy link

vicuna commented Jun 22, 2017

Original bug ID: 7563
Reporter: fahndrich
Assigned to: @gasche
Status: resolved (set by @gasche on 2017-06-26T15:21:01Z)
Resolution: fixed
Priority: normal
Severity: major
Platform: x86
OS: Linux
OS Version: Linux-gnu Centos
Version: 4.03.0
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: middle end (typedtree to clambda)
Monitored by: @gasche

Bug description

A reference from a module A to B picks up B.C (a module value) instead of B.C an extensible variant tag.

This results in a block with tag 0 containing the module values being passed around instead of an extensible variant value.

Repro is attached.

Steps to reproduce

ocamlopt a_module.ml debug.ml variant.ml repro.ml -o repro
./repro

Prints:
Stored: a Clash
Direct: unknown: tag: 0, Size 3
0: An int 0
1: tag: 247, Size 2
0: <unknown with tag 1001>
1: An int 1
2: tag: 247, Size 2
0: <unknown with tag 1001>
1: An int 1
Fatal error: exception Invalid_argument("compare: functional value")

The first print is getting the right variant value by accessing it via another name.
The second print is getting the wrong module value and I added some debug output to show the contents. As you can see the contents corresponds to the a_module contents (an int (empty list), and 2 closures).

The fatal error is trying to compare the ill-bound variant tag (which is really a module) and causing comparison of closures.

Additional information

Tested on 4.02.3, 4.03.0, 4.04.1, and 4.06.0+trunk and it fails in all cases.

(My category above is just a guess, middle end)

File attachments

@vicuna
Copy link
Author

vicuna commented Jun 23, 2017

Comment author: @gasche

Thanks for the report! The bug was introduced by module aliases in 4.02. Two ways to make the reproduction case simpler are:

  • A_module does not need to be an external module, it can be declared above (but you need a definition of the form "module Alias = A_module", just "module Alias = struct .. end" would not work). On the other hand, the access to the conflict name needs to be from a different compilation unit.

  • You can use "exception Clash" instead of the extensible datatype (anything that both uses transl_path in bytecomp/translcore.ml and has an uppercase last component would work), which allows to test the code under pre-4.02 version to check that the bug cannot be reproduced.

The following patch fixes the issue, but I am not sure that it is correct overall.

diff --git a/bytecomp/lambda.ml b/bytecomp/lambda.ml
index cca6315..b286772 100644
--- a/bytecomp/lambda.ml
+++ b/bytecomp/lambda.ml
@@ -535,7 +535,7 @@ let rec transl_normal_path = function
(* Translation of value identifiers *)

let transl_path ?(loc=Location.none) env path =

  • transl_normal_path (Env.normalize_path (Some loc) env path)
  • transl_normal_path (Env.normalize_path_prefix (Some loc) env path)

(* Compile a sequence of expressions *)

@vicuna
Copy link
Author

vicuna commented Jun 23, 2017

Comment author: @gasche

Proposed fix (plus a regression testcase) sent at

#1210

@vicuna
Copy link
Author

vicuna commented Jun 23, 2017

Comment author: @yallop

For full dramatic effect, here's a way to turn the bug into a segmentation fault:

$ ocaml
OCaml version 4.04.0

module M = struct

   module X = List
   exception X
 end;;

module M : sig module X = List exception X end

print_endline (Printexc.to_string M.X);;

Segmentation fault

@vicuna
Copy link
Author

vicuna commented Jun 23, 2017

Comment author: @gasche

Reporter: I am about to write a Changes entry, what is the name that should be used for crediting the report? Are you Manuel Fähndrich or Manuel Fahndrich? (Is it correct to assume that the first version is actually the correct one, despite the second one occurring more often on the internet?). If not, what name should be used to credit your work?

@vicuna
Copy link
Author

vicuna commented Jun 23, 2017

Comment author: fahndrich

I'm the same person. Do I have 2 accounts? I dropped the umlaut a long time ago when I went to the US. I only use it in LaTeX papers :-)

@vicuna
Copy link
Author

vicuna commented Jun 23, 2017

Comment author: @gasche

Well, googling for "site:caml.inria.fr/mantis fahndirch" only reported issues from way back, submitted also using your first name, so I preferred to ask than just assume. Thanks!

@vicuna
Copy link
Author

vicuna commented Jun 26, 2017

Comment author: @gasche

#1210 was merged after a round and review and some changes. It is only in the current trunk, so it will not be part of the next major release 4.05 that is frozen right now, but the following one (4.06).

Thanks for the report!

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