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

Allow the Esperanto support for lwt #945

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

dinosaure
Copy link
Member

This PR wants to fix the support with the Esperanto & Cosmopolitan. Esperanto is an OCaml toolchain which allows us to compile an OCaml application. The resulted application (an executable) should run anywhere (Linux, Mac, Windows and BSD family).

Currently, a simple project in OCaml can be compiled with Esperanto but a larger project which probably uses lwt can not be compiled because few details. This PR wants to fix these details and allow us to take an OCaml application which depends on lwt and be able to build-once an application which should run-anywhere.

This PR is a draft because a special commit (the second to last) is probably the most invasive and dependant to Esperanto but I can probably find an other situation than what I proposed. Feel free to comment the rest of commits which are valid for us for the Esperanto toolchain. I tried to be less invasive as I can.

@dinosaure
Copy link
Member Author

From this PR, this simple code works for Ubuntu:

open Lwt.Infix

let rec full_write fd buf off len =
  Lwt_unix.write fd buf off len >>= fun len' ->
  if len - len' > 0
  then full_write fd buf (off + len') (len - len')
  else Lwt.return_unit

let tmp = Bytes.create 0x1000

let rec cat () =
  Lwt.catch begin fun () ->
    Lwt_unix.read Lwt_unix.stdin tmp 0 (Bytes.length tmp) >>= fun len ->
    match len with
    | 0 -> Lwt.return_unit
    | len -> full_write Lwt_unix.stdout tmp 0 len >>= cat
  end @@ function
  | End_of_file -> Lwt.return_unit
  | exn -> raise exn

let () = Lwt_main.run (cat ())

If you want to try, you need to create a dune file, a dune-workspace and a opam file:

$ cat >dune <<EOF
(executable
 (enabled_if
  (= %{context_name} esperanto))
 (libraries lwt.unix)
 (modules cat_lwt_unix)
 (name cat_lwt_unix))

(rule
 (target cat_lwt_unix.com)
 (enabled_if
  (= %{context_name} esperanto))
 (mode promote)
 (deps cat_lwt_unix.exe)
 (action (run objcopy -S -O binary %{deps} %{target})))
EOF
$ cat >dune-workspace <<EOF
(lang dune 2.0)
(context (default))
(context
 (default
  (name esperanto)
  (toolchain esperanto)
  (merlin)
  (host default)))
EOF
$ cat >cat_lwt_unix.opam <<EOF
opam-version: "2.0"
name:         "cat-lwt-unix"
maintainer:   [ "Romain Calascibetta <[email protected]>" ]
authors:      [ "Romain Calascibetta <[email protected]>" ]
homepage:     "https://github.com/dinosaure/esperanto"
bug-reports:  "https://github.com/dinosaure/esperanto/issues"
dev-repo:     "git+https://github.com/dinosaure/esperanto"
doc:          "https://dinosaure.github.io/esperanto/"
license:      "MIT"
synopsis:     "cat tool with lwt and compiled with esperanto"
description:  "cat tool with lwt and compiled with esperanto"

build: [
  [ "dune" "build" "-p" name "-j" jobs ]
]
install:  [
  [ "dune" "install" "-p" name ] {with-test}
]

depends: [
  "ocaml"           {>= "4.08.0"}
  "dune"            {>= "2.6.0"}
  "lwt"
]

pin-depends: [
  [ "lwt.dev" "git+https://github.com/dinosaure/lwt.git#69419f4352d1ae0e60039275ec9da893c505e2c6" ]
]
EOF

Then, to be able to cross-compile this little project, you must use opam monorepo to compile dependencies into the same context:

$ opam pin add -y https://github.com/dinosaure/esperanto.git
$ opam monorepo lock
$ opam monorepo pull
$ dune build
$ bash -c "echo 'Hello World!' | ./cat_lwt_unix.com"
Hello World!

@dinosaure
Copy link
Member Author

