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 C stubs more resistant to header structure #55

Closed
wants to merge 2 commits into from

Conversation

emillon
Copy link
Contributor

@emillon emillon commented Sep 26, 2023

The existing stubs cause a warning on OCaml 5.1 because the declaration of caml_convert_signal_number can not be found.

The reason for that is that <caml/signals.h> is already included by some headers at the beginning of the file, so CAML_SIGNALS_H is already set by the time <caml/signals.h> is included manually and the extra internal definition are not included.

This used to work in previous versions of OCaml because presumably the header structure was different and <caml/signal.h> was not included transitively.

This fix just sets CAML_INTERNALS at the file level; it is not really possible to be granular in setting it.

@emillon
Copy link
Contributor Author

emillon commented Sep 26, 2023

cc @rgrinberg ; this replaces #48 and #53.

@rgrinberg
Copy link
Collaborator

Isn't this worse? Previously, we'd only open the internals of caml/signals.h, but now we need the internals of all headers.

Would it be possible to just import caml/signals.h first with CAML_INTERNALS set?

@emillon
Copy link
Contributor Author

emillon commented Sep 26, 2023

I don't think that the headers support this kind of modularity: if we do that, signals.h is going to include other headers (today, misc.h, mlvalues.h) with CAML_INTERNALS set, but this might change again later.

I agree with you that it's not completely satisfactory, but:

  • this is scoped to just this file
  • we're not duplicating the declaration (this kind of thing has changed in the past; I remember some JS stubs duplicating the definition of channels and causing an error that was pretty difficult to debug)
  • this is a piece of low level code that needs to know about details of the runtime, so I'm not too shocked it needs CAML_INTERNALS

So I believe it's fine to set it.

@dra27
Copy link
Contributor

dra27 commented Sep 27, 2023

It's not "worse" - you're just getting all the internals, which is the unstable part of the OCaml API. Manually declaring the declaration is worse (they do subtly change from time-to-time - linking macros, etc., and even renaming, which you then wouldn't pick up)

emillon and others added 2 commits September 27, 2023 06:50
The existing stubs cause a warning on OCaml 5.1 because the declaration
of `caml_convert_signal_number` can not be found.

The reason for that is that `<caml/signals.h>` is already included by
some headers at the beginning of the file, so `CAML_SIGNALS_H` is
already set by the time `<caml/signals.h>` is included manually and the
extra internal definition are not included.

This used to work in previous versions of OCaml because presumably the
header structure was different and `<caml/signal.h>` was not included
transitively.

This fix just sets `CAML_INTERNALS` at the file level; it is not really
possible to be granular in setting it.

Signed-off-by: Etienne Millon <me@emillon.org>
_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Collaborator

Enabling CAML_INTERNALAS per header doesn't work, but it's still possible to mitigate the damage. I've moved <caml/signals.h> to the top and undef'd CAML_INTERNALS after its included. At least the headers that aren't pulled in by <caml/signals.h> won't have their internals open.

@emillon
Copy link
Contributor Author

emillon commented Mar 11, 2024

Fixed in #58.

@emillon emillon closed this Mar 11, 2024
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.

3 participants