Lwt: [easy-ish] Reorganize the C files

[aantron]

Lwt's C files are currently quite messy. For example, lwt_unix_unix.h is currently about 3300 lines of stress-inducing C code, which is enough to give anyone a headache. Also, it shouldn't be a header file – everything in it is an implementation. However, it is currently included from lwt_unix_stubs.c as a janky form of conditional compilation.

Fixing that conditional compilation scheme might be somewhat involved, but the first thing we should do is split up lwt_unix_unix.h and lwt_unix_windows.h into lots of little files, close to one per C call or small C call category. For comparison, see the files of Unix from the standard library, and the Windows equivalent.

We can create all those little files in two new subdirectories of src/unix/ (one for actual Unix and one for Windows), and, as a first cut, #include each one of them into either lwt_unix_unix.h and lwt_unix_windows.h. This should be relatively simple, as C preprocessor inclusion is textual substitution. Header guards (#pragma once or #ifndef) are not necessary, because we know each of these files is only going to be included once, and also because the ultimate plan is to turn them into .c files eventually.

It would be great if we could figure out how to make git trace the history of each one of these files back to lwt_unix_unix.h or lwt_unix_windows.h, but I doubt git is actually able to do that.

If you'd like to work on this, please leave a comment below. We want to make sure that nobody else is likely modifying these files at the same time, to avoid a potentially brutal merge conflict.

[andrewray]

I am sure this is possible, however, a word of caution -- I changed the lwt_XXX_stubs.c file to a .h file to ensure jbuilder copied it when building (in fact it actually installs them as well).

I believe the more flexible way to deal with C code is to define it in a separate library.

[cedlemo]

@aantron, I really would like to try to work on this if you don't mind.

[aantron]

@cedlemo Absolutely I wouldn't mind! Quite the opposite, it would be VERY welcome :)

[cedlemo]

Nice !

I will start with some questions:

1. the destination directories where the little files should go:

We can create all those little files in two new subdirectories of src/unix/

Something like lwt/src/unix/unix/ and lwt/src/unix/windows/
or lwt/src/unix/unix_stubs/ and lwt/src/unix/windows_stubs/

1. Could you explain me what the following sentence imply ?

I believe the more flexible way to deal with C code is to define it in a separate library.

1. Do you want a big pull request (PR) with all the changes or should I do multiple pull requests (step by step)?

Like the first PR will, at least have :

• the directories created,
• first file src/unix/unix_stubs/readable.c (for the readable C binding ).
• And that the tests works (make test, I guess that this is the best way to check that it works ?)

[aantron]

Ok :)

1. Either of those options seems reasonable.

2. I am not sure what @andrewray meant by that sentence.

3. It's up to you. You could do one small PR showing a minimal version of the change, to practice and get some early feedback, followed by one big PR finishing the rest of it. As long as it's reasonably reviewable, it doesn't really matter, so do whatever seems easiest or most aesthetic for you :)

[cedlemo]

@aantron,

After some tests, it seems that it will not be as simple than that :).

• First jbuilder does not like sub-directories, for example the only way to make it works (for now), is to copy the file unix_c/readable.c in the root dir with jbuild copy_files stanza and define it as a c file to compile in jbuild here is the diff.

diff --git a/src/unix/jbuild b/src/unix/jbuild
index 3489f0b..39096ba 100644
--- a/src/unix/jbuild
+++ b/src/unix/jbuild
@@ -81,6 +81,8 @@
 ;; main library
 ;; Lwt_unix_jobs_generated and Lwt_config should be hidden

