|Anonymous | Login | Signup for a new account||2017-09-21 10:52 CEST|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0005461||OCaml||compiler driver||public||2012-01-03 12:33||2017-03-01 16:50|
|Target Version||Fixed in Version||3.13.0+dev|
|Summary||0005461: Double linking of bytecode modules|
|Description||It 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(_,_,_) ->
| _ ->
--- y.ml ---
let () =
Now link as follows:
$ ocamlc -o z unix.cma z.ml unix.cma y.ml
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
both define a module named Unix
|Attached Files|| 0001-PR-5461-an-incorrect-patch-to-fail-when-a-bytecode-c.patch [^] (3,450 bytes) 2012-01-03 14:28 [Show Content]
2012-01-11-updated-patch-to-prevent-double-link-with-ocamlbuild.patch [^] (3,942 bytes) 2012-01-11 19:16 [Show Content]
2012-01-11-updated-patch-to-use-warning-31.patch [^] (30,507 bytes) 2012-01-11 20:07 [Show Content]
warning_31.log [^] (4,021 bytes) 2012-02-10 15:29
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".
|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).|
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
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).
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).
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.
|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.|
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...
|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.|
|I added it as warning 31. What should we do now, merge in SVN ?|
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.
+"ocamlbuildlight.byte": -use_unix, nopervasives
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.
|I applied my last patch, with Nicolas' modification to avoid the double linking while building ocamlbuild_light.byte (commit r12033 in SVN trunk).|
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?
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.
Still present in 4.00.1
|I submitted a fix in revision 13296 of the SVN trunk. Anybody can double check ?|
|Now that the compiler is free of warning 31 (I guess!), I'd suggest to turn it into a proper error, since it can lead to segfaults (see example in 0004231). We could use OPAM to test that it doesn't break existing projects (or fix them).|
|Let's keep it as a warning for 4.01 and think about having "+31" as the default warn-error for 4.02.|
> Let's keep it as a warning for 4.01 and think about having "+31" as the default warn-error for 4.02.
Ok for 4.01, but then, shouldn't we turn it into a proper error?
|Warning 31 is now a "warn-error" by default. I'd prefer to have a real error at some point, so I leave this ticket open.|
|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|
|2013-06-14 13:35||frisch||Note Added: 0009486|
|2013-06-18 13:16||doligez||Note Added: 0009544|
|2013-06-18 13:17||doligez||Target Version||=> 4.02.0+dev|
|2013-06-18 13:22||frisch||Note Added: 0009545|
|2013-07-12 18:15||doligez||Target Version||4.02.0+dev => 4.01.1+dev|
|2013-11-29 13:01||doligez||Tag Attached: patch|
|2014-05-25 20:20||doligez||Target Version||4.01.1+dev => 4.02.0+dev|
|2014-07-30 23:05||doligez||Target Version||4.02.0+dev => 4.02.1+dev|
|2014-09-04 00:25||doligez||Target Version||4.02.1+dev => undecided|
|2015-12-09 18:49||frisch||Note Added: 0015100|
|2016-04-19 13:31||doligez||Target Version||undecided => later|
|2017-02-23 16:36||doligez||Category||OCaml general => -OCaml general|
|2017-02-27 13:49||doligez||Assigned To||lefessan => frisch|
|2017-02-27 13:49||doligez||Priority||normal => low|
|2017-02-27 13:49||doligez||Category||-OCaml general => compiler driver|
|2017-02-27 13:49||doligez||Target Version||later =>|
|2017-03-01 16:50||frisch||Assigned To||frisch => shinwell|
|2017-03-01 16:50||frisch||Status||confirmed => assigned|
|Copyright © 2000 - 2011 MantisBT Group|