Move native code into posixlib and clib respectively #1885

ekrich · 2020-08-29T02:42:02Z

The overall goal here is to move the C code into the corresponding library. We want to consider supporting Windows so allowing POSIX to be only available on UNIX like platforms makes sense.

This is just a first cut. We need to add pthread.c and a little more possibly that does not belong in the nativelib. Also, this will need to be repeated for clib. Most of the POSIX code has a xxx.scala file and a corresponding xxx.c file. clib was coded earlier before better conventions came along.

There is a question whether posixlib and the nativelib should exportJars := true in sbt. Removing this will still work fine and maybe build a little faster but to use the SNAPSHOT locally, publishLocal would have to be run. Let me know what you think about this.

LeeTibbert · 2020-09-01T12:20:33Z

Eric,

Thank you for stepping into this swamp and encouraging a healthy ecosystem.

Did you find a good decision rule for what should be in posixlib and what in clib.
The reason that I ask is that I seem to remember doing a PR which later got
overwhelmed by events which tried to move posixlib C files into a subdirectory
of resources. I think the intent was that once those had been coralled then the
clib C files into another: an early & aborted attempt similar to what you describe
here.

I seem to remember that I could find no clear separation of the two. That is
posix and c standards have become co-mingled over the years. IIRC, glibc
tends to implement posix routines and a more. Recent posix standards tend
to refer to the corresponding IEEE C standard (and describe what to do if
there are conflicts).

I ran aground because I did not want to duplicate implementations and could not find a way
in GitHub to use links.

I think the same chaotic boundary exists in the .rst documentation.

It would be nice to have (read: IMHO, essential) to have a clearly stated
decision rule to guide both SN and application developers.

That is, which library is more primitive and can the less primitive be implemented in terms
of the primitive? I believe clib is intended to be the primitive. Current posixlib impementations
may not be done in terms of the (intended underlying?) clib.

Pardon me if such exists and I missed it or was not clever enough to suss it out.

LeeTibbert · 2020-09-01T12:59:59Z

I believe there are at least 4 PRs in the queue which reference posixlib and which might
be affected by this PR. PR #1635 is especially complicated and provides a major capability.
It requires two PRs which are awaiting merge, both of which may be affected.

Not to hold up the base PR, but I think it would ease the overall effort if those earlier PRs
were considered before this one. Time is what keeps everything from happening at once.

ekrich · 2020-09-01T15:47:59Z

There are basically three parts to the situation.

There are the C routines that are needed in nativelib so that it doesn't depend on clib and visa versa. That is here.
https://github.com/scala-native/scala-native/blob/master/nativelib/src/main/scala/scala/scalanative/runtime/libc.scala There is also any C code referenced in this area that needs to stay in nativelib. https://github.com/scala-native/scala-native/tree/master/nativelib/src/main/scala/scala/scalanative/runtime
There is the C/C++ standard library that is very well defined. https://en.cppreference.com/w/c/header
Then there is POSIX. @LeeTibbert, myself and others learned early on that we needed to follow the specification and certain guidelines that I think are mostly in our heads or written on other PRs. Latest is here. https://pubs.opengroup.org/onlinepubs/9699919799/

It can be very confusing as looking back and forth between the specs and the code and between the C and POSIX specs can get dizzying but it can be separated as far as I know.

If we move the code to the posixlib and the clib it should be easier the tease apart any code that is in the wrong place especially when the Windows port (hopefully someone will step forward) gets started in earnest.

ekrich · 2020-09-01T15:53:18Z

@sjrd and others. Do we want the nativelib, posixlib, and clib to omit the exportJars := true in build.sbt?

We would need to publishLocal if we wanted to use the local SNAPSHOT and I would need to add some documentation in the Contributor section to that effect.

LeeTibbert · 2020-09-01T16:05:38Z

I concur that sorts based on experience gained and current knowledge & directions is the way to go.
In the medium range, as we work on posixlib stuff, we may want to see if it can be implemented as
a layer on top of clib. At one point, I put cross-pointers in the clib & posixlib .rst files/tables to
try and get a clean, or at least documented, description of what was expected in each library.

ekrich · 2020-09-29T23:57:15Z

@sjrd Do you think we could add this PR and a follow on for clib into the release so at least we get the associated native C code in the correct place?

The dependency chain currently javalib -> posixlib -> clib but technically it should be javalib to both. Then portions of the javalib once split would depend on posixlib or clib but not both.

The small dependency between posixlib and clib has been removed.

ekrich · 2020-10-02T18:33:39Z

I changed this to draft to address clib and any other changes needed.

ekrich · 2020-10-04T20:15:02Z

This now contains potential breaking changes to downstream applications.

Some items are in clib and not in posix lib as per the specs
Some posixlib items have been moved to the correct file so imports may need changing

Overall, we want to strive for correctness so I see no way around this as macro defs can only be in defined one place so deprecation is not possible. C is a flat name space.

ekrich · 2020-10-05T16:58:03Z

TODO - determine what should be in runtime package versus moved to javalib:

[X] Determine package for SocketHelpers, maybe posix.util?

My recommendation is that this gets moved to javalib when supporting Scala and C code gets moved. We do not want to maintain this as part of posixlib and it shouldn't be a public API. Right now it is in the scalanative.runtime package so that should be good for now.

[X] Do we want time and Java specific portions of Platform in runtime or javalib?

This should be determined by future PRs because how we segregate and organize code is a bigger issue and everything related to javalib should be moved into javalib.

Overall, as a rule, as long as the runtime does not depend on the code then the code should not be in nativelib.
A couple of other observations.

The handling of the optional jar/zip could be potentially handled as a special case in the javalib. Current handling by deleting of the code could maybe be done by filters looking for the code to link.
Potentially the libunwind code and the gc code could be handled separately, potentially in their own project. In particular, it should be easy to update the libunwind code potentially to match the minimum clang version.

LeeTibbert · 2020-10-14T11:55:23Z

nativelib/src/main/resources/scala-native/time_millis.c

+#define MICROS_PER_MILLI 1000LL
+
+    struct timeval tv;
+    gettimeofday(&tv, NULL);


I created issue #1948 to report that the error code for gettimeofday() is never checked.
I am not suggesting a change for this PR, just wanted to note that it had been detected.

Good idea. An error in C is like an error of death.

LeeTibbert · 2020-10-14T13:07:10Z

clib/src/main/resources/scala-native/errno.c

@@ -0,0 +1,11 @@
+#include <errno.h>
+
+int scalanative_errno() { return errno; }


float.c and math.c below seem to use the scalanative_libc_ prefix. That seems to be a useful convention. If not relevant here, to keep changes to a minimum, perhaps in a spin-off PR?

I think the scalanative prefix is good and we may need a libc too but this obviously is not totally agreed upon territory.

LeeTibbert · 2020-10-14T13:11:11Z

javalib/src/main/scala/java/io/File.scala

@@ -41,13 +41,13 @@ class File(_path: String) extends Serializable with Comparable[File] {
  }

  def canExecute(): Boolean =
-    Zone { implicit z => access(toCString(path), fcntl.X_OK) == 0 }
+    Zone { implicit z => access(toCString(path), unistd.X_OK) == 0 }


The change from fcntl.mumble to unistd.mumble in a number of places is a long overdue correction. Thank you!

I fixed everything I saw that was just wrong especially if I had to because of a dependency.

LeeTibbert · 2020-10-14T13:30:20Z

posixlib/src/main/resources/scala-native/errno.c

-// get the values out of those in a portable manner.
-
-// Omitting EDOM EILSEQ and ERANGE since they are duplicated in wrap.c
+// Omitting EDOM EILSEQ and ERANGE since they are in clib


I think that the comment and omission was (questionably) appropriate when the code was in nativelib but I believe it is wrong now. To my thinking, either you are posix complete (where that is another discussion altogether) or you are not. Those three are defined in posix, why make someone hare off to clib? Perhaps there could be defined
here as a call to the required clib version rather than duplicating code?

At the very least, I think this is worth a spin-off issue, to be corrected in detail and not hold up the overall PR.

I did not duplicate but if they have to be one place then they should be in clib. For certain clib should be complete but how we handle the overlap with POSIX in unanswered at this time.

This too was just moving things around to get them in the logical place. The code basically has a C file that is named the same as the Scala file except the extension when macros are needed. We didn't always early on have a plan - stuff was thrown wherever.

LeeTibbert · 2020-10-14T13:38:59Z

posixlib/src/main/resources/scala-native/types.h

@@ -1,6 +1,8 @@
 #ifndef __TYPES_H
 #define __TYPES_H

+// types used in pwd.c and sys/stat.c


OK, why? I know the answer but that is surely the question that a new reader will ask.

I put that there so the reader could know it is used only. Edit: also I am hoping we can have named struct members so we can avoid most of this additional code.

LeeTibbert · 2020-10-14T13:41:29Z

posixlib/src/main/resources/scala-native/unistd.c

@@ -9,6 +13,16 @@ int scalanative_w_ok() { return W_OK; }

 int scalanative_x_ok() { return X_OK; }

+// SEEK_CUR, SEEK_END, SEEK_SET in clib stdio


Again, IMO, those three should be defined here, probably as calls to the clib version.
Independent duplication is also possible, if you/we want to avoid a dependency of posixlib on clib.

There is not specific dependency but I get the point - some of this discussed later.

I looked at this and they were suppose to be in POSIX stdio.scala or stdio.c and there isn't one at all so this was the minimal fix that was actually correct.

LeeTibbert · 2020-10-14T13:42:35Z

posixlib/src/main/resources/scala-native/unistd.c

@@ -1,6 +1,10 @@
 #include <unistd.h>
 #include "types.h"

+extern char **environ;


I did not check this file against the posix standard, but it looks better than what it
replaces.

Yes, this is better.

Signed-off-by: ekrich <ekrichardson@gmail.com>

…iles Add some missing macro constants

…ative#1885)" This reverts commit 39ef208.

ekrich force-pushed the topic/posix-native branch from beddb43 to 412d6e6 Compare September 22, 2020 22:36

ekrich force-pushed the topic/posix-native branch from 412d6e6 to e672a80 Compare September 29, 2020 22:56

ekrich requested review from sjrd and LeeTibbert September 30, 2020 15:51

ekrich mentioned this pull request Oct 2, 2020

Migrate native code to libc and posixlib #1879

Closed

ekrich marked this pull request as draft October 2, 2020 18:32

ekrich changed the title ~~Move native code into posixlib~~ Move native code into posixlib and clib respectively Oct 2, 2020

ekrich force-pushed the topic/posix-native branch from af133fe to bbfdba0 Compare October 3, 2020 22:58

ekrich marked this pull request as ready for review October 4, 2020 23:32

ekrich mentioned this pull request Oct 6, 2020

Add bindings for sendto(...) and recvfrom(...) for using datagram soc… #1921

Closed

ekrich force-pushed the topic/posix-native branch 2 times, most recently from 96bbd88 to 537350b Compare October 12, 2020 19:19

This was referenced Oct 14, 2020

Consider clock_gettime for macos and linux #1915

Closed

nativelib time.c does not check errors, particularly gettimeofday() #1948

Closed

LeeTibbert reviewed Oct 14, 2020

View reviewed changes

ekrich added 18 commits October 14, 2020 16:55

Move native code into posixlib

cab79eb

Move pthread.c from nativelib to posixlib

3666d96

Export jars for posixlib

