This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Update supported system versions
ClosedPublic

Authored by philnik on Dec 19 2022, 12:59 PM.

Details

Diff Detail

Event Timeline

philnik created this revision.Dec 19 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 12:59 PM
philnik requested review of this revision.Dec 19 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 12:59 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

#libc_vendors Could you help filling the question marks?

libcxx/docs/index.rst
129

This seems to be the oldest glibc version referenced in libc++.

emaste added a subscriber: emaste.Dec 19 2022, 1:18 PM
emaste added inline comments.
libcxx/docs/index.rst
128

FreeBSD 10 and 11 are out of support now, so we likely won't be testing against the libc they came with. I think it would be fine to make this FreeBSD 12+. In addition, FreeBSD uses/supports libc++ across all of our supported architectures -- 32- and 64-bit x86, arm, powerpc, as well as 64-bit riscv.

FreeBSD has an integrated base system that includes the kernel, libc, userland tools etc., and generally doesn't have a concept of alternate libcs -- e.g. FreeBSD 12.3 has FreeBSD 12.3 libc. I'm not sure what we'd put in this table.

Perhaps we should just put the required glibc version in Notes for the Linux row?

MaskRay added inline comments.
libcxx/docs/index.rst
129

CC @dalias @q66 @thesamesam about the supported musl versions.

philnik added inline comments.Dec 19 2022, 1:37 PM
libcxx/docs/index.rst
128

FreeBSD 10 and 11 are out of support now, so we likely won't be testing against the libc they came with. I think it would be fine to make this FreeBSD 12+. In addition, FreeBSD uses/supports libc++ across all of our supported architectures -- 32- and 64-bit x86, arm, powerpc, as well as 64-bit riscv.

I'll update the version. For the architectures, please provide BuildKite bots, then we can update the list. I'm not 100% sure what the policy for architectures is, but we currently don't have any RISCV bots, so we definitely need one for that. We probably also require one per platform/architecture combination.

FreeBSD has an integrated base system that includes the kernel, libc, userland tools etc., and generally doesn't have a concept of alternate libcs -- e.g. FreeBSD 12.3 has FreeBSD 12.3 libc. I'm not sure what we'd put in this table.

We can just put N/A in the table.

Perhaps we should just put the required glibc version in Notes for the Linux row?

Let's see what other people have to say here. If this indeed only affects Linux we can do that, but I expect that it'll also affect at least Windows due to MinGW. I think MSVCRT also isn't bound to a windows version. @mstorsjo probably has more insight there.

philnik added inline comments.Dec 19 2022, 1:39 PM
libcxx/docs/index.rst
129

We don't have any bots testing musl + libc++, so I wouldn't consider musl to be supported currently. I think it might actually be broken in trunk. Maybe someone can provide a bot to test this configuration?

For the architectures, please provide BuildKite bots

Yes, first step is to get the x86-64 FreeBSD 13.1 bot going, D128084. Adding arm64 should be straightforward as a second one, others may take longer.

mstorsjo added inline comments.Dec 20 2022, 3:04 AM
libcxx/docs/index.rst
130

For the tested and supported C runtime on Windows, I'd just write UCRT - that's the one used both by MSVC and mingw setups (although with a little bit of different wrapping around it in the mingw case).

In mingw configurations, it's also buildable with msvcrt.dll - I do test building it regularly (every other day more or less), but that configuration doesn't pass all tests and isn't one of the formally supported configs.

philnik added inline comments.Dec 20 2022, 4:28 AM
libcxx/docs/index.rst
130

Is there any versioning for UCRT, and if yes - which ones work currently?

phosek added a subscriber: phosek.Dec 20 2022, 5:45 AM
phosek added inline comments.
libcxx/docs/index.rst
132

Would it be possible to also include Fuchsia in the list? Fuchsia is in the same category as FreeBSD where libc is part of the base system so the "supported libcs" should probably just say Fuchsia (or N/A or whatever we decide).

philnik added inline comments.Dec 20 2022, 5:57 AM
libcxx/docs/index.rst
132

If we get a Fuchsia BuildKite bot we can do that. See https://libcxx.llvm.org/AddingNewCIJobs.html#addingnewcijobs for details.

mstorsjo added inline comments.
libcxx/docs/index.rst
130

There is some versioning of it I think - as it's shipped as part of Windows and/or the Windows SDK, it does have version numbers matching the Windows builds, like e.g. 10.0.19041.0. However the version number is seldom shown/used very visibly in UCRT contexts.

I'm not sure exactly which version ends up tested in the CI - it depends on whatever version of Windows is used in the CI base images/runners (which @goncharov maintains). I guess it'd be possible to dig it up somehow...

Practically, I would expect all versions of UCRT to "work" (it's been present since 2015 since the launch of Windows 10), while older ones might fail some corner case tests. (We also do have detection of some known UCRT bugs which is used to make the tests "pass" - so older versions might be considered passing to the same extent if we'd add similar detection for their issues.)

ldionne added inline comments.Dec 20 2022, 4:53 PM
libcxx/docs/index.rst
127

For macOS, there's only the system libc, so it doesn't really make much sense. In other words, by saying macOS 10.9 +, we are basically specifying what libcs are supported.

128

Perhaps we should just put the required glibc version in Notes for the Linux row?

This is exactly what I was going to suggest -- IMO that makes the most sense.

goncharov added inline comments.Dec 21 2022, 7:15 AM
libcxx/docs/index.rst
130

I see that cmake reports

  • The C compiler identification is MSVC 19.29.30141.0
  • The CXX compiler identification is MSVC 19.29.30141.0
  • The ASM compiler identification is MSVC
  • Found assembler: C:/BuildTools/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe

is that what you are looking for?

mstorsjo added inline comments.Dec 22 2022, 2:20 AM
libcxx/docs/index.rst
130

No, those are different numbers.

There's one version number for the version of the WinSDK (which is installed as part of MSVC), normally not very visibly printed but easy to locate on disk (e.g. in paths such as Program Files (x86)/Windows Kits/10/lib/10.0.19041.0), and one version number for the actual running version of the OS (easily visible in e.g. the control panel, not quite as easily accessible via code).

If we'd link the UCRT statically (the libcxx tests don't), the version of WinSDK would be what matters, but here, it's the OS build version that matters.

I guess the simplest would be to figure out a way to check the exact OS build version via code, somehow, and include that with a printout in a temporary patch triggering a CI run.

goncharov added inline comments.Dec 22 2022, 2:27 AM
libcxx/docs/index.rst
130

sure, I can easily plug this to buildkite steps if that helps.

according to stackoverflow https://stackoverflow.com/questions/2665755/how-to-get-installed-windows-sdk-version
is can be done by
$(Get-Item "hklm:\SOFTWARE\Microsoft\Microsoft SDKs\Windows").GetValue("CurrentVersion")

have not tried. Is that the value you are looking for?

mstorsjo added inline comments.Dec 22 2022, 2:39 AM
libcxx/docs/index.rst
130

That would probably be the SDK version, but for the purposes of this document, we'd need to know the OS build version instead.

The CurrentVersion key in HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion should hold that value.

Other commands that would print the relevant info seem to be either of these:

> wmic Path CIM_DataFile WHERE Name='c:\\windows\\explorer.exe' Get Version | find "10"
> wmic os get BuildNumber
> systeminfo | findstr /B /C:"OS Version"
alvinhochun added inline comments.
libcxx/docs/index.rst
130

It seems slightly more complicated than that, I think. See https://learn.microsoft.com/en-us/cpp/windows/universal-crt-deployment?view=msvc-170

For the system-deployed UCRT, it's only tied to the OS version starting from Windows 10. For Windows Vista though Windows 8.1 it was distributed via Windows Update, with the latest version being 10.0.14393. But even for Windows 10 it's not exactly tied to the system version -- my Windows install is currently 21H2 (10.0.19044.2486) while C:\Windows\System32\ucrtbase.dll has the version 10.0.19041.789. I believe the version of ucrtbase.dll is what matters.

(Local deployment is also possible, but the linked page states that it only works for Windows versions before 10, as Windows 10 and 11 will always use the system UCRT, so it doesn't change much.)

daltenty added inline comments.
libcxx/docs/index.rst
131

For AIX the version here should be 7.2 TL5 and greater, that's the version the bots are running. Similar to some others, only the system libc is currently supported.

philnik updated this revision to Diff 501238.Feb 28 2023, 11:25 AM
philnik marked 10 inline comments as done.

Address comments

philnik retitled this revision from [libc++] List supported libcs to [libc++] Update supported system versions.Feb 28 2023, 11:25 AM
philnik edited the summary of this revision. (Show Details)
emaste accepted this revision.Feb 28 2023, 1:41 PM

LGTM for FreeBSD

daltenty accepted this revision.Feb 28 2023, 2:28 PM

LGTM for AIX

fsb4000 added inline comments.
libcxx/docs/index.rst
122

Maybe we should write minimal Windows version too, shouldn't we?

Is Windows 7 still supported?

thesamesam added inline comments.Feb 28 2023, 5:10 PM
libcxx/docs/index.rst
129

Does unsupported here mean "we expect help to fix things" or "we'll reject patches" (like we discussed in previous encounters)?

I think trunk works fine last time I checked.

I'm just not sure what the purpose of this change is with regard to libc. I don't expect the libc++ team to rush to fix musl bugs, just accept patches if we provide them.

I'd like to provide a buildbot but I'm not sure which hardware we could do it on right now.

philnik added inline comments.Feb 28 2023, 6:01 PM
libcxx/docs/index.rst
129

IMO it should definitely be "we'll reject patches", since that's the stance we have on any platforms, which a different libc effectively is. I guess I'll have a discussion with Louis about this at some point.
As I'm sure I said before, the problem is that we don't know whether the code actually works and we have no way to check. This in turn means that we either don't touch the code, or we break people and then get to know about it a year later. Neither of which is healthy for a project. Another problem is that we don't know whether anybody is still interested in a platform, which means we might drag code around that nobody actually cares about (we removed a lot of that since switching to a stricter compiler and platform support policy).

If you want to provide a builkite bot we're happy to help with setting things up.

mstorsjo added inline comments.Feb 28 2023, 11:14 PM
libcxx/docs/index.rst
129

I wholeheartedly disagree that the policy should be "we'll reject patches" for anything outside of this list. There should clearly be more nuance than that to it.

For example - currently the CI covers some OSes on some architectures. For Linux, this is 4 specific architectures right now. We clearly shouldn't go ahead and add an #if defined(__linux__) && !(defined(__i386__) || defined(__x86_64__) || defined(__arm__) || defined(__aarch64__)) #error Unsupported configuration, not allowed to use libc++ there!. I'm sure lots of Linux distros regularly build and use libc++ on other architectures than that. Do we guarantee that it works? No. Does it work in practice? Most probably.

E.g. for Windows on ARM64, I do build latest libc++ nightly, and run some amounts of tests with it (actual usage tests, not the libc++ testsuite itself - but I do test that one occasionally too), so it's not entirely broken. And if that gets broken, I'll let you know within 24h. It's not worthwhile to add an #error unsupported for such configurations, I hope you agree?

I totally agree that it's problematic to maintain the code when we don't know if it works or not, and if such code gets in the way for refactorings without a reasonable way forward, removing it might be the only option.

I get that there's a difference between untested variants of support for otherwise supported OSes already in the tree, and adding new code for entirely different OSes. But if that code mainly provides a different OS/libc specific handling of something that is already customized for all targets, I don't see a big issue with it.

There's a whole range of potential configurations inbetween, with various level of support. For new configurations, maybe aspiring to reach a level where it's included here with CI coverage in the future, it's hard to get there if any patches whatsoever are rejected unless the CI setup is there to begin with.

I.e., don't let perfect be the enemy of good. If it can provide value to users without causing maintainance harm, I don't think it needs to be rejected out of principle. If it causes maintainance issues, it should of course be reconsidered.

mstorsjo added inline comments.Feb 28 2023, 11:56 PM
libcxx/docs/index.rst
122

I guess we could mention the minimum Windows version, yes. AFAIK the baseline is Windows 7.

Mordante added inline comments.Mar 1 2023, 4:05 AM
libcxx/docs/index.rst
129

IMO it should definitely be "we'll reject patches", since that's the stance we have on any platforms, which a different libc effectively is. I guess I'll have a discussion with Louis about this at some point.

I'm not aware we (libc++) have that policy. I think it would be good for us to discuss that. Of course without CI it's not always possible to verify the patch fixes what it should, but we can verify it doesn't break an official supported platform.

In the past I have accepted patches for platforms we don't officially support. However when we don't officially support a platform, I will not consider that platform when working on patches. For example for formatting I need to support locales. These are far from portable, abbreviations of the day/month in the French locale looks different on MacOS and Linux. The CI will tell me what I need to adjust to fix all supported platforms.

I still prefer a CI. For officially supporting a platform I see that as required.

q66 added inline comments.Mar 1 2023, 4:19 AM
libcxx/docs/index.rst
129

I still prefer a CI. For officially supporting a platform I see that as required.

of course, a CI is always better, and it would be great to have one for musl, however, providing a bot is something small projects can't always afford to do (for instance I'm a maintainer of a Linux distribution that extensively relies on LLVM as its system toolchain, including libc++, and I really have enough pain thrown in my way as things are, and such policy would make me waste even more time on just proving the project's right to exist rather than working on anything more useful)

therefore i would greatly appreciate if the status quo was preserved; right now libc++ works great for me, and I am fine with providing patches for whenever something breaks, but please don't make me maintain separate support downstream, i really have enough to do already :)

philnik added inline comments.Mar 1 2023, 7:13 AM
libcxx/docs/index.rst
129

I wholeheartedly disagree that the policy should be "we'll reject patches" for anything outside of this list. There should clearly be more nuance than that to it.

For example - currently the CI covers some OSes on some architectures. For Linux, this is 4 specific architectures right now. We clearly shouldn't go ahead and add an #if defined(__linux__) && !(defined(__i386__) || defined(__x86_64__) || defined(__arm__) || defined(__aarch64__)) #error Unsupported configuration, not allowed to use libc++ there!. I'm sure lots of Linux distros regularly build and use libc++ on other architectures than that. Do we guarantee that it works? No. Does it work in practice? Most probably.

E.g. for Windows on ARM64, I do build latest libc++ nightly, and run some amounts of tests with it (actual usage tests, not the libc++ testsuite itself - but I do test that one occasionally too), so it's not entirely broken. And if that gets broken, I'll let you know within 24h. It's not worthwhile to add an #error unsupported for such configurations, I hope you agree?

I didn't want to say that we should go out of our way to break people. If it came across that way I'm sorry.

I totally agree that it's problematic to maintain the code when we don't know if it works or not, and if such code gets in the way for refactorings without a reasonable way forward, removing it might be the only option.

I get that there's a difference between untested variants of support for otherwise supported OSes already in the tree, and adding new code for entirely different OSes. But if that code mainly provides a different OS/libc specific handling of something that is already customized for all targets, I don't see a big issue with it.

There's a whole range of potential configurations inbetween, with various level of support. For new configurations, maybe aspiring to reach a level where it's included here with CI coverage in the future, it's hard to get there if any patches whatsoever are rejected unless the CI setup is there to begin with.

As I understand it, we accept patches if someone is working on getting a bot up. For example, there were quite a few patches landed for FreeBSD, without a bot currently there. The important part is that there is an effort to get us a bot.

I.e., don't let perfect be the enemy of good. If it can provide value to users without causing maintainance harm, I don't think it needs to be rejected out of principle. If it causes maintainance issues, it should of course be reconsidered.

That is the point where we are right now IMO. I'm not confident to refactor any of the locale stuff because we have lots of code that is basically not tested. For musl we even have CMake stuff.

To be clear, I'd be more than happy to have a minimal bot for musl where everything other than locale support is disabled. That would resolve any maintainability problems.

I still prefer a CI. For officially supporting a platform I see that as required.

of course, a CI is always better, and it would be great to have one for musl, however, providing a bot is something small projects can't always afford to do (for instance I'm a maintainer of a Linux distribution that extensively relies on LLVM as its system toolchain, including libc++, and I really have enough pain thrown in my way as things are, and such policy would make me waste even more time on just proving the project's right to exist rather than working on anything more useful)

therefore i would greatly appreciate if the status quo was preserved; right now libc++ works great for me, and I am fine with providing patches for whenever something breaks, but please don't make me maintain separate support downstream, i really have enough to do already :)

I get that it seems easier to have the code upstream, but I'm not sure it actually is. I don't think for you there is a significant difference. Currently, you update libc++ at some point, and might see that something broke. Then you have to create a patch to fix it and upstream that fix. With a downsteam fork, you instead update libc++ at some point and if something broke you fix it downstream. That would result in duplicated work, but that could be resolved by a semi-official musl fork. Then we don't have the problem of having to keep dead code around, but people who want libc++ to work in some specific configuration can still get it without too much work (assuming someone already did the work).

mstorsjo added inline comments.Mar 1 2023, 7:56 AM
libcxx/docs/index.rst
129

Thanks for the follow up - I guess our views aren’t that far apart after all :-)

Wrt refactoring, anything not covered by CI is quite expressly not supported, so you can essentially disregard it entirely. If it does break, an active downstream will send a patch within a week. If downstream doesn’t follow up, it is of course ripe to remove after some time.

I think there’s still a lot of value to have unofficial ports working in the upstream repo - if it works 90% of the time (especially around releases) it is tremendously useful already, compared to an outright downstream fork.

Plus it gives upstream visibility into what the unofficial ports do.

ldionne accepted this revision.Mar 2 2023, 9:03 AM

LGTM with the wording adjustment. This will also need to be rebased, I think.

libcxx/docs/index.rst
121

Our current policy for supporting other OSes/libcs/configurations is as @philnik stated. It doesn't mean that we'll go out of our way to break unsupported configurations, but it does mean that:

  1. We won't let any such configuration get in the way of improvements, refactorings, reorganizations, deprecations, removals.
  2. We won't accept any non-trivial or widespread changes for that platform.
  3. We won't list it as supported here.

We're not against unofficial ports, in fact I even have several downstream ports that would be considered "unsupported" by the definition above. This is fine. All I am saying is that it's not reasonable to put the burden of maintenance on the upstream project. If people have small patches that e.g. add an architecture to an existing #if defined(FOO) || defined(BAR) || ..., I don't think you'll be met with too much resistance. However, I'm deliberately not giving any guarantees here so that we can look at it on a case by case basis and make the best decision for the project every time.

To please everyone, I think let's just be a bit hand-wavy in the documentation w.r.t. what we mean by "officially supported". This is the status quo and it works reasonably well (if not someone please raise that).

To address @philnik 's concern about __locale specifically, I agree it's a freakin' mess over there. I think it should be possible to refactor in a way that we hide much of the platform-specific stuff away of our main implementation, untying our hands to touch __locale. In the current state of things, honestly I don't think any frequent contributor feels confident modifying our localization code, and that's a problem (not only theoretical -- I had to do some porting work recently and it was a real hindrance). If we can't refactor that effectively, then I would go for something more aggressive and prune configurations that are not supported officially, basically changing above back to Only glibc-2.24 and later and no other libc is supported.

This revision is now accepted and ready to land.Mar 2 2023, 9:03 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.