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

[OpenMP] Implements __kmp_is_address_mapped for Solaris/Illumos. #82930

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

devnexen
Copy link
Member

Also fixing OpenMP build itself for this platform.

@llvmbot llvmbot added the openmp:libomp OpenMP host runtime label Feb 25, 2024
@brad0
Copy link
Contributor

brad0 commented Feb 26, 2024

It was already known that illumos was not building with the __kmp_set_stack_info() function due to the difference in non-portable POSIX thread functions implemented by Solaris vs illumos.

From reading the man page I question whether you can mix Solaris threads and POSIX threads functions together.

CC @rorth

@rorth
Copy link
Collaborator

rorth commented Feb 26, 2024

You can. See threads(7):

NAME
       threads, pthreads - POSIX pthreads and Solaris threads concepts

SYNOPSIS
   POSIX
       #include <pthread.h>

   Solaris
       #include <sched.h>

       #include <thread.h>

DESCRIPTION
       POSIX  and  Solaris  threads  each have their own implementation within
       libc(3LIB). Both implementations are interoperable, their functionality
       similar,  and  can  be  used  within  the  same application. Only POSIX

Copy link
Collaborator

@rorth rorth left a comment

Choose a reason for hiding this comment

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

Also, please state where you tested the patch. Illumos only, Solaris, which version?

@devnexen
Copy link
Member Author

I tried on an Illumos based distro (openinidiana), note that illumos supports pthread_attr_get_np but not solaris.

@brad0
Copy link
Contributor

brad0 commented Feb 27, 2024

That looks better.

Copy link
Collaborator

@rorth rorth left a comment

Choose a reason for hiding this comment

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

Not a full review, just two comments.

@@ -66,6 +66,12 @@
#include <sys/types.h>
#include <sys/sysctl.h>
#elif KMP_OS_SOLARIS
#if defined(__LP64__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why restrict the code to __LP64__? openmp is 64-bit-only anyway.

@@ -66,6 +66,12 @@
#include <sys/types.h>
#include <sys/sysctl.h>
#elif KMP_OS_SOLARIS
#if defined(__LP64__)
#define _STRUCTURED_PROC 1
#include <sys/procfs.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

(GH is driving me nuts: lost part of my comments for the second time ;-()

Why do things this way? The documented way to access procfs is to just include <procfs.h> (which deals with _STRUCTURED_PROC internally).

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not know procfs.h did all, handy.

@rorth
Copy link
Collaborator

rorth commented Feb 27, 2024

I tried on an Illumos based distro (openinidiana), note that illumos supports pthread_attr_get_np but not solaris.

So you're adding a third variant when a simple conditional to distinguish between Solaris and Illumos would do? And how will you handle some case where there's no common ground between the two?

The Illumos community finally needs to deal with Issue #53919: @MaskRay asked me to file it two years ago when running into Illumos/Solaris differences wrt. dlpi_tls_modid in struct dl_phdr_info and stop worrying about Illumos until they got this Issue resolved. Two years later and not a thing has happened...

Also fixing OpenMP build itself for this platform.
@devnexen
Copy link
Member Author

I tried on an Illumos based distro (openinidiana), note that illumos supports pthread_attr_get_np but not solaris.

So you're adding a third variant when a simple conditional to distinguish between Solaris and Illumos would do? And how will you handle some case where there's no common ground between the two?

The Illumos community finally needs to deal with Issue #53919: @MaskRay asked me to file it two years ago when running into Illumos/Solaris differences wrt. dlpi_tls_modid in struct dl_phdr_info and stop worrying about Illumos until they got this Issue resolved. Two years later and not a thing has happened...

Unfortunately, it s the reality of OSS ; for now I m using an old api present in both proprietary and opensolaris descendants :) I realise now that even sanitizers use it too.

@devnexen devnexen merged commit 05280b5 into llvm:main Mar 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openmp:libomp OpenMP host runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants