This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove Solaris related code
ClosedPublic

Authored by ldionne on Nov 24 2022, 10:07 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG3b6bc8752027: [libc++] Remove Solaris related code
Summary

This was contributed ~10 years ago, but we don't officially support it
and I am not aware of any bot testing it, so this has likely rotten to
the point where it is unusable.

Diff Detail

Event Timeline

ldionne created this revision.Nov 24 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 10:07 AM
ldionne requested review of this revision.Nov 24 2022, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 10:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

This is mostly for discussion. I'm trying to figure out what's the status of that platform, and whether that is still relevant. @brad and @theraven have provided patches for that platform in the past, so I'm hoping they can provide additional context. Is this used? Does this work? If so, is it possible to get it officially supported?

ldionne added a subscriber: Restricted Project.Dec 5 2022, 6:15 AM

Adding libc++ vendors for visibility. Is anyone using this?

ldionne updated this revision to Diff 480084.Dec 5 2022, 6:17 AM

Rebase and fix CI issue.

tBricks was using this, but I think they've migrated off Solaris now. I'll check. I haven't had a Solaris system for a very long time.

tBricks was using this, but I think they've migrated off Solaris now. I'll check. I haven't had a Solaris system for a very long time.

They've confirmed that they are no longer using it. They were using it with libcxxrt, not libc++abi, so I have no idea if libc++abi ever worked with Solaris.

I am happy for it to be removed if no one steps up to maintain it. The various OpenSolaris derivatives' libc implementations have diverged enough that they're almost certainly not using this.

I believe @voskresensky.vladimir may have some idea whether Oracle is interested in maintaining any of this but if not then removing support seems the right path.

It looks as if Solaris added POSIX2008 locale support at some point in the last 12 years and so if anyone *is* interested in having it officially supported, then most of these hacks can be removed.

That's great, thanks a lot for the feedback. I'll wait a few days and then land this if nobody objects.

I think @ro is doing builds of LLVM for Solaris, not sure if libcxx is included there or not though - I'd request that you don't proceed until he has replied at least.

@ro I'll commit this on Monday December 12 unless someone objects. FWIW the comments made by other folks make sense -- this is old and hasn't been maintained, I would be rather surprised if it worked out of the box. But like I said, I'll wait for another week before proceeding.

@ro, I'm happy to help out on a more up-to-date Solaris port if someone wants to do one.

ro added a subscriber: jclulow.Dec 6 2022, 7:07 AM

As usual, it's complicated ;-)

  • While I do run two Solaris buildbots (sparcv9 and amd64), those only include llvm, clang, and compiler-rt.
  • The libcxx port isn't for Solaris only, but for Illumos (OpenSolaris derivative), too, so Solaris isn't the only stakeholder here. In fact, IIRC, the Illumos port was done in a way to break the Solaris-specific code, not particularly friendly IMO.
  • About two years ago, I tried myself at reviving the Solaris port while keeping the Illumos port working (although I believe it also had become broken at that point). I got it working on both OSes, and the test results weren't too bad. At that time, I even included support for Solaris 11.3, which, unlike 11.4, lacks XPG7 locale support. However, the 11.3 part would be ripped out today since I'm about to obsolete everything before 11.4 in GCC 13.The differences between Solaris and Illumos were minor at that point: only one of the 14-part patch series I came up with was Illumos-specific.
  • To test the waters, I sumitted the first patch in the series (D81602) to learn if the libcxx community would be amenable to reenabling the port.

However, as a prerequisite to move that patch (and the rest) forward, I learned that I would be required to setup Solaris instances of the libcxx CI. Unfortunately, that subproject uses buildkite, yet another buildbot besides LLVM's buildbot (and Golang's builder), which isn't even ported to Solaris (and won't build last time I tried). Add to that the fact that I'm already swamped with Solaris GCC, LLVM, and GDB maintenance and that my knowledge of C++ is minimal, I don't seem myself in the position to handle all that (getting the port integrated, setting up CI,...).

I can ask if someone inside Oracle is interested in taking up those tasks (I could host the CI, but nothing more). Unless there's someone to handle this, I'd have to decline.

However, this is only part of the deal: even if the Solaris port per se would be unmaintained, there's still the Illumos community which might be interested in taking this up. @jclulow might have suggestions here.

It would certainly good to have this port working, both for Solaris/Illumos and for libcxx: diversity usually helps pinpoint unwarranted assumptions in the codebase.

jperkin added a subscriber: jperkin.Dec 7 2022, 2:24 AM

I, and many others in the illumos community, would be grateful if you'd hold off on doing this. I've maintained the full set of clang/llvm packages for illumos systems for many years, and it provides a useful alternative to GCC, with potentially one day switching over fully.

There are only minor changes required to ensure the 15.x series continues to build on illumos (14.x were fine):

  • Apply the two patches below:
--- include/__locale.orig       2022-11-29 10:05:58.000000000 +0000
+++ include/__locale
@@ -495,7 +495,7 @@ public:
 # if defined(_AIX)
     static const mask __regex_word = 0x8000;
 # else
-    static const mask __regex_word = 0x80;
+    static const mask __regex_word = 0x80000;
 # endif
 #elif defined(_NEWLIB_VERSION)
     // Same type as Newlib's _ctype_ array in newlib/libc/include/ctype.h.
--- include/__support/solaris/xlocale.h.orig    2022-11-29 10:05:58.000000000 +0000
+++ include/__support/solaris/xlocale.h
@@ -13,6 +13,9 @@
 #ifndef __XLOCALE_H_INCLUDED
 #define __XLOCALE_H_INCLUDED
 
+#include <cstdlib>
+#include <cwchar>
+#include <__support/xlocale/__strtonum_fallback.h>
 #include <stdlib.h>
 
 #ifdef __cplusplus
@@ -31,6 +34,7 @@ int tolower_l(int __c, locale_t __l);
 struct lconv *localeconv(void);
 struct lconv *localeconv_l(locale_t __l);
 
+#if 0
 // FIXME: These are quick-and-dirty hacks to make things pretend to work
 inline _LIBCPP_HIDE_FROM_ABI long long
 strtoll_l(const char *__nptr, char **__endptr, int __base, locale_t __loc) {
@@ -66,6 +70,7 @@ inline _LIBCPP_HIDE_FROM_ABI long double
 strtold_l(const char *__nptr, char **__endptr, locale_t __loc) {
   return ::strtold(__nptr, __endptr);
 }
+#endif
 
 
 #ifdef __cplusplus
  • Build with -D_LIBCPP_NO_ABI_TAG=1, otherwise there are error: 'abi_tag' attribute applied to extern "C" declaration errors.

Obviously a proper patch would simply remove the duplicate strtonum functions, my patch is optimised for size.

Thanks.

Thanks for the additional context.

It would certainly good to have this port working, both for Solaris/Illumos and for libcxx: diversity usually helps pinpoint unwarranted assumptions in the codebase.

I agree that diversity of platforms is great. However, for it to be useful for the project, there needs to be a feedback loop that allows the project to catch these potential unwarranted assumptions. And for that to be useful for users, there needs to be a guard that ensures that the project keeps working on the platform. That's what CI is for, and that's the requirement that we impose systematically on platforms. Otherwise, code rots and it provides little to no benefit for anyone, in addition to dragging the project down. Since we support many many platforms, we need to abide by this rule extremely diligently or we risk rotting parts of the code base.

In a way, that's why enthusiast ports are sometimes not sustainable unless a serious investment made to back it and maintain it in the longer term. This is usually not something that a single individual can provide, and understandably so.

Unfortunately, that subproject uses buildkite, yet another buildbot besides LLVM's buildbot (and Golang's builder), which isn't even ported to Solaris (and won't build last time I tried).

If there is truly a desire to set up a CI bot and the only blocker is that buildkite-agent doesn't work on Solaris (is that really the situation?), we can work together to find ways around that. We can bend the rules of libc++ so that platforms where buildkite isn't available use another system, or maybe that's even the sign that we need to move towards something more flexible like GitHub actions.

If someone steps up to officially say "yes, I'll be the maintainer for Solaris and I'll work with the libc++ community to set up a CI bot of some sort", then we're good. If nobody is willing or able to step forward, we'll ship this patch and stop pretending that we "support" that platform.

In that case, it will be much more productive for whoever is interested in Solaris support to maintain an unofficial port on the side. What we could actually do is add a page for "Unofficial ports of libc++" to the libc++ documentation, alongside with links to a website, forum or e-mail that people can reach out to to learn more. This would provide a nice way to make unofficial ports discoverable without trying to keep them in the main tree in vain.

Unfortunately, that subproject uses buildkite, yet another buildbot besides LLVM's buildbot (and Golang's builder), which isn't even ported to Solaris (and won't build last time I tried).

If there is truly a desire to set up a CI bot and the only blocker is that buildkite-agent doesn't work on Solaris (is that really the situation?), we can work together to find ways around that.

FWIW buildkite builds fine on illumos with a very small patch (I'll look separately at fixing that properly):

$ uname -rsv
SunOS 5.11 joyent_20220728T031731Z

$ /buildkite-agent -v
buildkite-agent version 3.41.0, build x

We can either run an agent at MNX or @jclulow may have something appropriate in the illumos.org infrastructure.

We can either run an agent at MNX or @jclulow may have something appropriate in the illumos.org infrastructure.

Perhaps we can do both, even: I could run something with stock illumos-gate and/or OmniOS on Oxide-provided infrastructure, and you could run a SmartOS system at MNX.

We can either run an agent at MNX or @jclulow may have something appropriate in the illumos.org infrastructure.

Perhaps we can do both, even: I could run something with stock illumos-gate and/or OmniOS on Oxide-provided infrastructure, and you could run a SmartOS system at MNX.

Let's get this set up, then. Reach out to me privately on Discord and I'll send you a BuildKite Agent token, and we can go from there.

I just want to ping this thread now that most folks are back from the holidays. Is there concrete interest in setting something up? If not, I'm happy to set up documentation for unofficial ports where we can list yours.

I'll wait until we branch for LLVM 16 and then ship this if there's no additional activity.

ldionne updated this revision to Diff 493310.Jan 30 2023, 7:15 AM

Rebase onto main and fix conflicts.

Please let me know if someone would like to take up my offer of setting up documentation on unofficial ports.

Please let me know if someone would like to take up my offer of setting up documentation on unofficial ports.

Hey, I tried to send you DMs on the LLVM Discord but I'm not sure if I'm holding it correctly -- did you get them?

Please let me know if someone would like to take up my offer of setting up documentation on unofficial ports.

Hey, I tried to send you DMs on the LLVM Discord but I'm not sure if I'm holding it correctly -- did you get them?

Ah, yes I see it now! It was in my "DM Requests". Will reply there, putting this on hold for the time being.

ldionne updated this revision to Diff 519637.May 4 2023, 1:46 PM

Rebase. There has been no update in a few months, so I think it is time to land this.

ldionne updated this revision to Diff 519667.May 4 2023, 2:55 PM

Remove more files and fix generated-files

ldionne accepted this revision.May 5 2023, 5:39 AM
This revision is now accepted and ready to land.May 5 2023, 5:39 AM
This revision was landed with ongoing or failed builds.May 5 2023, 5:40 AM
This revision was automatically updated to reflect the committed changes.
libcxx/src/support/solaris/README.txt