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

Haiku: Add Haiku support for CoreCLR/PAL #93907

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

trungnt2910
Copy link
Contributor

This contains the code required to build this repo's native components and get paltests to pass on Haiku.

Most implementation details related to this commit are documented here.

Part of #55803.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 24, 2023
src/native/minipal/errno.h Outdated Show resolved Hide resolved
sprintf(name, "/shm-dotnet-%d", getpid());
name[sizeof(name) - 1] = '\0';
shm_unlink(name);
int fd = shm_open(name, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW, 0600);
#else // TARGET_FREEBSD
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the #elif defined(TARGET_LINUX) here and final #else with the posix fallback instead of hierarchical ifdef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. The final #else assigns the fd variable to -1 instead of doing the POSIX fallback itself, in case any of the platform-specific functions above return an error.

This should solve #72192 from the .NET 7 era.

src/coreclr/pal/src/include/pal/palinternal.h Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.unix.cpp Outdated Show resolved Hide resolved
src/coreclr/pal/inc/rt/safecrt.h Outdated Show resolved Hide resolved
src/coreclr/utilcode/sstring.cpp Outdated Show resolved Hide resolved
// unreachable
abort();
return Error_SUCCESS;
return Error_EINVAL;
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the old aborting code as a fallback and add separate #if for HAIKU instead? I am not familiar enough with the contract of this function though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added back the abort() logic, as an assertion that the function is unreachable.

This is true because on unsupported platforms, the higher-level C# libraries should already throw while trying to construct a network events socket.

Returning Error_SUCCESS does not make much sense though.

@trungnt2910
Copy link
Contributor Author

Strange, I'm not modifying any FreeBSD-related codepaths, what's wrong with the FreeBSD build?

@jkotas
Copy link
Member

jkotas commented Oct 25, 2023

FreeBSD build error is:

/__w/1/s/src/coreclr/pal/src/misc/sysinfo.cpp:215:57: error: use of undeclared identifier 'NUPML4E'
    lpSystemInfo->lpMaximumApplicationAddress = (PVOID) VM_MAXUSER_ADDRESS;
                                                        ^
/crossrootfs/x64/usr/include/machine/vmparam.h:186:35: note: expanded from macro 'VM_MAXUSER_ADDRESS'
#define VM_MAXUSER_ADDRESS      UVADDR(NUPML4E, 0, 0, 0)
                                       ^

I guess you have introduced it by changing conditions around some of the includes.

@trungnt2910
Copy link
Contributor Author

This is suspicious:

-- Looking for include file sys/user.h
-- Looking for include file sys/user.h - not found

I wonder how I could get more detailed CMake logs for the CI runs.

@trungnt2910
Copy link
Contributor Author

When looking at other files, it seems like sys/user.h is only used on FreeBSD. Somehow sysinfo.cpp includes this header for all platforms.

This time, instead of checking for this header during configuration as in #86391, a FreeBSD #ifdef guard is added instead.

