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

ldionne · 2024-03-05T20:18:32Z

__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

…xtension __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 · 2024-03-05T20:19:06Z

@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 · 2024-03-05T20:24:57Z

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 · 2024-03-05T20:33:15Z

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 · 2024-03-05T21:28:55Z

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 · 2024-03-06T15:52:14Z

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 · 2024-03-06T16:14:53Z

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 · 2024-03-06T16:35:18Z

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 · 2024-03-06T16:46:04Z

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 · 2024-03-06T16:50:04Z

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 · 2024-03-06T17:10:49Z

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 · 2024-03-06T17:26:33Z

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 · 2024-03-06T18:46:16Z

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 · 2024-03-06T23:18:09Z

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

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

AaronBallman · 2024-03-07T16:32:21Z

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 · 2024-03-07T18:35:02Z

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 · 2024-03-07T19:44:24Z

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 · 2024-03-07T20:11:54Z

I summarized the situation in #84372.

ldionne · 2024-03-07T20:13:07Z

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 · 2024-03-07T20:13:30Z

/cherry-pick 292a28d

…xtension (llvm#84065) __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 (cherry picked from commit 292a28d)

llvmbot · 2024-03-07T20:18:33Z

/pull-request #84374

…xtension (llvm#84065) __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 (cherry picked from commit 292a28d)

ldionne requested a review from a team as a code owner March 5, 2024 20:18

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 5, 2024

ldionne mentioned this pull request Mar 7, 2024

[clang] __has_extension changes semantics based on -pedantic-errors #84372

Open

ldionne merged commit 292a28d into llvm:main Mar 7, 2024
53 checks passed

ldionne deleted the review/availability-has_extension branch March 7, 2024 20:12

ldionne added this to the LLVM 18.X Release milestone Mar 7, 2024

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ldionne commented Mar 5, 2024

llvmbot commented Mar 5, 2024

philnik777 commented Mar 5, 2024

ldionne commented Mar 5, 2024

philnik777 commented Mar 5, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

ldionne commented Mar 6, 2024

AaronBallman commented Mar 7, 2024

ldionne commented Mar 7, 2024

AaronBallman commented Mar 7, 2024

ldionne commented Mar 7, 2024

ldionne commented Mar 7, 2024

ldionne commented Mar 7, 2024

llvmbot commented Mar 7, 2024

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

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

Conversation

ldionne commented Mar 5, 2024

llvmbot commented Mar 5, 2024

philnik777 commented Mar 5, 2024

ldionne commented Mar 5, 2024

philnik777 commented Mar 5, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

AaronBallman commented Mar 6, 2024

philnik777 commented Mar 6, 2024

ldionne commented Mar 6, 2024

AaronBallman commented Mar 7, 2024

ldionne commented Mar 7, 2024

AaronBallman commented Mar 7, 2024

ldionne commented Mar 7, 2024

ldionne commented Mar 7, 2024

ldionne commented Mar 7, 2024

llvmbot commented Mar 7, 2024