+(copy_files unix_c/*.c)
+
 (library
  ((name lwt_unix)
   (public_name lwt.unix)
@@ -118,6 +120,7 @@
     lwt_unix_job_tcsendbreak
     lwt_unix_job_truncate
     lwt_unix_job_unlink
+    readable
   ))
   (install_c_headers (
     lwt_config
diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h
index 67c39bc..010c6ed 100644
--- a/src/unix/lwt_unix_unix.h
+++ b/src/unix/lwt_unix_unix.h
@@ -44,16 +44,7 @@
 /* +-----------------------------------------------------------------+
    | Test for readability/writability                                |
    +-----------------------------------------------------------------+ */
-
-CAMLprim value lwt_unix_readable(value fd)
-{
-    struct pollfd pollfd;
-    pollfd.fd = Int_val(fd);
-    pollfd.events = POLLIN;
-    pollfd.revents = 0;
-    if (poll(&pollfd, 1, 0) < 0) uerror("readable", Nothing);
-    return (Val_bool(pollfd.revents & POLLIN));
-}

 CAMLprim value lwt_unix_writable(value fd)
 {

• Secondly include c sources directly in h seems bad practice in C.

Now I think I start to understand the idea of @andrewray of creating a C lib, maybe we should think of something like this :

/src/unix/unix_c/
              |____ jbuild
              |____ unix_c.h
              |____ readable.c
              |____ other files

You must have a better understanding of C and jbuilder than me tell me if this make sense to you.

[aantron]

Ack.

Actually, I have no experience with Jbuilder and C files. In fact, I'm not sure who in the community does at this point.

If you want to experiment with that, I'd be very happy to know the results. On the other hand, if you want to just split up the header files into lots of little ones, even that would be a big improvement.

[cedlemo]

If you want to experiment with that, I'd be very happy to know the results.

Yes I would like to see if I can make it works.

[cedlemo]

@aantron ,

just to confirm something I thought : jbuilder does not support subdirectories for stubs.

• https://github.com/janestreet/jbuilder/issues/34
• https://github.com/janestreet/jbuilder/issues/242

So the right way to reorganize the C files is to put them in a sub-directory and copy them in the main directory via the jbuild stanza (copy_files unix_c/*.c).

But it seems that there is a problem that will appear when we separate the lwt_unix_unix.h and lwt_unix_windows.h file in multiple files: the conditionnal compilation based on the system used.

In the original form:

• in the jbuild for the lwt_unix library part, there is the c_names stanza that indicate that lwt_unix_stubs (lwt_unix_stubs.c) should be compiled.
• lwt_unix_stubs.c has the following part which will include some code based on the system :

#if defined(LWT_ON_WINDOWS)
#include "lwt_unix_windows.h"
#else
#include "lwt_unix_unix.h"
#endif

But now if we do like I proposed:

• multiple files in unix_c and windows_c
• add both (copy_files unix_c/*.c) and (copy_files windows_c/*.c) stanzas
• append each windows and unix files in the c_names stanza

We lost the conditionnal compilation based on the system used and the build phase will fail.
It looks like it is not the good solution.

I have not tested the idea of creating a lib but I found an example that maybe could be interesting :

the re2 lib.

It looks like that in this lib, they have a subdiretory with the C lib that they build with a jbuild in the subdirectory with custom rules https://github.com/janestreet/re2/blob/master/src/re2_c/jbuild.

[aantron]

BTW, don't hesitate to poke the Jbuilder maintainers if you find something too limiting :)

[cedlemo]

@aantron,

I send 2 PRs in order to show you 2 possible simple solutions.

I started to see if I can use custom build rules like in the re2 lib and create a C libs, but it looks like it will be rather complicated.

Could you, please, look at the PRs and tell me if like one of them ? If needed, I will ask questions on the jbuilder github in order to see if there is better ways to do what you want.

[aantron]

Thanks. It looks like both have advantages and disadvantages:

For combining both systems in one file:

• It's harder for people to forget to add a Windows version of something.
• There won't be lots of annoying little files for everything that is not implemented.
• Less things to list in jbuild.

For having things in separate files:

• We don't have to have a 1:1 correspondence between the Windows and Unix files. We could just list all the unimplemented Windows system calls together in one .c file.

I weakly prefer the separate files for separate systems approach, as long as we don't have lots of little files containing one unimplemented Windows function macro each. However, it's not a strong preference, so if one way is much easier with Jbuilder, then please feel free to go that way :)

I see the AppVeyor builds are failing on MinGW – that is our only "real" Windows CI build. Let me know if you run into difficulties fixing that later, I'll try to help.

[cedlemo]

I weakly prefer the separate files for separate systems approach

Fine, I will work on this one and create a not_available.c file (tell me if you have a better idea for the name) in the windows_c directory.

Let me know if you run into difficulties fixing that later, I'll try to help.

Yes, I thought that the problem was the LWT_ON_WINDOWS macro that was not defined because the "lwt_config.h" was not included but I see that it is still failing with

# unix_writable.c:7:18: fatal error: poll.h: No such file or directory
#  #include <poll.h>
#                   ^
# compilation terminated.

while the code is

#include "lwt_config.h"

#if !defined(LWT_ON_WINDOWS)
#include <caml/mlvalues.h>
#include <caml/unixsupport.h>
#include <caml/version.h>

#include <dirent.h>
#include <poll.h>
#include <sys/resource.h>
#include <sys/time.h>
#include <sys/uio.h>
#include <sys/un.h>
#include <sys/wait.h>
#include <unistd.h>
#include "lwt_unix.h"

CAMLprim value lwt_unix_writable(value fd)
{
    struct pollfd pollfd;
    pollfd.fd = Int_val(fd);
    pollfd.events = POLLOUT;
    pollfd.revents = 0;
    if (poll(&pollfd, 1, 0) < 0) uerror("writable", Nothing);
    return (Val_bool(pollfd.revents & POLLOUT));
}
#endif

I don't really understand why the check !defined(LWT_ON_WINDOWS) is valid.

[aantron]

Your diagnosis was correct, it's just that you added #include "lwt_config.h" only to _readable.c but not _writable.c.

[cedlemo]

hum,

It seems that I did it for both files : https://github.com/ocsigen/lwt/pull/505/commits/498d3926ddf5e3e890e113613df93346fcfd8608

and https://github.com/ocsigen/lwt/pull/506/commits/db9cb7e950a0818ac89c9e2befb663ebc9886bf5

[aantron]

Those commits are each in a separate PR though :) To take one PR only, if you look at the net diff from #505, for example, you'll see that unix_writable.c doesn't have lwt_config.h included: https://github.com/ocsigen/lwt/pull/505/files#diff-72c30620819dd0f692480f65e99e778e, and correspondingly the AppVeyor error before the last commit was a failure in unix_readable, and after the last commit it was in unix_writable, because now that file is the first one still missing lwt_config.h.

[cedlemo]

Ok at least now I know where is the problem. ;-)

I send you a new clean PR with a few functions, you review/validate it and then I do the rest in one PR after that. Is that all right with you?

[aantron]

That sounds great :)

[cedlemo]

@aantron ,

you will find the first PR here #508

For the copyrights, I recopied the lwt_unix_unix.h copyright in the unix C files and the lwt_unix_windows.h copyright in the windows C files.

For the windows files, I have not added any header files, I have not found any windows related headers in the lwt windows sources (I am used to Linux platforms but not to Windows ones), could you tell me how the compilation find the windows header ?

[aantron]

Thanks, I replied in the PR. The thing with the copyrights sounds right :)

I'm assuming you're referring to the caml/ headers that are currently probably missing from the Windows files. I'm not sure what your question is, though. Headers are found the same way as on Linux. If my guess is correct, you're referring to lwt_unix_windows.h not including any other headers from inside it. That's because lwt_unix_windows.h is itself included in a larger file lwt_unix_stubs.c (a really janky form of system-specific configuration). lwt_unix_windows.h uses the includes that are above where it is included in lwt_unix_stubs.c.

I removed the easy label from this issue, because the splitting into .c files approach is arguably not so easy (and thanks for doing it) :)

[cedlemo]

lwt_unix_windows.h uses the includes that are above where it is included in lwt_unix_stubs.c.

That is what I understood but I looked for something like "windows.h" or other. For example, for the function WSAGetLastError in the windows implementation of lwt_unix_bytes_read when I make a google search in order to find the related header file I have got this https://msdn.microsoft.com/fr-fr/library/windows/desktop/ms741580(v=vs.85).aspx

I realise now that maybe the following headers are plateform independant because it uses gcc under mingw cygwin.

[aantron]

That is what I understood but I looked for something like "windows.h" or other

That's a good point. My guess is that the Windows platform headers are being pulled in transitively through the caml/ headers, in their versions on Windows. I'll confirm shortly. Relying on that is probably a bad idea, but fixing it is a whole other issue. However, you're welcome to use whatever Windows header you want directly to get the files to compile. They are found like on a Linux platform. The compiler on Windows (whether MSVC or MinGW) will have the platform headers in the include path.

I realise now that maybe the following headers are plateform independant because it uses gcc under mingw cygwin.

Lwt also builds with MSVC, but those headers are platform independent because they are part of the C standard library. Both MSVC and MinGW will have implementation of them, and they will be in the include path. This is true for Clang on macOS as well, so it's not because of GCC.

[aantron]

Yeah, confirmed about the transitive includes. lwt_unix_stubs.c includes lwt_unix.h which includes caml/unixsupport.h, which on Windows includes, among other things, winsock2.h, which, as you found, is where WSAGetLastError is declared:

https://github.com/ocaml/ocaml/blob/30e6aa9f1fdb552b538350180316c90d24955a36/otherlibs/win32unix/unixsupport.h#L20-L26

You could find chains like this for all the other functions, I'm sure.

[cedlemo]

I added the missing headers and I tried to build lwt on Windows in a VM and it seems to be Ok.

I was searching for issues to work on after that (#395 seems interesting) and I saw that the Lwt_unix module is not tested (https://github.com/ocsigen/lwt/issues/385#issuecomment-320372541), have you created an issue for this too ?

[aantron]

Yep, the Windows build is passing now. Just have to add 1 line to fix the Cygwin one (see PR).

Fixing #395 would be quite useful, but be sure to leave a comment there, because @jsthomas seems to currently be on a roll adding tests, so to avoid doing the same work :)

Adding tests for Lwt_unix is some important future work, but no, I haven't created the issue yet, because I wasn't ready to hyperlink it, and I wasn't sure anyone was ready to take something like that on anyway. You're very welcome to create it and/or start working on it, and I can add any additional information later in a comment on it.

Having unit tests for Lwt_unix is going to be very important if/when we are finally porting Lwt_unix to libuv (#328), because we will want to know and preserve the exact behavior of pretty much each function.

[cedlemo]

Having unit tests for Lwt_unix is going to be very important if/when we are finally porting Lwt_unix to libuv (#328), because we will want to know and preserve the exact behavior of pretty much each function.

Ok, I will let @jsthomas work on #395.

and I wasn't sure anyone was ready to take something like that on anyway

why? is it too difficult (I have not enough insight) or not really interesting ?

[aantron]

Well, he might not be working on it :)

why? is it too difficult (I have not enough insight) or not really interesting ?

It's not too difficult, but it is more difficult than tests for the Lwt "core" (Lwt, Lwt_mutex, etc.), because some behaviors may be platform-specific, you might not have some of the functions available on a given platform. The behaviors are more complex, less portable, there is the risk of needing conditional compilation. A lot of people don't know C, or don't like working with it, and writing good tests for Lwt_unix will require reading the C code and thinking about what it's doing, even though the tests themselves will be written in OCaml only. We don't currently have coverage analysis set up for C. That would require something like gcov. And so on.

Given there are simpler tasks to do, I figured people aren't rushing to create the Lwt_unix test suite at this point :p

[jsthomas]

I was planning to take on #395 next (but I don't want to monopolize all the testing fun, so let me know if you want to steal :) ). Normally I'd post my interest on an issue before I start working it, but #452 and #463 were so small that I just decided to go for them.

[cedlemo]

First I have to finish this, by Sunday at the least I think, so @jsthomas if you want to do #395 before I don't mind.

@aantron, if you have some tasks with higher priorities and maybe less difficult than the tests for Lwt_unix then let me know.

FYI I like C and I know a little about language bindings but just enough to participate/help you and not to create alone from start to end all the Lwt_unix tests.

[cedlemo]

@aantron,

I have almost finished reorganizing the C files, but the tests/ build fails on the cygwin environment and I can not see why.

Here is the output :
https://ci.appveyor.com/project/cedlemo/lwt/build/28/job/rsbh3kr3gr0a3fty

And the file that fails to build:
https://github.com/cedlemo/lwt/blob/reorg_lwt_unix_c_files_part_2/src/unix/unix_c/unix_gethostbyaddr_job.c

As you can see, the content of the file should not be compiled on Windows system (#if !defined(LWT_ON_WINDOWS)) but it generates an error.

edit :

I have finished to reorganize the C files, it just remains the little pb in the Cygwin env to be fixed.

[aantron]

Humm, I get similar errors on macOS as well:

During compilaton:

unix_gethostbyaddr_job.c:65:20: warning: implicit declaration of function 'hostent_dup' is invalid in C99 [-Wimplicit-function-declaration]
        job->ptr = hostent_dup(job->ptr);

During linking of the test suite:

Undefined symbols for architecture x86_64:
  "_hostent_dup", referenced from:
      _worker_gethostbyaddr in liblwt_unix_stubs.a(unix_gethostbyaddr_job.o)
  "_hostent_free", referenced from:
      _result_gethostbyaddr in liblwt_unix_stubs.a(unix_gethostbyaddr_job.o)
ld: symbol(s) not found for architecture x86_64

I'm looking into it.

I also get a ton of warnings about undeclared memcpy and strdup (for which string.h is necessary).

[aantron]

To take hostent_dup as an example, it was a static function originally defined in lwt_unix_unix.h, and the commits move it to unix_gethostbyname_job.c. However, it is also referenced in unix_gethostbyaddr_job.c, so it should probably be factored out into a common header they both include. That could be lwt_unix.h, or the new unix_get_network_information_utils.h.

[cedlemo]

o_o' sorry, I missed this one, I will fix that.