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

Rework many POSIX and C lib with new trait feature #2996

Merged
merged 19 commits into from Nov 17, 2022

Conversation

ekrich
Copy link
Member

@ekrich ekrich commented Nov 14, 2022

Just trying to do little heavy lifting to get towards a POSIX lib that can be used on its own.

There are definitely cases where it is unclear where things should go especially for types. In C we can have header files defined once so they can be referenced more than one place and they won't conflict. In Scala we could have them as types in more than one place and they could conflict on import. This is an area we need to work out best practices.

This still doesn't solve the "I have to look at 2 specs at the same time" problem but at least we know it can be defined once either in C shared with POSIX or POSIX only.

In general in the javalib with could see libc imports because lib C can do some things but it may be more likely that we see posix or windows as the main imports for native code.

For a developer, they should be able to just import libc or posix depending on what they need. We still have quite a bit of work in this area but it is getting better.

@ekrich
Copy link
Member Author

ekrich commented Nov 14, 2022

@LeeTibbert I can address the issues above. If you have a minute to look and see what might be wrong with the Windows builds, I would be appreciative.

@LeeTibbert
Copy link
Contributor

LeeTibbert commented Nov 14, 2022

Re: Windows builds. On the case.

5 Minutes later: All the Windows builds are currently in process. I'll check back in 1/2 hour or so
and see what is up.

10 Minutes later. CI is still progressing. A few Windows builds have completed.
I need to wait until all complete before drawing conclusions.
There are some successes on Windows and some failures. It looks like the
failures are on Scala 2.11. There LLVM is complaining about
the declaration of memset().

[error] D:\a\scala-native\scala-native\unit-tests\native\.2.11\target\scala-2.11\native-test\scala\scalanative\libc.ll:3677:22: error: '@memset' defined with type 'i8* (i8*, i32, i8*)*' but expected 'i8* (i8*, i32, i64)*'
[error]   %_30008 = call i8* @"memset"(i8* %_30005, i32 0, i64 %_30007)

It looks like the definition is using a Ptr[Byte] and the (LLVM intrinsic?) is expecting a 64 bit value
(raw pointer?).

Lets see what other Windows, macOS, multi-arch, and Linux say.

Did something obvious change with the declaration of memset? I'll rat around.
I'll also check if we have run aground on a C99 vs C11 change.

2022-11-14 19:56 UTC --

There is a Linux failure with SN 2.11 with the same memset message as two of the Windows
SN 2.11 failures.

The Windows 2019 failed by just rolling over on its back and wiggling its
legs, giving no useful info.

Later the evening, after this CI, dust settles, I will have to clone this PR and
run it on my Linux system under SN 2.11. Hopefully, I will get the same error
and can localize it in the .ll files.

So far, I see nothing obviously wrong. The SN 2.11 build throws some errors
which I have not seen before, but they seem unrelated.

@ekrich
Copy link
Member Author

ekrich commented Nov 14, 2022

@LeeTibbert It seems to only happen on Windows - I changed libc.string to use the new trait feature which is my guess because that is where memset is located other than in nativelib/src/main/scala/scala/scalanative/runtime/libc.scala which I didn't change.

Maybe @WojciechMazur needs to take a look. It could have gotten more strict so maybe a call site is bad - could only DWord stuff on the Windows side.

This is the only site that seems Windows specific - https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/io/File.scala#L681

@LeeTibbert
Copy link
Contributor

It happens on Linux SN 2.11 (sometimes). I just double checked the logs above.

I may have a clue.

          !privilegeSetLength = emptyPriviligesSize.toUInt

          val privilegeSet: Ptr[Byte] = stackalloc[Byte](!privilegeSetLength)
          memset(privilegeSet, 0, !privilegeSetLength)

The stackalloc is using a windows.DWord (as you mentioned). Has that line
changed in this PR? A DWord is a fixed 32 bit size, not float 32/64 for actual
architecture. The memset declarations use CSize as the third argument. I believe
that transforms to USize, which is 64 bits on 64 bit systems.

I did not see a Dfoo which would chameleon 32/64 bit the way that CSize will.

As a debugging shot in the dark, you could hard code that stackalloc to 64 bits
and see what happens.

If I am onto something, Scala 2.11 may be doing us a favor here by complaining,
in its obscure way. The values being put into the stackalloced location are
unlikely to be larger than 32 bits, but there definitely is a theoretical buffer overrun bug there.

@WojciechMazur
Copy link
Contributor

The errors are probably caused by wrong type signature by using boxed type (CSize) instead of primtive int/long. I'll try to fix this issue in the compiler plugin soon

@WojciechMazur
Copy link
Contributor

Depending on the order of loading defns one of the memset declarations would survive:

extern decl @"M31scala.scalanative.runtime.libc$C6memset" : (ptr, int, size) => ptr
extern decl @"M29scala.scalanative.libc.stringC6memset" : (@"T28scala.scalanative.unsafe.Ptr", int, @"T32scala.scalanative.unsigned.USize") => @"T28scala.scalanative.unsafe.Ptr"

The first one is correct one as it takes unboxed values. Second one takes boxed values. For Ptr[_] it does not make any difference because at codegen both class reference and primitive ptr are represented as i8*. However unboxed value of Size if i64/i32 not i8* leading to compilation errors.
At the point method calls the types are correct, so the fix should not be problematic.

@ekrich
Copy link
Member Author

ekrich commented Nov 15, 2022

Definitions in clib string:

def memset(dest: Ptr[Byte], ch: CInt, count: CSize): Ptr[Byte] = extern

In runtime libc like this.

def memset(dest: RawPtr, ch: CInt, count: CSize): RawPtr = extern

@WojciechMazur WojciechMazur merged commit 547f6d8 into scala-native:main Nov 17, 2022
@ekrich ekrich deleted the topic/stdlib branch November 17, 2022 16:07
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.

None yet

3 participants