Enable mono cross-build on SunOS-like OS#37560
Enable mono cross-build on SunOS-like OS#37560CoffeeFlux merged 1 commit intodotnet:masterfrom am11:feature/sunos/enable-mono-build
Conversation
am11
requested review from
CoffeeFlux,
SamMonoRT,
alexischr,
lambdageek,
lateralusX,
marek-safar and
vargaz
as code owners
|
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. |
akoeplinger
left a comment
akoeplinger
left a comment
There was a problem hiding this comment.
One suggestion, looks good otherwise. Sorry for the long delay :)
akoeplinger
left a comment
akoeplinger
left a comment
There was a problem hiding this comment.
Found one more issue with the autoconf checks :)
|
Failures are unrelated to changes:
|
CoffeeFlux
left a comment
CoffeeFlux
left a comment
There was a problem hiding this comment.
Sorry the review took so long! Looked at the runtime bits and there were a couple minor things. Thanks for this!
src/mono/mono/metadata/threads.c
Outdated
There was a problem hiding this comment.
Are these all unique to illumos? Maybe leave a comment?
There was a problem hiding this comment.
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.
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.
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.
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.
@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.
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. 😢
There was a problem hiding this comment.
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! |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Summary:
$(Compiler)inmono.proj, that comes fromeng/build.sh.-Werror=strict-prototypesformadviseandposix_madviseintrospection, which is used during the compilation.mono-threads-sunos.c.With this set of changes,
dotnet hwapp.dllworks with mono flavoredSystem.Private.CoreLib.dllandlibcoreclr.soon SmartOS 2020.Contributes to: #34944.