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

devnexen · 2024-02-25T17:48:51Z

Also fixing OpenMP build itself for this platform.

brad0 · 2024-02-26T00:03:02Z

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 · 2024-02-26T06:13:21Z

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

rorth

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

devnexen · 2024-02-26T16:31:23Z

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

openmp/runtime/src/z_Linux_util.cpp

brad0 · 2024-02-27T06:30:48Z

That looks better.

rorth

Not a full review, just two comments.

rorth · 2024-02-27T13:01:18Z

openmp/runtime/src/z_Linux_util.cpp

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


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

rorth · 2024-02-27T13:07:36Z

openmp/runtime/src/z_Linux_util.cpp

@@ -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>


(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).

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

rorth · 2024-02-27T13:15:21Z

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 · 2024-02-27T13:26:22Z

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.

`openmp` currently doesn't compile on 32-bit Solaris: ``` FAILED: projects/openmp/runtime/src/CMakeFiles/omp.dir/z_Linux_util.cpp.o [...] In file included from openmp/runtime/src/z_Linux_util.cpp:78: In file included from /usr/include/libproc.h:25: In file included from /usr/include/gelf.h:10: /usr/include/libelf.h:22:2: error: "large files are not supported by libelf" 22 | #error "large files are not supported by libelf" | ^ In file included from openmp/runtime/src/z_Linux_util.cpp:78: /usr/include/libproc.h:42:2: error: "Cannot use libproc in the large file compilation environment" 42 | #error "Cannot use libproc in the large file compilation environment" | ^ ``` Looking closer, there's no point in using `Pgrab` (the only interface from `<libproc.h>`) at all: the resulting `ps_prochandle_t *` isn't used in the remainder of the code and the original PR llvm#82930 gives no indication why it is deemed necessary/useful. While at it, this patch switches to use `/proc/self/xmap`, matching `compiler-rt`'s `sanitizer_procmaps_solaris.cpp`, and makes some minor formatting fixes. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, and `amd64-pc-solaris2.11`.

`openmp` currently doesn't compile on 32-bit Solaris: ``` FAILED: projects/openmp/runtime/src/CMakeFiles/omp.dir/z_Linux_util.cpp.o [...] In file included from openmp/runtime/src/z_Linux_util.cpp:78: In file included from /usr/include/libproc.h:25: In file included from /usr/include/gelf.h:10: /usr/include/libelf.h:22:2: error: "large files are not supported by libelf" 22 | #error "large files are not supported by libelf" | ^ In file included from openmp/runtime/src/z_Linux_util.cpp:78: /usr/include/libproc.h:42:2: error: "Cannot use libproc in the large file compilation environment" 42 | #error "Cannot use libproc in the large file compilation environment" | ^ ``` Looking closer, there's no point in using `Pgrab` (the only interface from `<libproc.h>`) at all: the resulting `ps_prochandle_t *` isn't used in the remainder of the code and the original PR #82930 gives no indication why it is deemed necessary/useful. While at it, this patch switches to use `/proc/self/xmap`, matching `compiler-rt`'s `sanitizer_procmaps_solaris.cpp`, and makes some minor formatting fixes. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, and `amd64-pc-solaris2.11`.

`openmp` currently doesn't compile on 32-bit Solaris: ``` FAILED: projects/openmp/runtime/src/CMakeFiles/omp.dir/z_Linux_util.cpp.o [...] In file included from openmp/runtime/src/z_Linux_util.cpp:78: In file included from /usr/include/libproc.h:25: In file included from /usr/include/gelf.h:10: /usr/include/libelf.h:22:2: error: "large files are not supported by libelf" 22 | #error "large files are not supported by libelf" | ^ In file included from openmp/runtime/src/z_Linux_util.cpp:78: /usr/include/libproc.h:42:2: error: "Cannot use libproc in the large file compilation environment" 42 | #error "Cannot use libproc in the large file compilation environment" | ^ ``` Looking closer, there's no point in using `Pgrab` (the only interface from `<libproc.h>`) at all: the resulting `ps_prochandle_t *` isn't used in the remainder of the code and the original PR llvm#82930 gives no indication why it is deemed necessary/useful. While at it, this patch switches to use `/proc/self/xmap`, matching `compiler-rt`'s `sanitizer_procmaps_solaris.cpp`, and makes some minor formatting fixes. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, and `amd64-pc-solaris2.11`.

llvmbot added the openmp:libomp OpenMP host runtime label Feb 25, 2024

rorth reviewed Feb 26, 2024

View reviewed changes

brad0 reviewed Feb 27, 2024

View reviewed changes

openmp/runtime/src/z_Linux_util.cpp Show resolved Hide resolved

devnexen force-pushed the openmp_solaris_upd branch from 444ec06 to 8407834 Compare February 27, 2024 06:29

rorth reviewed Feb 27, 2024

View reviewed changes

[OpenMP] Implements __kmp_is_address_mapped for Solaris/Illumos.

febb364

Also fixing OpenMP build itself for this platform.

devnexen force-pushed the openmp_solaris_upd branch from 8407834 to febb364 Compare February 27, 2024 13:23

brad0 approved these changes Mar 8, 2024

View reviewed changes

devnexen merged commit 05280b5 into llvm:main Mar 8, 2024

rorth mentioned this pull request Jun 2, 2025

[OpenMP] Don't use libproc on Solaris #142379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

devnexen commented Feb 25, 2024

Uh oh!

brad0 commented Feb 26, 2024

Uh oh!

rorth commented Feb 26, 2024

Uh oh!

rorth left a comment

Uh oh!

devnexen commented Feb 26, 2024

Uh oh!

Uh oh!

brad0 commented Feb 27, 2024

Uh oh!

rorth left a comment

Uh oh!

rorth Feb 27, 2024

Uh oh!

rorth Feb 27, 2024

Uh oh!

devnexen Feb 27, 2024

Uh oh!

rorth commented Feb 27, 2024

Uh oh!

devnexen commented Feb 27, 2024

Uh oh!

Uh oh!

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

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

Conversation

devnexen commented Feb 25, 2024

Uh oh!

brad0 commented Feb 26, 2024

Uh oh!

rorth commented Feb 26, 2024

Uh oh!

rorth left a comment

Choose a reason for hiding this comment

Uh oh!

devnexen commented Feb 26, 2024

Uh oh!

Uh oh!

brad0 commented Feb 27, 2024

Uh oh!

rorth left a comment

Choose a reason for hiding this comment

Uh oh!

rorth Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

rorth Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

devnexen Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

rorth commented Feb 27, 2024

Uh oh!

devnexen commented Feb 27, 2024

Uh oh!

Uh oh!