-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Enable mono cross-build on SunOS-like OS #37560
Enable mono cross-build on SunOS-like OS #37560
Conversation
cc @akoeplinger |
Any chance this PR also gets a review? (trying to avoid upcoming merge conflicts from other PRs) :) |
I'll take a look later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion, looks good otherwise. Sorry for the long delay :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one more issue with the autoconf checks :)
Failures are unrelated to changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry the review took so long! Looked at the runtime bits and there were a couple minor things. Thanks for this!
case SCHED_FSS: | ||
#endif | ||
#ifdef SCHED_FX | ||
case SCHED_FX: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these all unique to illumos? Maybe leave a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
illumos and Solaris at least. Is there a way to tell for sure which platform support which flag? The existing #ifdef SCHED_BATCH
is also without a comment, I guess because it is self-explanatory feature detection.
guint64 | ||
mono_native_thread_os_id_get (void) | ||
{ | ||
return (guint64)pthread_self (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is intended to return the actual TID, a pid_t
, not the results of pthread_self ()
. This needs to be the sunos gettid
syscall equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup and this is what I have found as an equivalent. Also, saw the same thing happening in coreclr against gettid
unavailability on SunOS:
runtime/src/coreclr/src/pal/src/include/pal/thread.hpp
Lines 780 to 782 in e6d8017
#else | |
#define PlatformGetCurrentThreadId() (SIZE_T)pthread_self() | |
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. And I guess the value from thr_self ()
is no better than the POSIX thread ID... What a mess. Can you leave a comment so it's clear this isn't just a mistake? Otherwise I suspect I'll see this code 6 months down the line and, having forgotten about this conversation, look into it all over again. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CoffeeFlux, I understand your position. and tbh, I am also not 100% sure whether to use light weight process (lwp) ID or pthread_self in this case as we are doing different things with this returned value at the call site (some places are using pthread library, others are making syscalls etc.). As far as I have read, POSIX does not provide strict specifications for process/thread related IDs and their relation with platform native threads, hence the disparity across all platforms and makes it difficult for devs to figure out the exact semantics (which we can see in all mono-threads-{platform}.c
files). I only made sure that the hello world is working without violating any assertion i.e. changes in threads.c were the result of assertion violation on run time.
It is not exactly straight-forward to port dotnet/runtime to a new Unix platform, and test it all in one go. The eng side is quite a work to pull and lots of (ever changing) moving parts to learn. That's the reason why I am currently pushing changes to get this semi-ready state checked, to be able to get SDK to work, and then port libraries and run tests on the host Sun-like platforms.
Added a TODO comment to that effect. So far, this is just-working. We will get more information, when we will be able to execute related libraries tests to exercise this code thoroughly, and it might get changed to lwpId, something else or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and thanks for the detailed response. I'm happy with a just-working state on this PR; I just want to make sure we're attempting to capture that context somewhere in the code instead of buried in a Github issue. Appreciate your efforts to get this working!
As far as I have read, POSIX does not provide strict specifications for process/thread related IDs and their relation with platform native threads, hence the disparity across all platforms
Yep. 😢
guint64 | ||
mono_native_thread_os_id_get (void) | ||
{ | ||
return (guint64)pthread_self (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and thanks for the detailed response. I'm happy with a just-working state on this PR; I just want to make sure we're attempting to capture that context somewhere in the code instead of buried in a Github issue. Appreciate your efforts to get this working!
As far as I have read, POSIX does not provide strict specifications for process/thread related IDs and their relation with platform native threads, hence the disparity across all platforms
Yep. 😢
@CoffeeFlux, @akoeplinger, resolved merged conflict in configure.ac and rebased with master. If there are no other comments, can this be merged? Thanks! :) |
Yep, the wasm failure on the Mono side is a known issue. Thanks again! |
Summary:
$(Compiler)
inmono.proj
, that comes fromeng/build.sh
.-Werror=strict-prototypes
formadvise
andposix_madvise
introspection, which is used during the compilation.mono-threads-sunos.c
.With this set of changes,
dotnet hwapp.dll
works with mono flavoredSystem.Private.CoreLib.dll
andlibcoreclr.so
on SmartOS 2020.Contributes to: #34944.