Comment on lines 62 to 71
// POSIX fallback
if (fd == -1)
{
char name[24];
sprintf(name, "/shm-dotnet-%d", getpid());
name[sizeof(name) - 1] = '\0';
shm_unlink(name);
fd = shm_open(name, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW, 0600);
shm_unlink(name);
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this block be in #else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should not, as stated in #93907 (comment).

Even for OSes with platform-specific ways to get a special memory file open, should their method fail, we can still always use the POSIX fallback.

Comment on lines 14 to 17
#ifdef TARGET_LINUX
#include <sys/syscall.h>
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Is this used by other Unix systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is only used on Linux:

#ifdef TARGET_LINUX
if (syscall(__NR_get_mempolicy, NULL, NULL, 0, 0, 0) < 0 && errno == ENOSYS)
return;
int highestNumaNode = GetNodeNum("/sys/devices/system/node", false);
// we only use this implementation when there are two or more NUMA nodes available
if (highestNumaNode < 1)
return;
g_numaAvailable = true;
g_highestNumaNode = highestNumaNode;
#endif

and

#ifdef TARGET_LINUX
return syscall(__NR_mbind, (long)start, len, 1, (long)nodemask, maxnode, 0);
#else

Comment on lines 577 to 578
#define _SIZE_T_DEFINED
#endif // !PAL_STDCPP_COMPAT
Copy link
Member

Choose a reason for hiding this comment

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

What's the impact range of this?

Copy link
Contributor Author

@trungnt2910 trungnt2910 Oct 30, 2023

Choose a reason for hiding this comment

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

What do you mean by "impact range"?

The line should affect all platforms. However, I believe that this is how things should be, since with PAL_STDCPP_COMPAT the header should not define things that potentially mess up system headers.

Also, CoreCLR builds are working, so it should not break the definition on any other platforms.

Some more comments about this change here: #55803 (comment)

@mangod9
Copy link
Member

mangod9 commented Mar 4, 2024

Since this is a large change, would it be possible to split the PR info smaller component PRs for PAL, hosting, libraries etc. so relevant teams can review the changes? Thx

@mangod9 mangod9 added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 15, 2024
@Anutrix
Copy link

Anutrix commented Apr 29, 2024

@trungnt2910 Are you still working on this?

@trungnt2910
Copy link
Contributor Author

Not right now (I'm too busy). I'm still tracking it though, so if there are small specific issues I can resolve, I can fix it.

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 29, 2024
@jkotas
Copy link
Member

jkotas commented Apr 30, 2024

As @mangod9 suggested above, this needs to be split into multiple smaller PRs per area to make it reviewable. I am going to switch it to draft for now.

@jkotas jkotas marked this pull request as draft April 30, 2024 04:01
@trungnt2910
Copy link
Contributor Author

To me, this PR seems like a pretty atomic change (misc insertions needed to allow native components to start building on Haiku).

How should I split it? Maybe src/coreclr in one PR, src/mono in another, src/native in one more, then the rest in one final PR?

@jkotas
Copy link
Member

jkotas commented Apr 30, 2024

Yes, that would work.

@trungnt2910 trungnt2910 changed the title Haiku: Initial CoreCLR support Haiku: Add Haiku support for CoreCLR/PAL Apr 30, 2024
@trungnt2910 trungnt2910 marked this pull request as ready for review April 30, 2024 14:26
@trungnt2910
Copy link
Contributor Author

I have edited this pull request to include only files in src/coreclr/pal.
Other parts will come in other PRs.
Please let me know if this needs to be split further.

@trungnt2910
Copy link
Contributor Author

trungnt2910 commented Apr 30, 2024

Latest Haiku builds are failing with this error:

  [  7%] Building CXX object inc/CMakeFiles/corguids.dir/__/pal/prebuilt/idl/cordebug_i.cpp.o
  In file included from /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/inc/rt/palrt.h:630,
                   from /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/inc/rt/rpc.h:15,
                   from /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/prebuilt/idl/cordebug_i.cpp:29:
  /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/inc/rt/safecrt.h:1093:16: error: conflicting declaration of C function 'size_t wcsnlen(const WCHAR*, size_t)'
   1093 | size_t __cdecl wcsnlen(const WCHAR *inString, size_t inMaxSize);
        |                ^~~~~~~
  In file included from /home/runner/work/dotnet-builds/dotnet-builds/dotnet-rootfs/boot/system/develop/headers/posix/wctype.h:10,
                   from /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/inc/pal.h:51,
                   from /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/inc/rt/palrt.h:136:
  /home/runner/work/dotnet-builds/dotnet-builds/dotnet-rootfs/boot/system/develop/headers/posix/wchar.h:122:17: note: previous declaration 'size_t wcsnlen(const wchar_t*, size_t)'
    122 | extern size_t   wcsnlen(const wchar_t *wcs, size_t maxLength);
        |                 ^~~~~~~
  /home/runner/work/dotnet-builds/dotnet-builds/runtime/src/coreclr/pal/inc/rt/safecrt.h:1098:16: error: conflicting declaration of C function 'size_t wcsnlen(const WCHAR*, size_t)'
   1098 | size_t __cdecl wcsnlen(const WCHAR *inString, size_t inMaxSize)
        |                ^~~~~~~
  /home/runner/work/dotnet-builds/dotnet-builds/dotnet-rootfs/boot/system/develop/headers/posix/wchar.h:122:17: note: previous declaration 'size_t wcsnlen(const wchar_t*, size_t)'
    122 | extern size_t   wcsnlen(const wchar_t *wcs, size_t maxLength);
        |                 ^~~~~~~
  make[2]: *** [inc/CMakeFiles/corguids.dir/build.make:76: inc/CMakeFiles/corguids.dir/__/pal/prebuilt/idl/cordebug_i.cpp.o] Error 1
  make[1]: *** [CMakeFiles/Makefile2:3250: inc/CMakeFiles/corguids.dir/all] Error 2
  make[1]: *** Waiting for unfinished jobs....

Seems like whoever included wctype.h did not expect wchar.h to be included as well.

According to the POSIX Standard, however:

[CX] [Option Start] Inclusion of the <wctype.h> header may make visible all symbols from the headers <ctype.h>, <stdarg.h>, <stddef.h>, <stdio.h>, <stdlib.h>, <string.h>, <time.h>, and <wchar.h>. [Option End]

What should I do in this case? Removing <wchar.h> from Haiku's system include just breaks everything. And according to POSIX Haiku isn't doing anything wrong in the first place...

@jkotas
Copy link
Member

jkotas commented Apr 30, 2024

Does anything break if our local wcsnlen implementation is deleted?

trungnt2910 and others added 2 commits May 1, 2024 00:02
This contains a part of the code required to build CoreCLR and get
`paltests` to pass on Haiku.

This commit covers `src/coreclr/pal/**`.

Co-authored-by: Jessica Hamilton <jessica.l.hamilton@gmail.com>
This conflicts with the standard one already included through `wctype.h` in some POSIX OSes like Haiku.
@trungnt2910
Copy link
Contributor Author

Hasn't broken my local Linux build and moved my local Haiku build to a different error. Let the CI inform the rest of the answer.

@trungnt2910
Copy link
Contributor Author

The next Haiku error is:

  asmparse.y:176: error: "_IMPORT" redefined [-Werror]
  In file included from /home/trung/dotnet-rootfs/boot/system/develop/headers/posix/sys/types.h:11,
                   from /home/trung/dotnet-rootfs/boot/system/develop/headers/posix/stdio.h:9,
                   from /home/trung/dotnet-rootfs/boot/system/develop/headers/bsd/stdio.h:9,
                   from /home/trung/dotnet-runtime/src/coreclr/pal/inc/pal.h:39,
                   from /home/trung/dotnet-runtime/src/coreclr/pal/inc/rt/palrt.h:136,
                   from /home/trung/dotnet-runtime/src/coreclr/pal/inc/rt/rpc.h:15,
                   from /home/trung/dotnet-runtime/src/coreclr/pal/inc/rt/objidl.h:12,
                   from /home/trung/dotnet-runtime/src/coreclr/pal/inc/rt/ole2.h:8,
                   from /home/trung/dotnet-runtime/src/coreclr/inc/cor.h:16,
                   from /home/trung/dotnet-runtime/src/coreclr/ilasm/./ilasmpch.h:8,
                   from asmparse.y:9:
  /home/trung/dotnet-rootfs/boot/system/develop/headers/os/BeBuild.h:95: note: this is the location of the previous definition
     95 | #define _IMPORT
        |
  asmparse.y:262: error: "_EXPORT" redefined [-Werror]
  /home/trung/dotnet-rootfs/boot/system/develop/headers/os/BeBuild.h:90: note: this is the location of the previous definition
     90 | # define _EXPORT __attribute__((visibility("default")))
        |

Currently I am solving this by adding these lines to ilasmpch.h:

#include "mdfileformat.h"
#include "stgpooli.h"

+#ifdef _EXPORT
+#undef _EXPORT
+#endif

+#ifdef _IMPORT
+#undef _IMPORT
+#endif

#endif

I wonder whether these lines should be put directly to pal.h instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member os-haiku
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants