Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
de76356
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.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 ofpthread_self ()
. This needs to be the sunosgettid
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
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!
Yep. 😢