[libc++] Enable availability based on the compiler instead of __has_extension

[ldionne]

__has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like

__has_extension(pragma_clang_attribute_external_declaration)

will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable.

rdar://124078119

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

__has_extension(...) doesn't work as intended when -pedantic-errors is used with Clang. With that flag, __has_extension(...) is equivalent to __has_feature(...), which means that checks like

__has_extension(pragma_clang_attribute_external_declaration)

will return 0. In turn, this has the effect of disabling availability markup in libc++, which is undesirable.

rdar://124078119

---

Full diff: https://github.com/llvm/llvm-project/pull/84065.diff

2 Files Affected:

• (modified) libcxx/include/__availability (+3-4)
• (added) libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp (+22)

diff --git a/libcxx/include/__availability b/libcxx/include/__availability
index 78438c55a3b7ba..bb3ed0a8da521b 100644
--- a/libcxx/include/__availability
+++ b/libcxx/include/__availability
@@ -72,11 +72,10 @@
 #  endif
 #endif
 
-// Availability markup is disabled when building the library, or when the compiler
+// Availability markup is disabled when building the library, or when a non-Clang
+// compiler is used because only Clang supports the necessary attributes.
 // doesn't support the proper attributes.
-#if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCXXABI_BUILDING_LIBRARY) ||                                       \
-    !__has_feature(attribute_availability_with_strict) || !__has_feature(attribute_availability_in_templates) ||       \
-    !__has_extension(pragma_clang_attribute_external_declaration)
+#if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCXXABI_BUILDING_LIBRARY) || !defined(_LIBCPP_COMPILER_CLANG_BASED)
 #  if !defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
 #    define _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
 #  endif
diff --git a/libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp b/libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp
new file mode 100644
index 00000000000000..c55a0a4d6e5d1b
--- /dev/null
+++ b/libcxx/test/libcxx/vendor/apple/availability-with-pedantic-errors.compile.pass.cpp
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: stdlib=apple-libc++
+
+// Test that using -pedantic-errors doesn't turn off availability annotations.
+// This used to be the case because we used __has_extension(...) to enable the
+// availability annotations, and -pedantic-errors changes the behavior of
+// __has_extension(...) in an incompatible way.
+
+// ADDITIONAL_COMPILE_FLAGS: -pedantic-errors
+
+#include <__availability>
+
+#if defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
+#  error Availability annotations should be enabled on Apple platforms in the system configuration!
+#endif

[philnik777]

I feel like we should fix this in Clang rather than in libc++. I think this behaviour is really surprising to people. CC @AaronBallman

[ldionne]

I could agree with that. Here's the analysis I did elsewhere, pasted:

The problem seems to be that _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS is disabled via

#if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCXXABI_BUILDING_LIBRARY) ||                                       \
    !__has_feature(attribute_availability_with_strict) || !__has_feature(attribute_availability_in_templates) ||       \
    !__has_extension(pragma_clang_attribute_external_declaration)
#  if !defined(_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS)
#    define _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
#  endif
#endif

And I am able to reproduce the cause for that here locally with:

cat <<EOF | xcrun clang++ -xc++ - -pedantic -pedantic-errors -fsyntax-only 
#if !__has_extension(pragma_clang_attribute_external_declaration)
#error extension missing
#endif
EOF

I just found this in clang/docs/LanguageExtensions.rst:143:

If the -pedantic-errors option is given, __has_extension is equivalent to __has_feature.

And in clang/lib/Lex/PPMacroExpansion.cpp:1161:

static bool HasExtension(const Preprocessor &PP, StringRef Extension) {
  if (HasFeature(PP, Extension))
    return true;

  // If the use of an extension results in an error diagnostic, extensions are
  // effectively unavailable, so just return false here.
  if (PP.getDiagnostics().getExtensionHandlingBehavior() >=
      diag::Severity::Error)
    return false;

  const LangOptions &LangOpts = PP.getLangOpts();

  ...

So basically, it looks like __has_extension(...) always returns 0 when -pedantic-errors is used. That means that as it stands, libc++ is not compatible at all with -pedantic-errors, since it uses a good number of compiler extensions (and really there’s no way it could go without using compiler extensions).

[philnik777]

That means that as it stands, libc++ is not compatible at all with -pedantic-errors, since it uses a good number of compiler extensions (and really there’s no way it could go without using compiler extensions).

It's not that -pedantic-errors disables extensions, it disables __has_extension properly reporting extensions.

[AaronBallman]

That means that as it stands, libc++ is not compatible at all with -pedantic-errors, since it uses a good number of compiler extensions (and really there’s no way it could go without using compiler extensions).

It's not that -pedantic-errors disables extensions, it disables __has_extension properly reporting extensions.

No, it kind of does disable extensions, which is why __has_extension returns false. When you specify -pedantic-errors, you are saying "any use of an extension is an error" so extensions really aren't available for you. The idea being:

#if __has_extension(foo)
#define FOO use_the_extension
#else
#define FOO use_the_fallback
#endif

should work. If __has_extension returns true with -pedantic-errors, then you'll use the extension and get compile errors as a result instead of using the fallback path.

[philnik777]

That means that as it stands, libc++ is not compatible at all with -pedantic-errors, since it uses a good number of compiler extensions (and really there’s no way it could go without using compiler extensions).

It's not that -pedantic-errors disables extensions, it disables __has_extension properly reporting extensions.

No, it kind of does disable extensions, which is why __has_extension returns false. When you specify -pedantic-errors, you are saying "any use of an extension is an error" so extensions really aren't available for you. The idea being:

#if __has_extension(foo)
#define FOO use_the_extension
#else
#define FOO use_the_fallback
#endif

should work. If __has_extension returns true with -pedantic-errors, then you'll use the extension and get compile errors as a result instead of using the fallback path.

I get the idea, but it doesn't work in practice. The (at least IMO) moral equivalents -Werror=pedantic or -pedantic -Werror don't change how __has_extension behaves, which means that the behaviour doesn't help you. It's only surprising that your code breaks when someone passes -pedantic-errors.

[AaronBallman]

I get the idea, but it doesn't work in practice. The (at least IMO) moral equivalents -Werror=pedantic or -pedantic -Werror don't change how __has_extension behaves, which means that the behaviour doesn't help you. It's only surprising that your code breaks when someone passes -pedantic-errors.

Sure, I think -pedantic-errors, -pedantic -Werror, and -Werror=pedantic should probably all be equivalent in terms of behavior because they're different ways of spelling the exact same thing. However, it's also worth noting, GCC has the same behavior as Clang: https://godbolt.org/z/57ExMncPh

[philnik777]

I get the idea, but it doesn't work in practice. The (at least IMO) moral equivalents -Werror=pedantic or -pedantic -Werror don't change how __has_extension behaves, which means that the behaviour doesn't help you. It's only surprising that your code breaks when someone passes -pedantic-errors.

Sure, I think -pedantic-errors, -pedantic -Werror, and -Werror=pedantic should probably all be equivalent in terms of behavior because they're different ways of spelling the exact same thing. However, it's also worth noting, GCC has the same behavior as Clang: https://godbolt.org/z/57ExMncPh

Given that it's just been added in GCC trunk (I'm quite surprised that got in) and the documentation says that it's for compatibility with clang, this isn't really surprising.

[AaronBallman]

Given that it's just been added in GCC trunk (I'm quite surprised that got in) and the documentation says that it's for compatibility with clang, this isn't really surprising.

I was surprised at how recently it was added to GCC, but what did surprise me was that the behavior was consistent between -pedantic-errors and -pedantic -Werror. Regardless, if we change behavior in Clang, we should be sure to coordinate with the GCC developers so they're aware of the changes.

[philnik777]

I was surprised at how recently it was added to GCC, but what did surprise me was that the behavior was consistent between -pedantic-errors and -pedantic -Werror.

Do you mean compared to clang?

Regardless, if we change behavior in Clang, we should be sure to coordinate with the GCC developers so they're aware of the changes.

Absolutely.

It seems we agree that something should change. Now the question is: what should change? Should we try to find all the possible ways to write -pedantic-errors and make __has_extension change behaviour based on that? Should __has_extension not be influenced by -pedantic-errors? Something I didn't think of yet?

[AaronBallman]

I was surprised at how recently it was added to GCC, but what did surprise me was that the behavior was consistent between -pedantic-errors and -pedantic -Werror.

Do you mean compared to clang?

Yup! (Sorry, that was horribly unclear.)

Regardless, if we change behavior in Clang, we should be sure to coordinate with the GCC developers so they're aware of the changes.

Absolutely.

It seems we agree that something should change. Now the question is: what should change? Should we try to find all the possible ways to write -pedantic-errors and make __has_extension change behaviour based on that? Should __has_extension not be influenced by -pedantic-errors? Something I didn't think of yet?

I can see arguments either way. The argument for making __has_extension neutral from diagnostic flags is that it's answering the question "does this implementation support this extension" not "can I use this extension without getting diagnostics" which is how __has_cpp_attribute works. e.g., https://godbolt.org/z/zMd5W64Ye

I think changing the behavior of __has_extension could break code: https://sourcegraph.com/github.com/ziglang/zig@1e67f5021159ed1d9888cf2d0b9f04ef73222f7d/-/blob/lib/include/sgxintrin.h?L17 (would currently return false with -pedantic-errors and thus not issue any diagnostics, but would issue pedantic error diagnostics if __has_extension began to return true in that case).

On balance, I think __has_extension should be influenced by pedantic errors, however we spell it. I also think __has_feature, __has_cpp_attribute, et al should be made consistent with __has_extension in terms of how we treat pedantic errors. WDYT?

[philnik777]

I can see arguments either way. The argument for making __has_extension neutral from diagnostic flags is that it's answering the question "does this implementation support this extension" not "can I use this extension without getting diagnostics" which is how __has_cpp_attribute works. e.g., https://godbolt.org/z/zMd5W64Ye

From a standards library perspective getting answers to the first question is much more interesting, since most diagnostics won't be issued to users anyways.

I think changing the behavior of __has_extension could break code: https://sourcegraph.com/github.com/ziglang/zig@1e67f5021159ed1d9888cf2d0b9f04ef73222f7d/-/blob/lib/include/sgxintrin.h?L17 (would currently return false with -pedantic-errors and thus not issue any diagnostics, but would issue pedantic error diagnostics if __has_extension began to return true in that case).

Breaking code is definitely not great, especially changing documented behaviour. OTOH: What do users expect? In my experience people tend to not read the documentation of their compilers and standard libraries very closely (I'm very much guilty of that myself).

On balance, I think __has_extension should be influenced by pedantic errors, however we spell it. I also think __has_feature, __has_cpp_attribute, et al should be made consistent with __has_extension in terms of how we treat pedantic errors. WDYT?

Extending the question to __has_feature and __has_cpp_attribute is interesting. If we want to make that consistent, we have to break some code no matter what we do.

All in all, I think answers to both questions can be interesting. I'm just not sure how to answer the second without having a million special cases. Here are a few cases where I'm not sure what the correct answer should be:

• __has_extension(datasizeof), but using __datasizeof doesn't generate any diagnostic (which might be a bug?)

• #pragma clang diagnostic ignored "-Wc11-extensions"
  _Atomic(int) v; // doesn't generate a diagnostic
  auto v = __has_extension(c_atomic); // Should this be 0 or 1?

• // with -std=c++11 -Wno-error=-c++17-attribute-extensions
  [[nodiscard]] int func(); // Will issue a diagnostic, but not an error
  auto v = __has_cpp_attribute(nodiscard); // Should this be 0 or 1?

• #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wc++17-attribute-extensions"
  #if __has_cpp_attribute(nodiscard)
  #  define NODISCARD [[nodiscard]]
  #else
  #  define NODISCARD
  #endif
  #pragma clang diagnostic pop
  
  NODISCARD int func();

They are quite related (other than the first) and it seems to me like we may want to produce different results based on how they're used.

I'm also not sure how one would test things properly if -Werror changes the behaviour of the code.

[ldionne]

Personally, I view -pedantic-errors as something that means more or less -Werror -Wpedantic, and I think it's extremely confusing that warning flags would change the semantics of code, not just the diagnostics you get. When we allow changing semantics of code based on things that look like diagnostics, we can end up with things as surprising as ODR violations based on what the user thinks is a warning flag. That feels extremely surprising and undesirable to me.

I do understand the desire to check whether using an extension is going to produce an error, but I think that desire is misguided in most cases -- what people really want to know is whether the compiler they use supports the extension. In case you're writing a library and you want to allow people to compile with pedantic diagnostics, you should

1. Check for __has_extension to know whether the compiler supports the extension
2. Use pragmas to silence the usage of non-standard extensions to support users turning on these diagnostics

[AaronBallman]

Okay, I'm changing my opinion based on this discussion -- thank you for the extra feedback! I found these points to be particularly compelling:

I'm just not sure how to answer the second without having a million special cases.

#pragma clang diagnostic ignored "-Wc11-extensions"
_Atomic(int) v; // doesn't generate a diagnostic
auto v = __has_extension(c_atomic); // Should this be 0 or 1?

and especially:

and I think it's extremely confusing that warning flags would change the semantics of code, not just the diagnostics you get.

-Wwrite-strings changes the behavior of code in C and it's a terrible diagnostic in terms of usability as a result. __has_extension has similar (but less severe) impacts.

I still think all of the various feature testing macros should behave the same way, but now I'm leaning towards making them all behave more like __has_cpp_attribute instead of more like the current __has_extension behavior. That said, I'm still worried about breaking changes because __has_extension has been around for a long time, is documented to behave this way, and sees relatively heavy use. I think we'd have to find some way to warn users about the change in behavior, give a grace period for them to update their code (perhaps a fix-it or clang-tidy check could help?), and then later change behavior. I don't see that transition going quickly, either.

[ldionne]

I still think all of the various feature testing macros should behave the same way, but now I'm leaning towards making them all behave more like __has_cpp_attribute instead of more like the current __has_extension behavior. That said, I'm still worried about breaking changes because __has_extension has been around for a long time, is documented to behave this way, and sees relatively heavy use. I think we'd have to find some way to warn users about the change in behavior, give a grace period for them to update their code (perhaps a fix-it or clang-tidy check could help?), and then later change behavior. I don't see that transition going quickly, either.

Yeah, I think that's fair. We could (and should) also try it out on large code bases -- I can certainly help doing that over here if someone provides a Clang patch.

I would like to still move forward with this libc++ change independently though, since it is correct and will fix this issue immediately, which is important for anyone using libc++ with -pedantic-errors and availability markup. Does that sound reasonable?

[AaronBallman]

I still think all of the various feature testing macros should behave the same way, but now I'm leaning towards making them all behave more like __has_cpp_attribute instead of more like the current __has_extension behavior. That said, I'm still worried about breaking changes because __has_extension has been around for a long time, is documented to behave this way, and sees relatively heavy use. I think we'd have to find some way to warn users about the change in behavior, give a grace period for them to update their code (perhaps a fix-it or clang-tidy check could help?), and then later change behavior. I don't see that transition going quickly, either.

Yeah, I think that's fair. We could (and should) also try it out on large code bases -- I can certainly help doing that over here if someone provides a Clang patch.

Thank you for the offer! I can't promise I'll have time to do that patch myself in the near future, but I can promise to review it if someone wants to work on it (even if it's just an experiment without a public PR at first).

I would like to still move forward with this libc++ change independently though, since it is correct and will fix this issue immediately, which is important for anyone using libc++ with -pedantic-errors and availability markup. Does that sound reasonable?

That sounds totally reasonable to me!

[ldionne]

I summarized the situation in #84372.

[ldionne]

I will cherry-pick this onto release/18.x since this breaks essentially anyone who uses availability markup and is trying to use -pedantic-errors.

[ldionne]

/cherry-pick 292a28d

[llvmbot]

/pull-request #84374