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

-for-pack does not implement proper namespace isolation of internal symbols #7086

Closed
vicuna opened this issue Dec 11, 2015 · 10 comments
Closed

Comments

@vicuna
Copy link

vicuna commented Dec 11, 2015

Original bug ID: 7086
Reporter: @alainfrisch
Status: acknowledged (set by @xavierleroy on 2015-12-13T17:51:51Z)
Resolution: open
Priority: normal
Severity: minor
Category: back end (clambda to assembly)
Monitored by: @diml @hcarty

Bug description

If we compile u.ml with "-for-pack X" and x__U.ml without -forpack, both will generate symbols such as "_camlX__U" that would clash if we link the two units together.

@vicuna
Copy link
Author

vicuna commented Dec 13, 2015

Comment author: @xavierleroy

" Doctor, it hurts when I do this!

  • Then don't do it."

If you feel strongly about this, we could probably use '$' instead of '__' as the module separator. '$' followed by an integer (ASCII code) is used in code emitters to encode infix symbols, but I think no conflict is possible there. However, some ports (PPC) use '.' instead of '$' for the latter purpose, and I'm not sure why. Could this indicate that some assemblers do not support '$' in symbols?

@vicuna
Copy link
Author

vicuna commented Dec 13, 2015

Comment author: @xavierleroy

Of course, '.' as module separator would be uber cool, but again I'm unsure about support by various assemblers (MASM?)

@vicuna
Copy link
Author

vicuna commented Dec 14, 2015

Comment author: @alainfrisch

What about using _ as the module separator, escaping _ into __ inside names ?

@vicuna
Copy link
Author

vicuna commented Feb 3, 2016

Comment author: @damiendoligez

I don't think this problem is pressing enough to justify mangling user idents.

@vicuna
Copy link
Author

vicuna commented Dec 8, 2016

Comment author: @mshinwell

Also, Flambda cannot cope with this situation (#7349 and "A question about the semantics of -for-pack" on caml-devel). It was suggested that perhaps there should be a proper error in this situation, since it causes a fatal error with Flambda.

@vicuna
Copy link
Author

vicuna commented Mar 7, 2017

Comment author: @alainfrisch

What do you mean by "this situation"? Here, this is not about linking directly module compiled with -pack. The problem is the same if the module "U" in the PR description is -packed into a module X and this X is linked together with X__U. The problem here is not related to module clashes, but to the internal convention used by -pack to avoid symbol conflicts.

@vicuna
Copy link
Author

vicuna commented Mar 7, 2017

Comment author: @mshinwell

Ah yes, I misread this one.

@github-actions
Copy link

github-actions bot commented May 9, 2020

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.

@github-actions github-actions bot added the Stale label May 9, 2020
@lthls
Copy link
Contributor

lthls commented May 9, 2020

As far as I know, this is not restricted to packs, defining a submodule U in a regular file x.ml will generate symbols that may clash with symbols in x__U.ml (at least with flambda, I don't remember what closure generates in this case).
I think this issue should be kept open, as a reminder that we may want to choose a new separator in symbols that is not allowed in identifiers, but it is very low priority.

@github-actions github-actions bot removed the Stale label Jun 15, 2020
@github-actions
Copy link

github-actions bot commented Oct 8, 2021

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

2 participants