Mantis Bug Tracker
Main | My View | View Issues | Change Log | Roadmap
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005461OCamlOCaml generalpublic2012-01-03 12:332013-02-18 13:18
Reportergerd 
Assigned Tolefessan 
PrioritynormalSeverityminorReproducibilityalways
StatusconfirmedResolutionfixed 
PlatformOSOS Version
Product Version3.12.1 
Target VersionFixed in Version3.13.0+dev 
Summary0005461: Double linking of bytecode modules
DescriptionIt is possible to link toplevel modules several times (bytecode only). This leads to errors that are hard to find and to track down, like that exceptions cannot be caught anymore.
Steps To Reproduce--- z.ml ---

let m ex =
  match ex with
    | Unix.Unix_error(_,_,_) ->
    print_endline "match"
    | _ ->
    print_endline "nomatch"

--- y.ml ---

let () =
  Z.m(Unix.Unix_error(Unix.ENOENT,"",""))

Now link as follows:

$ ocamlc -o z unix.cma z.ml unix.cma y.ml

And:

$ ./z
nomatch

ocamlopt is immune:

ocamlopt -o z unix.cmxa z.ml unix.cmxa y.ml
File "y.ml", line 1, characters 0-1:
Error: Files /opt/godi-3.12/lib/ocaml/std-lib/unix.cmxa
       and /opt/godi-3.12/lib/ocaml/std-lib/unix.cmxa
       both define a module named Unix
TagsNo tags attached.
Attached Filespatch file icon 0001-PR-5461-an-incorrect-patch-to-fail-when-a-bytecode-c.patch [^] (3,450 bytes) 2012-01-03 14:28 [Show Content]
patch file icon 2012-01-11-updated-patch-to-prevent-double-link-with-ocamlbuild.patch [^] (3,942 bytes) 2012-01-11 19:16 [Show Content]
patch file icon 2012-01-11-updated-patch-to-use-warning-31.patch [^] (30,507 bytes) 2012-01-11 20:07 [Show Content]
log file icon warning_31.log [^] (4,021 bytes) 2012-02-10 15:29

- Relationships
has duplicate 0004231resolvedlefessan Link multiple casse le système de types 

-  Notes
(0006581)
gerd (reporter)
2012-01-03 12:40

 Of course it's usually not such a simple ocamlc invocation. The problem can be well hidden, e.g. there can be two libraries l1.cma and l2.cma, and both contain the same module by accident.

As bytecode can be partially linked, it is easy to get into such situations even with standard modules like Unix (the problem report I got showed exactly this problem) - just have funny users who do "ocamlc -a -o out.cma unix.cma mylib.cma".
(0006582)
ygrek (reporter)
2012-01-03 13:20

 This is indeed a rather hard to find issue for the novice. Even in the ocamlopt case - the error message could be better (e.g. add the check that both files are not only defining the same module but are actually the same file).
(0006583)
gasche (developer)
2012-01-03 14:35
edited on: 2012-01-03 14:36

 I wrote a quick patch (attached) to tackle the issue, that just reproduces the error logic of asmlink.ml into bytelink.ml. It turns out that such an easy patch does *not* work. My understanding (which may be wrong) is that the way dependencies between compilation units are detected is fundamentally different between native and bytecode compilation, and is accurate in the native case, but approximate in the bytecode case. While that doesn't make a difference in most cases, here it turns out that the bytecode heuristic is not precise enough, and raises a lot of "multiple definitions" when compiling existing code (the OCaml compiler).

My conclusion is that fixing this is harder than it seems, and it might even not be entirely fixable (without losing the separate compilation properties that .cmo have today, and that .cmx don't have). I would go for a warning, instead of failing with an error, when this situation is (approximately) detected.

Of course, this is limited by my own understanding. A developer from the Ocaml team may provide further information and find a convenient way to fix this. I'm just saying that it does not look simple.

Below is a typical error I get when trying to "make world" with this patch applied:

+ boot/ocamlrun boot/myocamlbuild.boot -tag-line true: -use_stdlib -log _boot_log1 ocamlbuild/ocamlbuildlightlib.cma ocamlbuild/ocamlbuildlight.byte
../ocamlcomp.sh -g -I stdlib stdlib/pervasives.cmo ocamlbuild/ocamlbuild_pack.cmo ocamlbuild/ocamlbuildlight.cmo -o ocamlbuild/ocamlbuildlight.byte
File "_none_", line 1:
Error: Files stdlib/pervasives.cmo and ../stdlib/stdlib.cma(Pervasives)
       both define a module named Pervasive
(0006585)
frisch (developer)
2012-01-04 07:33
edited on: 2012-01-04 07:33

 The problem can also be triggered without libraries and also with ocamlopt (using -pack in this case); actually, it is not only about weird semantics, this can really break the soundness of the type system. This was reported (in French) in 0004231. One should also be careful with dynlink (0004229, 0004839).
(0006603)
lefessan (developer)
2012-01-05 23:13

 Gabriel, the fact that your patch triggers compilation errors for OCaml itself shows that the problem is even worse, since in the example you give, Pervasives is linked twice in ocamlbuildlight.byte (what is then, for example, the behavior of "at_exit", since it appears now twice in the executable ?).

We should actually use your patch to fix these problems first in the distribution, and then maybe add an option -linkonce to trigger your patch only on demand (once namespaces will be available in OCaml, there will be no reason to accept double linking of the same module, since namespaces can be used to solve this issue).
(0006604)
gasche (developer)
2012-01-05 23:31

 I agree that those could be bugs, but that is for maintainers to decide at each occurence. I'll ping Xavier Clerc.

I think the simplest course of action would be to create a warning enabled by default, that could be turned off (in the strange cases where this is the intended behavior) or into an error (for sane developpers) by default. I don't think we could have an error right now, if that breaks existing programs.

The reason why I haven't provided a patch with the warning yet is that adding a warning is heavier than adding an error (you need to pick a warning number, etc.), so it's best to know if that is considered a good choice first.

Alain, what do you think? Your 0004231 conclusion was rather on the error side. We could also implement the heuristic your suggest for packed modules.
(0006617)
frisch (developer)
2012-01-06 19:38

 I've some preference for the error solution; we are talking about a way to circumvent the type system here (-> type unsoundness), and there is no reason to accept it... But there might some code base around which depend on the current behavior and would not be trivial to fix, so a warning could be a good (temporary?) solution.
(0006636)
lefessan (developer)
2012-01-10 10:39

 I tried for one hour to tell ocamlbuild to not link with stdlib/pervasives.cmo, but I don't even understand when it decides to link with it (I tried multiple options, -no-stdlib, changing the tag line, -nopervasives, logging and going through the sources). As I don't want to become crazy, could somebody who understand the tool prevent it from double linking pervasives.cmo ?

What about switching to Makefile to build ocamlbuild and camlp4 ? If somebody agrees with it, I can fill a bug report asking for it...
(0006644)
lefessan (developer)
2012-01-11 19:19

 Gabriel, I updated your patch to prevent ocamlbuild from linking twice several modules (I had to put all the modules of stdlib in a -ignore '...' argument, but it worked). So, now, we just have to replace the error by a warning to make it ready for inclusion.
(0006645)
lefessan (developer)
2012-01-11 20:08

 I added it as warning 31. What should we do now, merge in SVN ?
(0006650)
ertai (developer)
2012-01-12 00:35

 What about adding the nopervasives tag to ocamlbuildlight.byte in the ocamlbuild/_tags?

I did some light testing and seen the extra pervasives.cmo disapear.

I acknowledge that the handling of stdlib vs nostdlib, pervasives vs nopervasives, ocamlbuildlight vs ocamlbuild, ... makes the whole thing complicated. However I think that any building tool end up like this if we do not make the extra work of simplifying the problem from time to time.

diff ocamlbuild/_tags
-"ocamlbuildlight.byte": -use_unix
+"ocamlbuildlight.byte": -use_unix, nopervasives
(0006652)
gasche (developer)
2012-01-12 06:34
edited on: 2012-01-12 07:03

 In any case, I think we could separate the fix in two, with the compiler changes in one patch, and eventual ocamlbuild (or anything else) fixes in a second patch. This would make review, blaming etc. easier.

I don't know about the ocamlbuild part, but on the purely-compiler side Fabrice's patch looks good to me -- thanks for taking the time to write it.
(0006707)
lefessan (developer)
2012-01-17 22:59

 I applied my last patch, with Nicolas' modification to avoid the double linking while building ocamlbuild_light.byte (commit r12033 in SVN trunk).
(0006912)
gasche (developer)
2012-02-10 15:29

 As of the current trunk (12114) I still get some related warnings. I have attached the output of `grep "Warning 31" -B 3` on my `make world.opt` output. There is still one warning for pervasives.cmo for ocamlbuildlight.byte, and a bunch of warnings for camlp4boot.byte.

How should they be fixed?
(0006913)
lefessan (developer)
2012-02-10 16:39

 I don't understand: these are the original warnings, that were supposed to be removed by the modification of _tags provided by Nicolas. Moreover, they used to trigger an error, not a warning.

I won't fight again with ocamlbuild. My previous patch (attached) fixes this problem by asking ocamlbuild to ignore stdlib modules. It can still be used unless a more elegant solution is found.
(0008858)
lefessan (developer)
2013-02-18 11:52

 Still present in 4.00.1
(0008859)
lefessan (developer)
2013-02-18 13:18

 I submitted a fix in revision 13296 of the SVN trunk. Anybody can double check ?

- Issue History
Date Modified Username Field Change
2012-01-03 12:33 gerd New Issue
2012-01-03 12:40 gerd Note Added: 0006581
2012-01-03 13:20 ygrek Note Added: 0006582
2012-01-03 14:28 gasche File Added: 0001-PR-5461-an-incorrect-patch-to-fail-when-a-bytecode-c.patch
2012-01-03 14:35 gasche Note Added: 0006583
2012-01-03 14:36 gasche Note Edited: 0006583 View Revisions
2012-01-04 07:29 frisch Relationship added duplicate of 0004231
2012-01-04 07:33 frisch Note Added: 0006585
2012-01-04 07:33 frisch Note Edited: 0006585 View Revisions
2012-01-05 23:13 lefessan Note Added: 0006603
2012-01-05 23:31 gasche Note Added: 0006604
2012-01-06 19:38 frisch Note Added: 0006617
2012-01-10 10:39 lefessan Note Added: 0006636
2012-01-11 19:16 lefessan File Added: 2012-01-11-updated-patch-to-prevent-double-link-with-ocamlbuild.patch
2012-01-11 19:19 lefessan Note Added: 0006644
2012-01-11 20:07 lefessan File Added: 2012-01-11-updated-patch-to-use-warning-31.patch
2012-01-11 20:08 lefessan Note Added: 0006645
2012-01-11 20:11 lefessan Relationship replaced has duplicate 0004231
2012-01-11 20:12 lefessan Assigned To => lefessan
2012-01-11 20:12 lefessan Status new => acknowledged
2012-01-12 00:35 ertai Note Added: 0006650
2012-01-12 06:34 gasche Note Added: 0006652
2012-01-12 07:03 gasche Note Edited: 0006652 View Revisions
2012-01-17 22:59 lefessan Note Added: 0006707
2012-01-17 22:59 lefessan Status acknowledged => resolved
2012-01-17 22:59 lefessan Fixed in Version => 3.13.0+dev
2012-01-17 22:59 lefessan Resolution open => fixed
2012-02-10 15:29 gasche Note Added: 0006912
2012-02-10 15:29 gasche File Added: warning_31.log
2012-02-10 16:39 lefessan Note Added: 0006913
2013-02-18 11:52 lefessan Note Added: 0008858
2013-02-18 11:52 lefessan Status resolved => confirmed
2013-02-18 13:18 lefessan Note Added: 0008859

Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker