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
Should String_val return 'const char *' when safe-string is globally enabled? #7594
Comments
Comment author: @xavierleroy If we do this, the following common idiom char * p = String_val(v); and probably many others will warn or break compilation if -warn-error. That means breakage not only in .ml files but also on .c files. If anyone wants to experiment with this approach and report on the breakage, that would be most welcome. |
Comment author: @xavierleroy And of course we need #define Bytes_val(x) ((char *) Bp_val(x)) in <caml/mlvalues.h>. On second thoughts, make that "(unsigned char *)" just to gain more machine independence. ("char" can be signed or unsigned.) |
Comment author: @yallop I ran some preliminary experiments, changing String_val in trunk to return 'const char *', and looking at what needed to be changed in the distribution. The branch is here: yallop/ocaml@trunk...yallop:const-string-val Summary:
It'd also be helpful to look at some packages that use C extensions. I plan to look at a few such packages shortly. |
Comment author: @xavierleroy Thanks for the experiment, these are interesting data points.
Just one remark: I think Bytes_val should be reserved for data that has type bytes on the Caml side, not to gain write access to a Caml string. For the latter use, "(char *) String_val(v)" makes the intent clearer, in my opinion. |
Comment author: @yallop
Thanks for the clarification. I agree that what you suggest is better. Ok, here are a few statistics about String_val and OPAM packages. Of the OPAM packages that call String_val (and that aren't explicitly constrained to an earlier OCaml version)
|
Comment author: @mshinwell I'm in favour of a change along these lines. |
Original bug ID: 7594
Reporter: @yallop
Assigned to: @yallop
Status: resolved (set by @yallop on 2017-08-03T13:27:57Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.06.0 +dev/beta1/beta2/rc1
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: runtime system and C interface
Monitored by: @gasche @dbuenzli
Bug description
Since GPR 687 (#687) it's possible to enable -safe-string globally at configure time.
There's a second GPR open (#1252) that makes configuration-time -safe-string the default.
At present, -safe-string only affects OCaml code; C stubs that mutate OCaml strings will continue to compile without error. This may lead to subtle bugs, not least since the compiler now takes string immutability into account during optimizations (#703).
Should we update String_val to return 'const char *' when -safe-string is enabled, like this?
#ifdef CAML_SAFE_STRING
#define String_val(x) ((const char *) Bp_val(x))
#else
#define String_val(x) ((char *) Bp_val(x))
#endif
We'd also need a new 'Bytes_val' that always returned 'char *'.
This change would be slightly backwards-incompatible, but probably not any more so than defaulting to -safe-string. And it seems likely to improve the quality of code, by making it more explicit which C code can actually modify OCaml string/bytes values.
The text was updated successfully, but these errors were encountered: