Skip to content

[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

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Oct 18, 2024

This was slated for removal years ago, so now's a good time to remove it.

@ldionne ldionne requested a review from a team as a code owner October 18, 2024 18:23
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 18, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This 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:

  • (modified) libcxx/docs/ReleaseNotes/20.rst (+3)
  • (modified) libcxx/include/__configuration/availability.h (-8)
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)

Comment on lines +88 to +106
- 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.
Copy link
Contributor

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.

CC @isuruf @xhochy @beckermr @jakirkham

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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!

Copy link
Contributor

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?

Copy link
Member Author

@ldionne ldionne Nov 4, 2024

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.

Copy link
Contributor

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

@ldionne
Copy link
Member Author

ldionne commented Oct 21, 2024

CC @llvm/libcxx-vendors for awareness

@ldionne
Copy link
Member Author

ldionne commented Oct 29, 2024

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

@h-vetinari
Copy link
Contributor

@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.
@ldionne ldionne force-pushed the review/remove-disable-availability branch from 1733145 to 14d1e52 Compare November 19, 2024 23:39
@ldionne
Copy link
Member Author

ldionne commented Nov 19, 2024

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

@h-vetinari
Copy link
Contributor

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)

@ldionne ldionne merged commit afae1a5 into llvm:main Nov 24, 2024
59 of 63 checks passed
@ldionne ldionne deleted the review/remove-disable-availability branch November 24, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants