Skip to content

Use the usual layout to compile freestanding C stubs#441

Merged
dinosaure merged 5 commits into
mirage:masterfrom
dinosaure:compatible-c-layout
Mar 17, 2021
Merged

Use the usual layout to compile freestanding C stubs#441
dinosaure merged 5 commits into
mirage:masterfrom
dinosaure:compatible-c-layout

Conversation

@dinosaure
Copy link
Copy Markdown
Member

@dinosaure dinosaure commented Feb 18, 2021

This PR wants to provide a smooth transition between MirageOS 3 and MirageOS 4. Even if the goal permits a long term support of MirageOS 3 and tcpip, the real goal is to avoid any constraints between tcpip, MirageOS 3 and MirageOS 4.

As we checked (@TheLortex and me) the usual layout for C stubs available on mirage-crypto and digestif is compatible with MirageOS 4. tcpip (due to the story) still continue to use some corner cases about the MirageOS 3 linking step. This PR is like an unification of C stubs in our MirageOS eco-system.

By this way and if we cut a release with that, we don't need to upgrade tcpip for MirageOS 4 and we don't need to care about incompatible details between MirageOS 3, MirageOS 4 and this version of tcpip. So it paves the way for a smooth transition.

NOTE: the new artifact libtcpip_freestanding_stubs.a is needed only for MirageOS 3 (at the linking step). Then, when time forgets MirageOS 3, we will be able to delete that.

@dinosaure dinosaure force-pushed the compatible-c-layout branch 4 times, most recently from 6f1ebff to 671fda4 Compare February 18, 2021 17:29
@dinosaure dinosaure mentioned this pull request Feb 19, 2021
Merged
@hannesm
Copy link
Copy Markdown
Member

hannesm commented Feb 19, 2021

AFAICT at the moment the C stubs are provided by mirage-xen & mirage-solo5 -- including them here will lead to duplicated symbols at the link step (this means, mirage-solo5/mirage-xen should be updated to not provide the symbols anymore at the same time)... maybe this PR should be delayed until mirage4 is out (which, IIUC, will handle C stubs without the need for extra Makefiles)?

@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Feb 19, 2021

AFAICT at the moment the C stubs are provided by mirage-xen & mirage-solo5 -- including them here will lead to duplicated symbols at the link step (this means, mirage-solo5/mirage-xen should be updated to not provide the symbols anymore at the same time)...

It seems that the gcc linker is less strict because, with this PR, I can link the unikernel.

Currently, tcpip is incompatible with MirageOS 4 due to this old linking-trick - next week, I will try to figure out why precisely. This PR is only a bridge between MirageOS 3 and MirageOS 4 and deserve only one purpose: with that, we don't need to cut a release of tcpip after MirageOS 4 and we don't need to apply constraints.

However, as you can say, it seems that, at least for FreeBSD I believe, this patch does not work.

So, to clear the situation, without any solution:

  1. when MirageOS 4 will be released it will be incompatible with this version of tcpip
  2. we need to make a release of tcpip to be able compatible with MirageOS 4 and incompatible with MirageOS 3

For my perspective, may be it's the time to delete checksum stubs on mirage-solo5/mirage-xen?

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Feb 19, 2021

to avoid conflicts / unnecessary churn, I propose:

  • (a) rename the C symbols used by tcpip_checksum and add the freestanding "cross-compilation" runes, release tcpip (this will now work with mirage 3 and mirage 4)
  • (b) remove the old checksum stubs from mirage-solo5 and mirage-xen

The renaming leads to old mirage-solo5 and new tcpip continues to be symbol-conflict-free linkable (old tcpip releases will need to conflict with new mirage-solo5/mirage-xen (those with the checksum_stubs removed)).

overall, it is not clear to me what the impact is: what about cstruct stubs and io-page stubs? anything else that is now in mirage-solo5/mirage-xen, and should instead be in the respective library that uses it?

@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Feb 19, 2021

overall, it is not clear to me what the impact is: what about cstruct stubs and io-page stubs? anything else that is now in mirage-solo5/mirage-xen, and should instead be in the respective library that uses it?

We did not find a problem for cstruct mostly because mirage-tcpip has a sub-package tcpip.unix which takes C stubs - otherwise, you have missing symbols for a simple executable. This is not the case for cstruct which takes C stubs by default and I believe that io-pages has some trouble.

@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Feb 25, 2021

I prepended C functions with mirage_, I can take care about the release. As far as I can tell, a minor one should be enough.

@dinosaure dinosaure force-pushed the compatible-c-layout branch from 605c928 to 2b29b0c Compare March 16, 2021 12:05
@dinosaure dinosaure mentioned this pull request Mar 16, 2021
Closed
@hannesm
Copy link
Copy Markdown
Member

hannesm commented Mar 17, 2021

to me this change looks good. should we move forward and remove the tcpip.unix package entirely? (this would be a breaking change, but I think it is fine - there are barely users of tcpip.unix AFAICT)

@hannesm hannesm mentioned this pull request Mar 17, 2021
Closed
29 tasks
@hannesm
Copy link
Copy Markdown
Member

hannesm commented Mar 17, 2021

I looked, and tcpip.unix is hardcoded e.g. in the mirage tool. While with this PR this sublibrary does not serve any purpose anymore, let's keep it until the next major release (or until we have a mirage released that does not depend on tcpip.unix).

hannesm
hannesm approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@hannesm hannesm left a comment

Choose a reason for hiding this comment

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

LGTM. Please merge and release when CI is happy.

@hannesm hannesm force-pushed the compatible-c-layout branch from 3cf50af to 97d3fed Compare March 17, 2021 15:51
@dinosaure dinosaure merged commit d3d6f40 into mirage:master Mar 17, 2021
@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Mar 17, 2021

Thanks for the further work!

dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 17, 2021
@dinosaure
[new release] tcpip (6.1.0)
661b80c 
CHANGES:

* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
@dinosaure dinosaure mentioned this pull request Mar 17, 2021
Closed
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 26, 2021
@dinosaure
[new release] tcpip (6.1.0)
dbbe1e2 
CHANGES:

* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 26, 2021
@dinosaure
[new release] tcpip (6.1.0)
8dc4803 
CHANGES:

* checksum stubs: Drop `caml_` from their name (@hannesm, mirage/mirage-tcpip#445)
* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 29, 2021
@dinosaure
[new release] tcpip (6.1.0)
26907b3 
CHANGES:

* checksum stubs: Drop `caml_` from their name (@hannesm, mirage/mirage-tcpip#445)
* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
@dinosaure dinosaure mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hannesm hannesm hannesm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dinosaure @hannesm