Page MenuHomePhabricator

[libc++] Provide SEM_VALUE_MAX fallback on Solaris
Needs ReviewPublic

Authored by ro on Jun 10 2020, 11:07 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

This is the first in a series of patches to port libc++ to Solaris. There does exist a
slightly bitrotten port, however it was done on Illumos (an OpenSolaris derivative)
and the two have diverged in some areas over the last 10 years.

Building libc++ on Solaris fails like this:

In file included from /vol/llvm/src/llvm-project/local/libcxx/include/atomic:550,
                 from /vol/llvm/src/llvm-project/local/libcxx/include/memory:681,
                 from /vol/llvm/src/llvm-project/local/libcxx/include/algorithm:643,
                 from /vol/llvm/src/llvm-project/local/libcxx/test/libcxx/double_include.sh.cpp:24:
/vol/llvm/src/llvm-project/local/libcxx/include/semaphore:165:73: error: ‘SEM_VALUE_MAX’ was not declared in this scope
  165 |     typename conditional<(__least_max_value > 1 && __least_max_value <= _LIBCPP_SEMAPHORE_MAX),
      |                                                                         ^~~~~~~~~~~~~~~~~~~~~

This happens because <limits.h> doesn't define SEM_VALUE_MAX. While this is
allowed by XPG7 if the value is indeterminate ([[ https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html#tag_13_24 | XPG7 <limits.h> ]] and the
application is expected to fall back to sysconf(_SC_SEM_VALUE_MAX), this isn't the case on Solaris:
sysconf(_SC_SEM_VALUE_MAX) always returns _SEM_VALUE_MAX from
<sys/param.h>.

I've chose a solution similar to the one in Python Issue 3110 which uses that value as a fallback.

Tested on amd64-pc-solaris2.11 (Solaris 11.4, 11.3 and OpenIndiana 2020.04).

Ok for master?

Diff Detail

Event Timeline

ro created this revision.Jun 10 2020, 11:07 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJun 10 2020, 11:07 AM
jfb added a comment.Jun 10 2020, 1:01 PM

I'd much rather remove unsupported platforms, instead of seemingly supporting them. Is there interest in officially supporting Solaris? Are there going to be actively maintained bots, as well as a specific set of people who'll fix issues? Would these folks do more libc++ contributions than just maintain Solaris?

ro added a comment.Jun 11 2020, 5:32 AM

Where are such requirements documented? Nothing like this has ever come up with my Solaris patches to llvm, clang, or compiler-rt.

That said, I do maintain both Solaris/SPARC and Solaris/x86 buildbots. However, I certainly cannot commit to full-time maintenance of LLVM on
Solaris in addition to my Solaris GCC maintainership in my copious spare time: such a claim would deceive both myself and others. If that's
a requirement for accepting any Solaris patches, please say so up front so I can avoid wasting both your and my time. However, that should be a
matter of LLVM project policy, not the personal preference of an individual maintainer.

In D81602#2087403, @ro wrote:

Where are such requirements documented? Nothing like this has ever come up with my Solaris patches to llvm, clang, or compiler-rt.

That said, I do maintain both Solaris/SPARC and Solaris/x86 buildbots. However, I certainly cannot commit to full-time maintenance of LLVM on
Solaris in addition to my Solaris GCC maintainership in my copious spare time: such a claim would deceive both myself and others. If that's
a requirement for accepting any Solaris patches, please say so up front so I can avoid wasting both your and my time. However, that should be a
matter of LLVM project policy, not the personal preference of an individual maintainer.

Let's put it that way: as far as I can tell, this code is dead, so this patch isn't adding anything beyond technical debt. And the reason why it's dead is that there are no *libc++* build bots set up for Solaris, and no maintainer actively working on supporting that platform AFAICT.

JF made the comment he made because I recently complained to him about how the maintenance burden for libc++ is both huge and also rests on the shoulder of too few. The main reason for the burden being so large is that we try to support a high number of system configurations and sub-configurations of libc++ (like no-threads, no-stdio, etc.), and we also tend to bend backwards instead of requesting that systems conform to e.g. POSIX in a more stringent way. That's unfortunately part of a discussion that needs to happen and that is much larger than this patch, but my point is that libc++ needs to start dropping workarounds for systems that are not actively supported, not add new ones.

Going back to the patch itself -- is there any reason why <limits.h> can't define SEM_VALUE_MAX on Solaris? That's the right way of solving this problem.

ro added a comment.Jun 11 2020, 8:50 AM

Let's put it that way: as far as I can tell, this code is dead, so this patch isn't adding anything beyond technical debt. And the reason why it's dead is that there are no *libc++* build bots set up for Solaris, and no maintainer actively working on supporting that platform AFAICT.

True, because right now libc++ doesn't build even on Illumos. As I mentioned, my patch is only the first in a series to change that.
I'd have posted more of them already but for the pushback I got on the first trivial one. While working on my patch series, I already encountered more of those (like AIX and partially Windows) that caused me to spend quite some time trying to verify that my changes wouldn't negatively affect them, only to discover in the end that the code doesn't even compile.

JF made the comment he made because I recently complained to him about how the maintenance burden for libc++ is both huge and also rests on the shoulder of too few. The main reason for the burden being so large is that we try to support a high number of system configurations and sub-configurations of libc++ (like no-threads, no-stdio, etc.), and we also tend to bend backwards instead of requesting that systems conform to e.g. POSIX in a more stringent way. That's unfortunately part of a discussion that needs to happen and that is much larger than this patch, but my point is that libc++ needs to start dropping workarounds for systems that are not actively supported, not add new ones.

Fully understood: keeping code around for dead systems is only creating work for everyone else and making the code harder to maintain
for no gain. However, demanding systems that conform to the very latest C and POSIX.1 standards quickly restricts you to a very small set: many people tend to forget that.

Going back to the patch itself -- is there any reason why <limits.h> can't define SEM_VALUE_MAX on Solaris? That's the right way of solving this problem.

Yes and no: I'm discussing this very question with the Solaris engineers as we speak. However, even if this were changed tomorrow, there's
a large installed base without that fix, so unless you restrict libc++ to an (at least initially) neglegible number of systems, you have to allow
for those. Besides, even if the choice of not defining SEM_VALUE_MAX is strange especially if the return value of sysconf(_SC_SEM_VALUE_MAX) is a constant, it's fully within at least the letter of POSIX.1/XPG7 which allows for that situation.

jfb added a comment.Jun 11 2020, 3:19 PM
In D81602#2087793, @ro wrote:

Let's put it that way: as far as I can tell, this code is dead, so this patch isn't adding anything beyond technical debt. And the reason why it's dead is that there are no *libc++* build bots set up for Solaris, and no maintainer actively working on supporting that platform AFAICT.

True, because right now libc++ doesn't build even on Illumos. As I mentioned, my patch is only the first in a series to change that.
I'd have posted more of them already but for the pushback I got on the first trivial one. While working on my patch series, I already encountered more of those (like AIX and partially Windows) that caused me to spend quite some time trying to verify that my changes wouldn't negatively affect them, only to discover in the end that the code doesn't even compile.

JF made the comment he made because I recently complained to him about how the maintenance burden for libc++ is both huge and also rests on the shoulder of too few. The main reason for the burden being so large is that we try to support a high number of system configurations and sub-configurations of libc++ (like no-threads, no-stdio, etc.), and we also tend to bend backwards instead of requesting that systems conform to e.g. POSIX in a more stringent way. That's unfortunately part of a discussion that needs to happen and that is much larger than this patch, but my point is that libc++ needs to start dropping workarounds for systems that are not actively supported, not add new ones.

Fully understood: keeping code around for dead systems is only creating work for everyone else and making the code harder to maintain
for no gain. However, demanding systems that conform to the very latest C and POSIX.1 standards quickly restricts you to a very small set: many people tend to forget that.

Right, I didn't see bots for this platform and that's why I asked. I'm completely happy if libc++ supports a variety of platforms, IMO more is better because it prevents local-minimum solutions, but this has to be balanced with maintenance cost. Right now so many platforms have a semblance of support without actually being supported that it's a big hindrance to folks working on libc++. Committing a patch might break bots that are unmaintained, or might break configurations for which there are no bots at all (and the we get reports a few weeks after the facts). That's not an ideal situation at all.

If you're actually supporting Solaris, providing bots and fixing incoming breakages then I agree that the platform should be supported. However, it would be good to do more than just support a platform, and actually support libc++ as a whole. There's a significant number of bugs and features which aren't getting the attention they need, in large part because maintainers are busy with the large number of semi-supported platforms.

In other words, I'd much rather see ongoing commitments to the codebase, not just for a single platforms. The reality is that right now many important things are lagging because most folks support their platforms and nothing else. That's far from ideal, but I understand that it's sometimes the only option.