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

Make caml_named_value return a const value* #2293

Merged
merged 4 commits into from Mar 18, 2019

Conversation

@stedolan
Copy link
Contributor

commented Mar 6, 2019

Values can be passed from OCaml to C by registering them under a known name using Callback.register and retrieving them using caml_named_value, which returns a value* that is then dereferenced.

This patch changes the return type to const value*, to indicate that the C code cannot use the pointer that is returned to change which value is registered. The ability to mutate the value using the raw pointer returned by caml_named_value is:

  • incompatible with multicore
  • never used, as far as I can tell (see below)

This patch requires minor code changes to make C stubs continue to compile cleanly (detailed below). As well as the change to the type of caml_named_value, this patch makes these changes where necessary in the stdlib, and updates the manual to reflect the change.

Code changes in C stubs

There are two ways that caml_named_value is used in C stubs. The first is by immediately dereferencing the returned pointer:

*caml_named_value(...)

The second is by caching the returned pointer in a static variable:

static value* cache;
if (!cache) cache = caml_named_value(...);
... *cache ...

With this patch, the second type of occurrence will cause a compiler warning ("assignment discards const qualifier", or something to that effect). This warning is non-fatal (unless compiling with -Werror or compiling as C++) and no undefined behaviour is triggered by ignoring it.

To avoid the warning, the declaration of cache must be changed to:

static const value* cache;

This change is compatible with all versions of OCaml: it is always safe to store a value* in a variable of type const value*. So, it is not necessary to maintain two versions of C stubs to cleanly compile both before and after this patch, but C stubs will generate a warning until they're updated.

OPAM survey

To check whether all uses of caml_named_value fit one of the patterns above, I downloaded the sources of the latest version of each OPAM package (using this script) and grepped them.

In OPAM, there are 1297 references to caml_named_value, of which 627 match the first pattern above (they dereference immediately as *caml_named_value(...) and are unaffected by this patch).

The remaining 670 nontrivial occurrences are spread over 173 files of C source, 8 files of C++ source, and 85 other files (mostly documentation and generated JavaScript).

I manually inspected all of the C++ source occurrences and a random sample of 10% of the C occurrences; every single one fit the second pattern above. After this patch, these occurrences will generate a warning until a const is added. The 8 C++ sources (in packages irrlicht, lablqt and leveldb) will fail to compile until const is added.

Edit: The occurrence count numbers above are all a bit too high (by ~20%), since my hacky script downloads the source of each package, despite the fact that many packages share the same source.

Optimisation

A minor side benefit of this change is that since the registered values can no longer be modified from C code, they can be registered with caml_register_generational_global_root instead of caml_register_global_root. This means that they need no longer be scanned during minor GC. (I expect the benefit will be minimal, however: few programs register very many of these values).

@stedolan stedolan changed the title Make `caml_named_value` return a `const value*` Make caml_named_value return a const value* Mar 6, 2019
@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

I agree, in hindsight this would have been a better API. I'm still worried about the number of packages that this change might break. I'm afraid most packages will just ignore the warning or cast back to char *, negating the additional safety you hope to gain with this change. So, I'm on the fence. Other opinions welcome.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Is your concern that packages will break because of the const warning (e.g. because they're using -Werror), or that packages will break because they rely on the ability to mutate the pointers returned by caml_named_value?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Is your concern that packages will break because of the const warning (e.g. because they're using -Werror)

Yes.

, or that packages will break because they rely on the ability to mutate the pointers returned by caml_named_value?

No. I'm not expecting any existing code to do this, for the reasons you wrote.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

In that case, an alternative would be to leave the function non-const but keep the changes to the examples in the manual, to indicate that mutation of the returned value is unsupported.

I'd prefer a warning for new code, but perhaps that could be opt-in via some "strict mode"-type preprocess macro

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

We could also merge the change, and consider reverting if the later OPAM builds show us packages broken by the change. (This might happen as late as 4.09+beta1, if we don't ping opam-build-testers before that, but this change is easy to recognize and revert.)

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

We talked about various ways of conditionalising the API changes for multicore so that people could gradually opt into them. Could we do a similar thing here?

stedolan added a commit to ocaml-multicore/ocaml-multicore that referenced this pull request Mar 8, 2019
It's not too hard to support in multicore, and it avoids lots of
compatibility pain that was caused by the caml_named_root API.

Multicore support relies on the fact that named roots are not
mutated by C code. See ocaml/ocaml#2293 for discussion.
stedolan added a commit to ocaml-multicore/ocaml-multicore that referenced this pull request Mar 11, 2019
It's not too hard to support in multicore, and it avoids lots of
compatibility pain that was caused by the caml_named_root API.

Multicore support relies on the fact that named roots are not
mutated by C code. See ocaml/ocaml#2293 for discussion.
@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

We could also merge the change, and consider reverting if the later OPAM builds show us packages broken by the change.

I'm OK with this, but would be happier if the OPAM-wide testing could take place soon rather than at the last minute before releasing 4.09.

Copy link
Contributor

left a comment

I reviewed the changes and they look good to me.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@xavierleroy : it's your lucky week! @Armael and myself are working on this from Marrakech.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

Armael and myself are working on this from Marrakech.

I'm so glad you two are in Marrakech :-)

stedolan added 4 commits Mar 6, 2019
This commit just adds "const" in several places.
This is a mild optimisation, valid now that named values cannot
be modified directly from C code.
@stedolan stedolan force-pushed the stedolan:named-value-const branch from 40d8958 to c58cddf Mar 12, 2019
@stedolan stedolan merged commit 4f03a14 into ocaml:trunk Mar 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.