make string/bytes distinguishable in bytecode by hhugo · Pull Request #1617 · ocaml/ocaml

hhugo · 2018-02-17T12:09:04Z

Follow up to #596.
This PR move the distinction between bytes and string down to the bytecode.
This should allow the js_of_ocaml compiler to choose different runtime representation for bytes and string (when safe-string is on).

The most important changes are:

In bytecode, Bytes.unsafe_to_string & Bytes.unsafe_of_string are no longer no-op and now call into the C runtime. The native compilation is unchanged.
Add a new bytecode instruction for %string_unsafe_get, different from the instruction used for %bytes_unsafe_get

hhugo · 2018-02-17T12:21:49Z

For people reading emails only, I've amended the feature description:

In bytecode, Bytes.unsafe_to_string & Bytes.unsafe_of_string are no longer no-op and now call into the C runtime. The native compilation is unchanged.

nojb · 2018-02-20T12:48:57Z

bytecomp/translcore.ml

-  "%caml_string_set32u", Pstring_set_32(true);
-  "%caml_string_set64", Pstring_set_64(false);
-  "%caml_string_set64u", Pstring_set_64(true);
+  "%caml_string_set16", Pbytes_set_16(false);


Is there a good reason to keep the old names %caml_string_set*? I agree that backwards compatibility is generally good, but am just wondering.

As @xclerc pointed out, a library such as ocp-indent will benefit from this backwards compatible change.

Several packages were broken by a previous version of a similar change by Hongbo that turned %string_safe_set into %bytes_safe_set -- see #596 (comment) . I would be wary of changing the external-referenceable name of a primitive.

nojb · 2018-02-20T12:54:48Z

byterun/str.c

+/**
+ * [caml_string_set16] is deprecated,
+ * use [caml_bytes_set16] instead
+ */


What is the reason for keeping these caml_string_set* C primitives? AFAICS they are not exposed to user code and not used by the bytecode interpreter.

I guess it is related to your previous comment, but someone might
reference them through external, e.g. ocp-endian.

But they would reference the %caml_string_set* name, not the C primitive, right?

Sorry, you meant "make the old OCaml primitive name point to a new C symbol"?

I think there could still be a problem, with an .o file generated by a previous version
of the compiler and then linked with the new runtime; however, if magic numbers are
bumped it should not be a problem (cannot remember whether magic numbers are
automatically bumped nowadays).

Yes, bumping the magic number would do the trick; I don't think supporting "old-with-new" linking is a good idea anyway.

I've made the change.

hhugo · 2018-02-20T20:01:42Z

I expect the tests to pass now.
Bootstrapping was required in order to remove primitives from the runtime.

hhugo · 2018-02-20T20:24:22Z

File "/home/travis/build/ocaml/ocaml/toplevel/opttopmain.ml", line 213, characters 24-29:
Error: Unbound value unset
Hint: Did you mean set?
make: *** [toplevel/opttopmain.cmx] Error 2

I'm not sure what this is about..

shindere · 2018-02-20T20:59:04Z

hhugo (2018/02/20 12:24 -0800):

``` File "/home/travis/build/ocaml/ocaml/toplevel/opttopmain.ml", line 213, characters 24-29: Error: Unbound value unset Hint: Did you mean set? make: *** [toplevel/opttopmain.cmx] Error 2 ``` I'm not sure what this is about..

My bad, sorry. Fixed on latest trunk.

hhugo · 2018-02-21T17:15:41Z

Marshal.to_string and Marshal.to_bytes were calling into the same c function. It is now fixed.

nojb · 2018-02-27T14:34:36Z

This patch looks correct to me, and it should be backwards compatible thanks to leaving the name of old externals caml_string_set* untouched.

However, I think the cmo magic number needs to be bumped due to the bytecode change.

xavierleroy · 2018-03-02T15:56:05Z

Warning: this PR contains a bootstrap. (Several, actually.) According to house regulations it will have to be merged manually by a core developer.

trefis · 2018-03-06T16:47:08Z

However, I think the cmo magic number needs to be bumped due to the bytecode change.

Indeed, but I think it was agreed recently that we'd change all the magic numbers between each major release anyway.
For example, we've recently merged some changes to types.ml without changing the magic number of cmis just yet.

damiendoligez · 2018-03-07T15:56:54Z

@hhugo could you reorder the commits (moving up the marshall commit) so we only have to deal with two bootstraps instead of three?

Also, rebase Changes (but not the bootstrap compilers, they are a nice reminder that we need to merge manually).

hhugo · 2018-03-07T19:47:19Z

Not sure what you meant by

Also, rebase Changes (but not the bootstrap compilers, they are a nice reminder that we need to merge manually).

This PR now only include 2 bootstraps

hhugo · 2018-03-13T21:28:49Z

I guess this PR needs a formal approval and a core-dev to manual merge & redo the bootstraps.
@nojb, would you be able to do this ?

nojb · 2018-03-13T21:41:09Z

@hhugo Yes, will do.

Rebase of #1617 (make string/bytes distinguishable in bytecode)

nojb · 2018-03-15T19:43:34Z

Merged as #1659, thanks!

hhugo · 2018-03-15T19:47:20Z

Thanks for all the boostraps

nojb reviewed Feb 20, 2018

View reviewed changes

hhugo force-pushed the safe-string2 branch 2 times, most recently from 7ff7b7b to ef36776 Compare February 20, 2018 19:59

hhugo force-pushed the safe-string2 branch from ef36776 to df6f5b4 Compare February 20, 2018 22:33

damiendoligez added the high-priority label Mar 1, 2018

hhugo force-pushed the safe-string2 branch from b901fc4 to 6104ce5 Compare March 7, 2018 19:24

hhugo added 10 commits March 7, 2018 19:27

Add Pbytes_set/get_16/32/64, Remove Pstring_set_16/32/64

6d5b8e8

bytes_of/to_string is no longer the identity in bytecode

3957c06

Add bytecode instruction to access string, different from the bytes one.

3cf76eb

Rename instructions GET/SETSTRINGCHAR -> GET/SETBYTESCHAR

6bb2a2d

fix primitives for marshal

1031232

coreboot

c23ef5c

remove unused primitives

b7725e4

fix tests

4524bb1

coreboot

55b412b

CHANGES

66ee4af

hhugo force-pushed the safe-string2 branch from 6104ce5 to 66ee4af Compare March 7, 2018 19:45

nojb approved these changes Mar 13, 2018

View reviewed changes

nojb mentioned this pull request Mar 14, 2018

Rebase of #1617 (make string/bytes distinguishable in bytecode) #1659

Merged

nojb added a commit that referenced this pull request Mar 15, 2018

Merge pull request #1659 from nojb/safe-string2

30fc788

Rebase of #1617 (make string/bytes distinguishable in bytecode)

nojb closed this Mar 15, 2018

hhugo deleted the safe-string2 branch March 15, 2018 19:47

gasche mentioned this pull request Jan 16, 2025

Immutable arrays #13097

Merged

Conversation

hhugo commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hhugo commented Feb 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhugo commented Feb 20, 2018

Uh oh!

hhugo commented Feb 20, 2018

Uh oh!

shindere commented Feb 20, 2018 via email

Uh oh!

hhugo commented Feb 21, 2018

Uh oh!

nojb commented Feb 27, 2018

Uh oh!

xavierleroy commented Mar 2, 2018

Uh oh!

trefis commented Mar 6, 2018

Uh oh!

damiendoligez commented Mar 7, 2018

Uh oh!

hhugo commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hhugo commented Mar 13, 2018

Uh oh!

nojb commented Mar 13, 2018

Uh oh!

nojb commented Mar 15, 2018

Uh oh!

hhugo commented Mar 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hhugo commented Feb 17, 2018 •

edited

Loading

hhugo commented Mar 7, 2018 •

edited

Loading