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

Define _REENTRANT for SUNOS #104527

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Define _REENTRANT for SUNOS #104527

merged 1 commit into from
Aug 19, 2024

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Jul 7, 2024

This matches mono. This is required to make errno a thread-local variable. This fixes problems where calls into libSystem.Native return failure but the the errno is 0. This was causing all kinds of odd asserts in System.IO.Tests and other libraries tests.

See the man page for errno(3c):

In the case of multithreaded applications, the -mt option must be specified on the command line at compilation time (see threads(7)). When the -mt option is specified, errno becomes a macro that enables each thread to have its own errno.

Since -mt is presumably specific to an old Sun compiler, the threads(7) man page says:

Users of other compilers such as gcc and clang should manually set -D_REENTRANT on the compilation line. There are no other libraries flags necessary.

Contributes to #34944

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2024
@jkotas jkotas requested a review from am11 July 8, 2024 00:30
@am11
Copy link
Member

am11 commented Jul 8, 2024

I hope you won't take it the wrong way, and I am happy this port is moving forward. I think it'd be better to port something sizeable, such as one of the remaining runtime library needed to complete shared framework, rather than many teeny tiny patches. Otherwise, we would be burdening the maintainers which will slow down the port process / interest.

Thanks for pushing it forward.

@jkotas jkotas added area-PAL-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 8, 2024
@AustinWise
Copy link
Contributor Author

I hope you won't take it the wrong way, and I am happy this port is moving forward. I think it'd be better to port something sizeable, such as one of the remaining runtime library needed to complete shared framework, rather than many teeny tiny patches. Otherwise, we would be burdening the maintainers which will slow down the port process / interest.

Thanks for pushing it forward.

No offense taken. My intention in making these small PRs was to make them easier to review, not more difficult. I can conceive that that per PR overhead for maintainers like evaluating Build Analysis of the flaky CI system can add up. Please let me know if there is a better way to collaborate on this port.

For what it's worth, this PR and #104448 make it possible to run library tests without the test runner dying in asserts or memory corruption. I expect future PRs to implement larger swaths of missing functionality in the libraries.

Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

LGTM, it is aligning with mono:

add_definitions(-DGC_SOLARIS_THREADS -DGC_SOLARIS_PTHREADS -D_REENTRANT -D_POSIX_PTHREAD_SEMANTICS -DUSE_MMAP -DUSE_MUNMAP -DHOST_SOLARIS -D__EXTENSIONS__ -D_XPG4_2)

@am11 am11 requested a review from janvorli July 9, 2024 05:06
This is required to make errno a thread-local variable. See the manpages for errno(3c) and threads(7).
@AustinWise
Copy link
Contributor Author

Now that .NET 9 has branched, I rebased this PR.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit f19e4ec into dotnet:main Aug 19, 2024
147 of 149 checks passed
@AustinWise AustinWise deleted the austin/errno branch August 19, 2024 19:11
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants