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

Stack overflow of ocamlopt while linking a pack with name clashes #6537

Closed
vicuna opened this issue Sep 3, 2014 · 5 comments
Closed

Stack overflow of ocamlopt while linking a pack with name clashes #6537

vicuna opened this issue Sep 3, 2014 · 5 comments

Comments

@vicuna
Copy link

vicuna commented Sep 3, 2014

Original bug ID: 6537
Reporter: monate
Status: feedback (set by @alainfrisch on 2015-12-11T14:49:22Z)
Resolution: open
Priority: normal
Severity: minor
Platform: AMD64
OS: Linux
Version: 4.02.0+beta1 / +rc1
Target version: later
Category: back end (clambda to assembly)
Related to: #4080
Monitored by: @hcarty

Bug description

Hi,
Product version is 4.02.0 and is installed by opam.
$ ocamlopt -v
The OCaml native-code compiler, version 4.02.0
Standard library directory: /home/tis/1.7/opam/4.02.0/lib/ocaml
$ cat u.ml
prerr_endline "Hello";;
$ ocamlopt -for-pack U -c u.ml
$ ocamlopt -o U.cmx -pack u.cmx
Fatal error: exception Stack overflow
If this use case is rejected, it should probably be marked as a change breaking compatibility with former OCaml versions.
Thanks for making OCaml so great!
B.

File attachments

@vicuna
Copy link
Author

vicuna commented Sep 3, 2014

Comment author: @damiendoligez

This is a variant of #4080. You should probably count yourself lucky that you get a compiler crash rather than a program crash.

@vicuna
Copy link
Author

vicuna commented Dec 11, 2015

Comment author: @alainfrisch

The attached diff (inspired from my first note on #4080) removes the stack overflow, and fixes the problem when the target is in a different directory. it's only a POC: it breaks the naming of exceptions, and probably would not work as is with flambda.

When the target is in the same directory, we end up asking the linker to produce an object file of the same name than one of the linked files. At least MS linker does not like it:

LINK : fatal error LNK1000: Internal error during BuildLibrary

  Version 9.00.30729.01

  ExceptionCode            = C0000005
  ExceptionFlags           = 00000000
  ExceptionAddress         = 74CDAF74 (74CA0000) "C:\Windows\WinSxS\x86_microsoft.vc90....."
  NumberParameters         = 00000002
  ExceptionInformation[ 0] = 00000001
  ExceptionInformation[ 1] = FED80000
...

We should probably fail early in this situation, although it's not really possible to know in general if two filenames point to the same location (one needs to be case insentitive if the file system is so; one needs to take symlinks into account; etc).

Rejecting the case where the name of the pack is the same (with a case insensitive comparison) as of one its components might be overly restrictive (or not).

@vicuna
Copy link
Author

vicuna commented Dec 11, 2015

Comment author: @alainfrisch

One way would be to remove the target .o/.obj file before calling the linker and then check that all input object files are still there. Is it worth doing that just to get a better error message?

@vicuna
Copy link
Author

vicuna commented Dec 11, 2015

Comment author: @alainfrisch

Benjamin, you wrote:

If this use case is rejected, it should probably be marked as a change breaking compatibility with former OCaml versions.

I don't think it was ever supported to have a packed foo.cmx include a file foo.cmx from the same directory. After the fix for #4080, this now works fine when files are in different directories. Can you clarify whether you are indeed interested in the "same directory" case, and if so, if you think it was supported before?

(I'm reducing the severity, since this is no longer a crash of the compiler but of the external linker.)

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

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

1 participant