b91480c

Fix nativelib to clib and javalib build dependencies

8408251

Move the posix code to the root of the scala-native dir

49b1273

Move complex to clib

b2cc8ad

Start moving and organizing C code

f457299

Move macros to float.c

11cc46a

Add math.c to clib, move wrap.c functions

49a04df

Signed-off-by: ekrich <ekrichardson@gmail.com>

Move signal macros to clib

5d855e5

Move stdlib macros to clib

f321708

Move final stdio macros from wrap.c

b23248c

Move additional files from nativelib to posixlib

af64b9a

Move contents of posix.c to errno.c and unistd.c and out of native lib

d4a2e2a

Refactor fnctl and unistd to put functions in the correct order and f…

978d747

…iles Add some missing macro constants

Fix F_OK macro

32db2ad

Detangle POSIX time from nativelib

8466469

Add stdio.h to get NULL

7be6f0f

ekrich force-pushed the topic/posix-native branch from 537350b to 7be6f0f Compare October 14, 2020 23:56

ekrich requested a review from WojciechMazur October 16, 2020 15:59

sjrd approved these changes Oct 19, 2020

View reviewed changes

sjrd merged commit f659a18 into scala-native:master Oct 19, 2020

ekrich deleted the topic/posix-native branch October 19, 2020 17:24

WojciechMazur mentioned this pull request Nov 6, 2020

Constant definitions missing in scala.scalanative.posix.unistd (0.4M2) #1621

Closed

vicopem pushed a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020

Move native code into posixlib and clib respectively (scala-native#1885)

39ef208

vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020

Revert "Move native code into posixlib and clib respectively (scala-n…

bceb312

…ative#1885)" This reverts commit 39ef208.

vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020

Revert "Move native code into posixlib and clib respectively (scala-n…

8ec8137

…ative#1885)" This reverts commit 39ef208.

ekrich mentioned this pull request Feb 22, 2021

move process_monitor.cpp to javalib #2167

Closed

ekrich added a commit to ekrich/scala-native that referenced this pull request May 21, 2021

Move native code into posixlib and clib respectively (scala-native#1885)

45e30dc

WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021

Move native code into posixlib and clib respectively (scala-native#1885)

789941e

Move native code into posixlib and clib respectively #1885

Move native code into posixlib and clib respectively #1885

ekrich commented Aug 29, 2020

LeeTibbert commented Sep 1, 2020

LeeTibbert commented Sep 1, 2020

ekrich commented Sep 1, 2020

ekrich commented Sep 1, 2020

LeeTibbert commented Sep 1, 2020

ekrich commented Sep 29, 2020

ekrich commented Oct 2, 2020

ekrich commented Oct 4, 2020

ekrich commented Oct 5, 2020 •

edited

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020

ekrich Oct 16, 2020

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020 •

edited

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020

ekrich Oct 16, 2020

LeeTibbert Oct 14, 2020

ekrich Oct 14, 2020

		@@ -0,0 +1,11 @@
		#include <errno.h>

		int scalanative_errno() { return errno; }

		@@ -9,6 +13,16 @@ int scalanative_w_ok() { return W_OK; }

		int scalanative_x_ok() { return X_OK; }

		// SEEK_CUR, SEEK_END, SEEK_SET in clib stdio

Move native code into posixlib and clib respectively #1885

Move native code into posixlib and clib respectively #1885

Conversation

ekrich commented Aug 29, 2020

LeeTibbert commented Sep 1, 2020

LeeTibbert commented Sep 1, 2020

ekrich commented Sep 1, 2020

ekrich commented Sep 1, 2020

LeeTibbert commented Sep 1, 2020

ekrich commented Sep 29, 2020

ekrich commented Oct 2, 2020

ekrich commented Oct 4, 2020

ekrich commented Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekrich Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekrich commented Oct 5, 2020 •

edited

ekrich Oct 14, 2020 •

edited