-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[libc++] Remove _LIBCPP_DISABLE_AVAILABILITY macro #112952
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
Conversation
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThis was slated for removal years ago, so now's a good time to remove it. Full diff: https://github.com/llvm/llvm-project/pull/112952.diff 2 Files Affected:
diff --git a/libcxx/docs/ReleaseNotes/20.rst b/libcxx/docs/ReleaseNotes/20.rst
index 44912d2ddafac6..a212921ee1d73c 100644
--- a/libcxx/docs/ReleaseNotes/20.rst
+++ b/libcxx/docs/ReleaseNotes/20.rst
@@ -85,6 +85,9 @@ Deprecations and Removals
- The function ``__libcpp_verbose_abort()`` is now ``noexcept``, to match ``std::terminate()``. (The combination of
``noexcept`` and ``[[noreturn]]`` has special significance for function effects analysis.)
+- The ``_LIBCPP_DISABLE_AVAILABILITY`` macro that was used to force-disable availability markup has now been removed.
+ Whether availability markup is used by the library is now solely controlled at configuration-time.
+
Upcoming Deprecations and Removals
----------------------------------
diff --git a/libcxx/include/__configuration/availability.h b/libcxx/include/__configuration/availability.h
index f42ff460db4544..8dee132e2b0803 100644
--- a/libcxx/include/__configuration/availability.h
+++ b/libcxx/include/__configuration/availability.h
@@ -67,14 +67,6 @@
//
// [1]: https://clang.llvm.org/docs/AttributeReference.html#availability
-// For backwards compatibility, allow users to define _LIBCPP_DISABLE_AVAILABILITY
-// for a while.
-#if defined(_LIBCPP_DISABLE_AVAILABILITY)
-# if !defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
-# define _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
-# endif
-#endif
-
// Availability markup is disabled when building the library, or when a non-Clang
// compiler is used because only Clang supports the necessary attributes.
#if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCXXABI_BUILDING_LIBRARY) || !defined(_LIBCPP_COMPILER_CLANG_BASED)
|
- The ``_LIBCPP_DISABLE_AVAILABILITY`` macro that was used to force-disable availability markup has now been removed. | ||
Whether availability markup is used by the library is now solely controlled at configuration-time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the setup in our distribution pretty essentially. We always ship an up-to-date libc++, but keep compiling for pretty old deployment targets (currently still on 10.13). The only way to make many current-gen C++ libraries work under those constraints is to disable the availability annotations, which has been how we've been supporting both modern packages AND old hardware (docs). This happens at build time for said library, but at runtime from the POV of the already-compiled libc++.
We currently have >250 packages making use of this. What would be an alternative in a world after this PR? Just define _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
?
I realize that we're not a common use-case in and of ourselves, but we do have >2B of monthly downloads (across all platforms, not just osx), so a relatively big multiplier. We believe that keeping compatibility for as long as feasible is an important quality, and since we've got the infrastructural wherewithal to solve the technical issues, it would be a pity to shut this door.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you ship your own libc++ you shouldn't enable availability annotations. I don't know exactly what kind of hacky solution you're using, but you probably want to inject your own __config_site
. That's obviously not officially sanctioned, but should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly what kind of hacky solution you're using, but you probably want to inject your own
__config_site
.
If that makes it possible to tell clang to ignore the system libcxx and use the one we're shipping, that'd be great! That's really the key issue we're working around.
If you ship your own libc++ you shouldn't enable availability annotations.
Yeah, I can understand that. Our case is simple though - our availability annotations are equivalent to what the current(==newest) version of libcxx supports, so it's equivalent to _LIBCPP_DISABLE_AVAILABILITY
for that version AFAIU. (and there's some details w.r.t. libc++abi that I'm leaving out, see discussion in #110920).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h-vetinari I think what you want here is simply to define LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS=OFF
at CMake configuration time. In fact, LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS=OFF
is the default, so I think there's something I'm not understanding about your use case. Why do you manually enable LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS=ON
and then attempt to disable it via some other mechanism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldionne I think they actually use the Apple SDK headers but ship their own libc++.so to support some programs on older platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, -nostdinc++
works when a SDK is in use. There's nothing different when a SDK is in use, really: it's just that the sysroot where Clang starts looking for stuff is located inside the SDK, but all the behavior is otherwise the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to answer the questions w.r.t. our setup (to the best of my understanding, much of the setup was done before my time, I'm trying to document & improve things as I go).
Because our users are necessarily installing a full environment (where we control dependencies and distribution), we can rely on the fact that we can always ship an up-to-date libc++.dylib
. We also use up-to-date the libc++ headers when compiling things, but we still need to use point to some Apple SDK (for which older versions are available, e.g. from https://github.com/phracker/MacOSX-SDKs or https://github.com/alexey-lysiuk/macos-sdk), and we're generally on a 10.13 baseline for now.
We need the SDK for platform-related things and the C stdlib, so this is kind of unavoidable. However, the libc++ headers will trigger availability warnings based on the detected macOS version and therefore report false positives (from our POV, because our C++ stdlib is always up-to-date and available).
These things happen at runtime (from the POV of libc++), because different packages may require a newer SDK for some reason, and we can provide that based on the configuration of a given build (often while still leaving MACOSX_DEPLOYMENT_TARGET
unchanged).
We're also relying on ABI stability of libc++, in the sense that artefacts compiled against libc++ vN get a runtime dependency libcxx >=N
, so we that if we later have libc++.{N+2}.dylib
in the environment, things still work in the vast majority of situations. I don't want to scare people here in that this is what the ironclad expectation is, but rather that this is something that has worked for us for years, and rebuilding literally 10's of thousands of artefacts for every new libc++ version is simply not realistic.
So if _LIBCPP_DISABLE_AVAILABILITY
gets removed, AFAIU we'd have to either:
- Patch things so that we recreated the effect of
_LIBCPP_DISABLE_AVAILABILITY
- Provide our own vendor availability annotations that match that effect
Neither sounds impossible, but we try to minimize the amount of patches we carry, obviously...
Actually there is one exception where we do modify the default availability annotations, which has to do with another wrinkle. While we could build and ship libc++abi
just fine (and we did in the past), there are situations where certain libraries or frameworks (including CPython) will bypass the fact that everything points to our custom $PREFIX
and simply go to the system libc++abi
straight away. Because it's basically impossible to chase down and patch all cases where this happens, we've stopped shipping libc++abi
and now always rely on the system-provided one, which is why we unconditionally disable _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION
for example, as the required ___cxa_init_primary_exception
isn't available before macOS 15 AFAIU (and our deployment target baseline is much lower). This is also why I'm interested in -DLIBCXX_CXX_ABI=system-libcxxabi
, c.f. #110920.
Finally, where we're aware of it, we do patch relevant ABI differences for older macOS system libs, compare e.g. this patch to realign a struct, resp. the related discussion (I think the original email thread got split a bit by the discourse migration). This isn't great, but given that the system ABI isn't 100% stable, this is +/- the best we can do under the given constraints.
Hope that explains the situation a bit more. Happy to answer more questions of course, and thanks for giving us a chance to make the case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h-vetinari All of this doesn't explain why you can't use your own libc++ headers via -nostdinc++
. Is it just that you've never though of that or is there an issue with using that strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because our users are necessarily installing a full environment (where we control dependencies and distribution), we can rely on the fact that we can always ship an up-to-date
libc++.dylib
. We also use up-to-date the libc++ headers when compiling things, but we still need to use point to some Apple SDK (for which older versions are available, e.g. from https://github.com/phracker/MacOSX-SDKs or https://github.com/alexey-lysiuk/macos-sdk), and we're generally on a 10.13 baseline for now.
Ok, this makes sense. I assume you're doing something like -isysroot <PATH-TO-OLDER-SDK>
, which means you're getting the C library headers (and other system headers) from that SDK. However, as a side effect, you're also getting the older C++ Standard Library headers located in SDKROOT/usr/include/c++/v1
, and those have undesirable availability markup in them.
You don't want that. In particular, you really really don't want to be using Apple "system libc++" headers from the SDK but linking against your own self-built libc++.a
or libc++.dylib
. That's brittle and can break badly at any point. Those are two distinct libraries and they only happen to share the same name and the same symbol names for the most part, but pedantically you're using entirely incompatible headers and built library. If you want to use the SDK but provide your own C++ Standard Library, you should do this:
-isysroot <SDK>
-nostdinc++ -isystem <path-to-your-libcxx-headers>
-nostdlib++ -L <path-to-your-libcxx-dylib> -l c++
That way, you're overriding both the library and the header search paths to make sure you find consistent ones. I think this is where @philnik777 was going above. Please let me know if this makes sense to you.
Edit: Going even further, it's a shame that we didn't select a different namespace and install-name for the Apple system libc++. It would have made these issues so much clearer. For example, if the Apple system libc++ were using an inline namespace like std::__applesdk::
instead of std::__1::
like upstream LLVM, you'd be getting a clear linker error when you try to mix the two, and many subtly incorrect setups would have been caught. Unfortunately, changing this has serious challenges at this point.
Finally, where we're aware of it, we do patch relevant ABI differences for older macOS system libs, compare e.g. this patch to realign a struct, resp. the related discussion (I think the original email thread got split a bit by the discourse migration). This isn't great, but given that the system ABI isn't 100% stable, this is +/- the best we can do under the given constraints.
We do strive to be 100% backwards compatible. I added a reply to the Discourse thread explaining what I think is happening here and why I don't think that's libc++'s "fault": https://discourse.llvm.org/t/shipping-custom-libc-on-macos/58606/7. TLDR, it looks to me like some programs end up with multiple copies of libc++ in them, which is an ODR violation. Reordering these lines as you do in the patch simply makes that ODR violation be benign and inconsequential, but doesn't technically solve the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the comprehensive response here. It was a fair amount for me to chew on, which is why I ended up postponing having to deal with it a few times - apologies 😅
I'll try to use "just the C bits" of the SDK as you describe. It sounds like a more solid approach already so I'm hoping that'll work. 🤞
Also 💯 on the different symbol namespaces, that would have been excellent. Finally, the case you commented on in discourse really is unusual (also for us), in it's triggered by an uncommon case of flags (using -Wl,-rpath
, but not -L
to point to $PREFIX/lib
, where our libc++
lives.)
CC @llvm/libcxx-vendors for awareness |
@h-vetinari Gentle ping -- I don't want to break your use case, but I'd like to find a path forward for this change. |
Thanks for checking in again! 🙏 I'm trying to gather information to answer this, but have been pretty underwater workwise recently. I should have something tomorrow. |
This was slated for removal years ago, so now's a good time to remove it.
1733145
to
14d1e52
Compare
I am going to merge this next week unless there is new information. I don't mean to put pressure, but we also don't want to hold up cleanups in the project for unsupported use cases (which it seems like this is unless new information is brought). |
Sorry for the slow responses 😑 I'll try to apply the suggestions you made how to deal with the C vs. C++ SDK split. In any case, I'd raise an issue if necessary during the RC phase (or even before, I'm already doing some builds from main for other reasons) |
This was slated for removal years ago, so now's a good time to remove it.