Skip to content

Choosing safe-string at configure time#687

Merged
alainfrisch merged 4 commits intoocaml:trunkfrom
alainfrisch:safe_string_configure
Jul 19, 2016
Merged

Choosing safe-string at configure time#687
alainfrisch merged 4 commits intoocaml:trunkfrom
alainfrisch:safe_string_configure

Conversation

@alainfrisch
Copy link
Copy Markdown
Contributor

This PR adds a -safe-string flag to ./configure. When the system is compiled in this mode, the compile-time option -safe-string is implied, and -unsafe-string is ignored (printing a warning on stderr). This is to serve two goals:

  • Facilitate the detection of packages that are not ready for -safe-string (e.g. through an OPAM switch).
  • Start supporting optimizations that depend on immutable strings, and which can be broken by any linked unit that has not been compiled with -safe-string. In particular, this PR enables sharing of string literals in a given compilation unit.

Hopefully, this will give extra incentive to the community to move to -safe-string.

In addition to the configure flag, this PR also records in .cmi file the safe/unsafe_string status. Currently, this is only used to fail when opening a .cmi file compiled in unsafe-string mode in a compiled configured in safe-string mode. This is not bullet proof as explained in the corresponding commit message. (We don't even check that the implementation is compiled in -safe-string when its interface is; we should probably do that.) A simple stronger alternative would be to use different magic numbers (as for flambda .cmx files), but the proliferation of different magic number for each version of OCaml become tedious (magic number are not really suited to encode orthogonal flags). One could also have a flag stored in .cmi and "inherited" automatically. But anyway, this is rather minor and only to protect people from their own mistake (forgetting to recompile a library already compiled in unsafe-string mode when switching to a compiler configured with safe-string ).

(This is related to http://caml.inria.fr/mantis/view.php?id=7113.)

@alainfrisch
Copy link
Copy Markdown
Contributor Author

Any opinion on this proposal? I think we need to move forward with the safe-string migration, and this tiny step might help, so let's have it in 4.04 unless someone is against.

@Octachron
Copy link
Copy Markdown
Member

A probably naive remark, but simply ignoring "-unsafe-string" with a warning sounds quite bold at this point. Would it makes sense to rather make "-unsafe-string" an error at this point? I would imagine that if a library/executable does take the time to precise the "-unsafe-string" option it has a good reason to do so and should not be compiled with only a simple, and quite easy to miss, warning.

@hcarty
Copy link
Copy Markdown
Member

hcarty commented Jul 18, 2016

I agree with @Octachron that making --unsafe-string an error would be nice to have.

As an OCaml user I'd like to use this configure option in 4.04. I try to use the option in my projects (along with its sequence and format counterparts) but don't always remember to do so when setting them up. The fewer steps to remember for future-proof and clean code the better.

@alainfrisch
Copy link
Copy Markdown
Contributor Author

The rationale for make -unsafe-string a warning is that some projects might use this flag because they are not ready for -safe-string, but turning that into a fatal error would prevent people from seeing (e.g. on CI logs) the actual piece of code that breaks.

But this is indeed dubious, and since the default is still -unsafe-string, few projects are using the explicit option, I guess.

So I'll turn that into a proper error.

@alainfrisch alainfrisch force-pushed the safe_string_configure branch from 9960a9e to c6640ef Compare July 19, 2016 07:34
@alainfrisch
Copy link
Copy Markdown
Contributor Author

Does anyone want to review this?

error (Need_recursive_types(ps.ps_name, !current_unit))
| Unsafe_string ->
if Config.safe_string then
error (Depend_on_unsafe_string_unit (ps.ps_name, !current_unit));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this break existing code not using the new configure switch, if safe and unsafe modules are mixed? That seems like it could break a lot of code, even if it should have been broken already.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is only raised when the system is configured with safe-string (this is Config above, not Clflags). So no, no risk (unless I somehow screwed it up).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense - thank you for the explanation.

When configured with -safe-string, the OCaml tools will default to the
safe-string mode and ignore -unsafe-string command-line arguments.  This
is intended to serve two purposes:

 - Facilitate the detection of packages that are not ready
   for -safe-string ready.  (Perhaps with some OPAM switch?)

 - Enable some optimizations that assume that all linked units are
   compiled with -safe-string.

Note: currently, there is no check that units compiled with an OCaml
configured without -safe-string are not linked in.
This is used to prevent depending on a unit compiled with -unsafe-string
when the compiler has been *configured* with -safe-string.  This is not
100% bullet-proof, since the unsafe_string marker is not propagated
transitively, so it is possible to "hide" it by going through an
intermediate compilation unit compiled with -safe-string (with a
compiled configure without -safe-string).  But this should catch most
cases of old files staying around produced by a compiler not configured
with -safe-string.
@alainfrisch alainfrisch force-pushed the safe_string_configure branch from 5234ba6 to 9538a54 Compare July 19, 2016 12:29
@alainfrisch alainfrisch merged commit 8a36133 into ocaml:trunk Jul 19, 2016
@chambart
Copy link
Copy Markdown
Contributor

The asmcomp/staticalloc test is failing in this mode. Strings are not mutable anymore. I'm not sure how to write that part of the test now...

@alainfrisch
Copy link
Copy Markdown
Contributor Author

The driver Makefile could pass the value of SAFE_STRING (from config/Makefile) to the test, so it could either test with == or with !=. Do you see something simpler?

@chambart
Copy link
Copy Markdown
Contributor

This looks like the right solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants