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

Hide/deal with posix.sys.stat as it's not binary compatible with POSIX standard #3902

Open
keynmol opened this issue May 3, 2024 · 4 comments

Comments

@keynmol
Copy link
Contributor

keynmol commented May 3, 2024

I've just noticed that stat structure in posix seems to be incorrect: https://github.com/scala-native/scala-native/blob/main/posixlib/src/main/scala/scala/scalanative/posix/sys/stat.scala#L45-L60

It doesn't match the layout in posix docs: https://www.man7.org/linux/man-pages/man3/stat.3type.html

And there seems to be special handling: https://github.com/scala-native/scala-native/blob/main/posixlib/src/main/resources/scala-native/sys/stat.c#L7-L36

Seems like it's making this stat structure indeed not useful for passing to any extern function that actually expects stat.


I stumbled on it because of cryptic errors, caused by passing a function pointer (implemented in Scala) to a C library, which invokes it with actual POSIX stat*, expecting mutation in place.

That mutation corrupts the result because of incorrect alignment and sizes and all hell breaks loose.

@keynmol keynmol changed the title Hide posix.sys.stat as it's not binary compatible with POSIX standard Hide/deal with posix.sys.stat as it's not binary compatible with POSIX standard May 3, 2024
@ekrich
Copy link
Member

ekrich commented May 3, 2024

The code should match the spec for sure.

In general, there are problems with structs because we do not have named structs and posix does not specify the order so there can be OS dependent ordering and alignment - @LeeTibbert has talked extensively about this - sure you are aware.

We started by having native adapters to use names in the C as we know the order/size of elements in the Scala but Lee didn't like the copying so then I sort of lost what the state of the art is.

I think longer term we need the named structs and perhaps conditional compilation on the Scala side - sort of like the link time stuff - not sure.

@LeeTibbert
Copy link
Contributor

LeeTibbert commented May 4, 2024

keynmol.

Sorry that these "features" took you time. Some of them long pre-date me and we have been
doing the best we could to make them useful to real people.

"binary compatible with POSIX Standard" is not a real thing. Reasonably recent Open Group
specifications, say from this century, specify the primitive type of fields but rarely the exact size
and order. There are some cases, I think, where they specify "only these fields" but in most
cases they say something like "at least these fields". Many operating systems do add fields.
In addition, compilers can add padding bytes as they see fit. You probably know all of this.

One can do at least two things posixlib uses to make ScalaNative/C interop easier:

  1. The way that is fastest at execution time is to check at compile time that the structures used by
    Scala Native and C are compatible. This means testing the size and offset of the structures
    in C include files on a given operating system.

    It also means that the SN and C structures must be manually kept in correspondence. Depending
    on who wrote the checking code, it also means that adding new operating systems is detected and rejected.

  2. Create and use more extensive intermediate C code; aka "glue code". This is slower at execution time
    but allows re-ordering of C structures which are known to vary widely by operating systems. In the past
    month I have been doing some work using posixlib dirent.scala and have had to rely upon the pre-existing
    glue code.

    The line in sys/stat.h is an example of the kind of warning that some authors have used. Agreed, that it might
    be helpful before-the-fact but is probably annoying-after-the-fact:

 // This structure is _not_ a candidate for direct pass-thru to OS.  
  1. In some places there may be some Ops magic where an implicit function handles
    required conversions. That if, if one access the structure by field number (_1) one is on one's own.
    If one use a name from the implicit class, say d_name, the proper field is accessed.
    I just introduced such in dirent.scala to deal some complexity (topic of another issue)

    I believe this technique was used to deal with the presence of an 'extra` field (sin_len?) on BSD
    operating systems.

I think the bottom line is that one must be exceedingly careful when passing a Scala Native
structure to C. At the least, one should check for a corresponding .c file or glue file to see if
direct passthru is checked or one must use glue, either that provide or one's own.

My goal is to be helpful. Please suggest any ways forward. A number of us have been trying
to corral & improve posixlib for a long time.

@LeeTibbert
Copy link
Contributor

Personal opinion.

I believe that fixing up operating system differences should be kept out of the mainline compiler,
for at least the next decade or until the AI revolution, whichever comes first.

Yes, posixlib is of finite size and yes the number of operating systems Scala Native supports is of finite
size. The cross-product should also be of a large but finite size. Notice the understated use of the word "large".

There are probably more people who either have the expertise to write and evaluate "glue" code or who can be
trained up than there are people who have SN compiler expertise.

Agreed, "glue" code is ugly and expensive at runtime. In a number of cases we have been able to change it to
"guard" code which allows direct passthru.

It would be wonderful to never have to read or write "glue" code but that wonder may preclude greater wonders,
such as fast compilation of "normal" Scala Native code.

@LeeTibbert
Copy link
Contributor

Re: Bookkeeping

I recommend that this Issue stay open for a week to allow comments and then be closed as "economically untractable".

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

No branches or pull requests

3 participants