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

No CSE with string constants #6991

Closed
vicuna opened this issue Sep 16, 2015 · 7 comments
Closed

No CSE with string constants #6991

vicuna opened this issue Sep 16, 2015 · 7 comments

Comments

@vicuna
Copy link

vicuna commented Sep 16, 2015

Original bug ID: 6991
Reporter: @mmottl
Status: acknowledged (set by @xavierleroy on 2015-11-23T13:47:16Z)
Resolution: open
Priority: normal
Severity: feature
Version: 4.02.3
Target version: later
Category: back end (clambda to assembly)
Monitored by: @jmeber @mmottl

Bug description

Since strings have recently become immutable, it would make sense to only emit one instance of each string during code generation and share the memory location. This is already the case with other immutable values.

E.g. note the difference for the generated assembly for the following file, where "x" and "y" are shared, but "a" and "b" are not.


let x = Some 42
let y = Some 42

let a = Some "asdf"
let b = Some "asdf"

@vicuna
Copy link
Author

vicuna commented Sep 17, 2015

Comment author: @alainfrisch

Strings are immutable only when compiling with -safe-string but a module compiled with this option can very well be linked with another one not using it, thus breaking the invariant that strings cannot be mutated even for strings created in that module, as soon as they can be accessed from outside.

We could ignore this problem and declare that in a given compilation unit compiled with -safe-string, we allow the compiler to do as if strings were indeed immutable. To be safer, we should then perhaps put these string literals into a read-only data section.

@vicuna
Copy link
Author

vicuna commented Sep 17, 2015

Comment author: @mmottl

Right, "-safe-string" is actually not the default yet, but imperative operations on strings are already documented as deprecated. I think it won't take long until the flag becomes the default.

I think ignoring the issue may be acceptable. It's hard to imagine that there is any production code out there that relies on mutability of string constants across module boundaries and also doesn't compile the modules with the same set of flags.

Putting immutable values (maybe not just strings) in a read-only data section is probably a good idea.

@vicuna
Copy link
Author

vicuna commented Sep 23, 2015

Comment author: gerd

There is no read-only section for bytecode. So, whatever you do it is only a half solution until mutable strings are entirely removed from the language.

@vicuna
Copy link
Author

vicuna commented Sep 25, 2015

Comment author: @alainfrisch

Even with read-only section, this would be a half solution. Getting an unrecoverable "attempted to write to read-only memory" error when writing to a bytes value would not be very satisfactory anyway.

I think it won't take long until the flag becomes the default.

Even if it is the default, as long as mutable strings are possible, we cannot do anything really robust.

And unfortunately, considering our tradition of backward compatibility for core features, I don't see mutable strings completely removed in the medium term.

@vicuna
Copy link
Author

vicuna commented Nov 23, 2015

Comment author: @xavierleroy

I'd be in favor to implement this when -safe-strings becomes the default compilation mode. This won't be in 4.03 as too many libraries haven't been updated to safe strings yet. Postponing to after 4.03.

@vicuna
Copy link
Author

vicuna commented Nov 23, 2015

Comment author: @mshinwell

A minor change in Flambda should produce the desired behaviour, once -safe-string is the default.

@xavierleroy
Copy link
Contributor

Since 4.10, -safe-string is the default configuration and string literals are shared as expected.

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