This PR is definitely ready:

  1. it seems that the behavior of discover.exe is the same as before - however, many issues persist about the cross-compilation but I don't have the time to look deeper on that. At least, I added some comments to explain what is going on. The result is: we can build an Esperanto/Cosmopolitan application only if libev is not installed (otherwise, lwt will try to bring it even if we want to compile everything into a cross-compiled context).
  2. The example above still work on my machine
  3. Each commits are well commented for further lecture
  4. The CI fails for other reasons than this PR

If you have any question, don't hesitate but I hope that I clearly described everything!

@raphael-proust
Copy link
Collaborator

Thanks for this PR! I like the idea behind esperanto/cosmopolitan very nice! Some comments about the MR specifically:

  • It touches the part of Lwt that I'm not familiar with and not really competent to check (I haven't done any C in too long, I would need a refresher).
  • If we go this way (which I'd be inclined to do, but would like additional opinions as well) we probably need some new tests in the CI to actually check the portability. It's not needed right now though, we can wait until it's a feature we announce we support.
  • The CI failure is on windows and I don't have a machine to try to reproduce things on. They seem to be related to opam build of ocaml-config and a checksum fail at that. So probably some opam repo issue. I've restarted the jobs to see if it's transient. (EDIT: they are not transient, I'll have to check that separately)
  • There are plans to drop support for some old versions of OCaml (keeping only 4.08+). This should simplify the code somewhat, and especially the C bindings which are full of #if OCAML_VERSION …. I don't think it will interfere with your PR but something we might want to keep in mind. It might be useful if/when esperanto supports more OCaml versions.

Re: libev
It might be related to e9a9c7b and you might have to pass --libev-default=false explicitly to dune exec src/unix/config/discover.exe. I haven't tinkered with this yet, but I can have a more in-depth look if you need me to.

@dinosaure
Copy link
Member Author

It might be useful if/when esperanto supports more OCaml versions.

Currently, Esperanto support 4.12, 4.13 and 4.14 (MirageOS support) but I can not go further when it requires more time that I don't really have.

@raphael-proust
Copy link
Collaborator

Currently, Esperanto support 4.12, 4.13 and 4.14 (MirageOS support)

Nice! I got the 4.14 support from the Esperanto README:

Currently, we only support OCaml 4.14

This might need updating.

Otherwise I had a longer look and I don't think the two changesets will conflict. So we should be good on that front.

@dinosaure dinosaure force-pushed the esperanto branch 12 times, most recently from ddd176a to bda6db2 Compare June 15, 2022 09:27
@dinosaure
Copy link
Member Author

I added a CI which compile and run a simple LWT program with esperanto toolchain on Ubuntu (latest) and it's work. I would like to clarify only one point: lwt + esperanto does not work, for the moment, on Windows due to a lack of portability of the pthreads library (the issue is here dinosaure/esperanto#11 for more details). A possible solution was found however, but I need much more times to complete this task.

Regardless the Windows support, this PR is ready - however, you should get some conflict with #953.

@jart
Copy link

jart commented Sep 30, 2022

@dinosaure Cosmopolitan Libc has pthreads support now. https://github.com/jart/cosmopolitan/releases/tag/2.1 If there's an issue with setcontext() and getcontext() that are causing issues for fibers, then let me know and I'll fix it.

@dinosaure
Copy link
Member Author

Currently, this PR can compile a simple program. However, the example (a simple cat) hangs at the end. I will try to debug that as soon as I can.

In some contexts, the C compiler can integrate few -I and -L and pthread
becomes standalone from the user's point-of-view. However, in the context of
a C cross-compiler, standalone pthread's definitions can be different than the
host system (available into [default_search_paths]) and they can clash.

This patch allow us to compile a C code without any flags and let the
C (cross?)-compiler to solve by itself where is pthread. This behavior is added
from what src/unix/config/discover.exe did before.
This patch use if/else if/else instead of a switch ... case which
expects constants. These values are not constants from the Cosmopolitan
libc which set these variables at the "boot" of the application and set
them with the right value according to the running system.
Instead of a static & global array, we moved the array of MADV_* values into
the function which uses it.
Instead of a static & global array, we moved the array of TC* values into
the function which uses it.
Instead of a static & global array, we moved the array of WNOHANG & WUNTRACED
values into the function which uses it.
This patch create 2 C functions to initialize an array with speed values
or termios values. If we pass NULL to these functions, they returns the
length of the array. By this way, we initiate required arrays which
contains speeds & termios value at the runtime - instead of the compile
time.

This patch is needed because these values are not constants for the
Cosmopolitan libc and we must set/initialize arrays at runtime.
This patch is probably the more invasive about Esperanto/Cosmopolitan. It adds
a new compilation path when [__ESPERANTO__] is defined. A note was added to
explain where it comes from: the cross-compiler [arch-esperanto-none-static-cc]
systematically defines [-D__ESPERANTO__] in order to let third-party libraries
(such as lwt) to choose different compilation paths. And here we are!

Cosmopolitan does not defines required [_SC*] constants and
[unix_get_pw_gr_nam_id_job] can not be compiled. We discard this piece of code
when we want to compile with Cosmopolitan as it's the case for Android too.
The default case of [C_library_flags.detect] puts systematically
[-I/usr/include] and [-L/usr/lib] even in a cross-compilation context which is
wrong because [/usr/include/pthread.h] can clash [cross-env/pthread.h] for
instance.

The default case about underlying libraries (libev or pthread) should be
available only from a restrictive set of flags instead of a pervasive one. This
patch restricts this set to what we really need instead to pervasively picks
some random flags.

A clarification about the recognition of [pthread] was made too to really
understand the behavior of [discover.exe] and give a chance to be more
reproducible afterwards
If we consider pthread as the default implementation needed by lwt, we
should prioritize the recognition of [pthread] before [libev] but more
concretely, due to the undeterministic behavior of [discover.exe] and
our usage of [C_library_flags.set_{c,link}_flags] to reset any flags
when we can link [pthread] from a cross-compiler, it seems that we reset
aggregate flags needed for [libev] (specially [-lev]) in the same time.

This patch aggregate flags needed for [pthread] first and let the
recognition of [libev] then. However, that mostly means that in the
context of the cross-compilation, the user must NOT install
[conf-libev] (otherwise, we will try to link with the host's [libev]
which is obviously incompatible with our cross-compiler). This patch
wants to keep, as much as we can, the same behavior - but it highlights
limits of [discover.exe].
The last release of esperanto does not define speed_t constants. We must
protect the definition of our speed_t table when the toolchain is
esperanto.
Currently, the Cosmopolitan/Esperanto toolchain protects the usage of
pthread and we are not able to pass to it 0. This patch allows us to
compile (and recognize the support of pthread) the code with the
esperanto toolchain.
@dinosaure
Copy link
Member Author

This PR is actually ready, the CI works on my side and we are able to compile & run a Cosmopolitan binary with lwt 🎉. Feel free to redo a final review.


static struct {
struct speed_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you should be re-defining speed_t here. It's in the libc.

Data Type: speed_t

The speed_t type is an unsigned integer data type used to represent line speeds. 

https://www.gnu.org/software/libc/manual/html_node/Line-Speed.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to look on it but keep in your mind that we currently cross-compile lwt with the Cosmopolitan C library. The GNU C library is not involve anymore in the case of the esperanto toolchain. A #define will be appropriate to drive the compilation according to the target.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my bad, I got confused! You're defining struct speed_t, but posix defines speed_t as a typedef to an unsigned integer type. They're two different namespaces, all is fine. Reusing speed_t is not great IMO, but works (although names ending in _t are reserved).

{B150, 150},
};

long _speedtable(struct speed_t dst[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking names beginning with an underscore (_) are reserved by the C implementation.

default:
return 0;
}
long _terminal_io_descr(long dst[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

void encode_terminal_status(struct termios *terminal_status, volatile value *dst)
{
long *pc;
int i;

long _NSPEEDS = _speedtable(NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

And there too.

@MisterDA
Copy link
Contributor

Might be interesting to checkout this patch to GCC, allowing compilation of unmodified code with Cosmo, by compiler rewriting uses of preprocessor constants.
https://ahgamut.github.io/2023/07/13/patching-gcc-cosmo/
Does Lwt builds unmodified with this patch?

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.

4 participants