Basic: Update the rule for whether to use FEATURE or EXTENSION.

[pcc]

It was discovered that EXTENSION has some unexpected implications for
ABI-affecting extensions due to being influenced by warning flags,
so update the rule to allow ABI-affecting extensions to be FEATUREs.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Peter Collingbourne (pcc)

Changes

It was discovered that EXTENSION has some unexpected implications for
ABI-affecting extensions due to being influenced by warning flags,
so update the rule to allow ABI-affecting extensions to be FEATUREs.

---

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

1 Files Affected:

• (modified) clang/include/clang/Basic/Features.def (+4-3)

diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index c58e3f2400adc..94d5432f5fb3d 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -18,9 +18,10 @@
 // extension will be made available.
 //
 // FEATURE(...) should be used to advertise support for standard language
-// features, whereas EXTENSION(...) should be used for clang extensions. Note
-// that many of the identifiers in this file don't follow this rule for backward
-// compatibility reasons.
+// features or ABI-affecting clang extensions, whereas EXTENSION(...) should be
+// used for clang extensions which do not affect ABI. Note that many of the
+// identifiers in this file don't follow this rule for backward compatibility
+// reasons.
 //
 //===----------------------------------------------------------------------===//
 

[pcc]

Other possibilities:

• Drop the check for warning flags, but this could cause some code to start failing to compile.
• Create a new macro for ABI-affecting extensions, and check everything that uses the macro before the check for warning flags.

[AaronBallman]

I'm not super excited about this, but I'd like to understand more about what problem needs solving because this has some significant impacts on user-facing __has_extension and __has_feature checks.

It took years for us to get to the point where extensions were being listed as EXTENSION and features were listed as FEATURE. This muddies the waters even further IMO. For example, bounds safety attributes are ABI impacting, but they're an extension and not a feature. But they're only ABI impacting if you use the parts of the extension which impact the ABI. So how should a PR author or a reviewer know which one to use? The current rule is more defensible IMO.

[pcc]

Here's the problem that I was solving. I wanted to opt-in certain libc++ types to pointer field protection (PFP) by making them non-standard-layout. This was part of #151652 and I later wrote an RFC for my proposal. So I had something like this:

class __force_nonstandard_layout_base1 {};
class __force_nonstandard_layout_base2 : __force_nonstandard_layout_base1 {};
class __force_nonstandard_layout : __force_nonstandard_layout_base1, __force_nonstandard_layout_base2 {};

class foo
#if __has_extension(pointer_field_protection)
: __force_nonstandard_layout
#endif
{
  int *p;
};

The problem was that _has_extension(pointer_field_protection) would evaluate to false in translation units built with -pedantic-errors which resulted in PFP only being enabled in some translation units, leading to a runtime crash.

We're also considering other possibilities for how to opt structs into PFP; it may be possible to avoid relying on __has_feature or __has_extension (e.g. if an attribute is used to opt in, __has_attribute could be used), but we can also imagine other cases where code needs to only do something (such as low level code that manually removes signatures from pointers) if PFP is enabled, and that code would need a reliable is-PFP-enabled macro.

The same problem seems to affect ptrauth_qualifier with the PAuth ABI. For example, consider this TU:

struct S {
  void *
#if __has_extension(ptrauth_qualifier)
      __ptrauth(1, 1, 1)
#endif
          field;
};

clang test.cpp -march=armv8.3a -fptrauth-intrinsics -E -o - preserves the qualifier, but adding -pedantic-errors removes it, which results in an ABI-incompatible definition.

[ojhunt]

@AaronBallman Oh, that's a real problem with the conversion from __has_feature to __has_extension we made for ptrauth -- given the original request it did not occur to anyone involved that this would change the semantics of the flag. Given these semantics we'll need to return to FEATURE/__has_feature for llvm21. This is ABI, it's not valid for error flags to change the ABI.

[ojhunt]

This change seems reasonable to me - the existing silently semantic changes incurred by the extension state need to be made explicitly visible to authors so that it is more clear whether using __has_extension as a test facility is even an option (which it is not for things that have ABI impact).

I'm going to r+ this, but I do want @AaronBallman to approve as well before making this change.

[AaronBallman]

Here's the problem that I was solving. I wanted to opt-in certain libc++ types to pointer field protection (PFP) by making them non-standard-layout. This was part of #151652 and I later wrote an RFC for my proposal. So I had something like this:

class __force_nonstandard_layout_base1 {};
class __force_nonstandard_layout_base2 : __force_nonstandard_layout_base1 {};
class __force_nonstandard_layout : __force_nonstandard_layout_base1, __force_nonstandard_layout_base2 {};

class foo
#if __has_extension(pointer_field_protection)
: __force_nonstandard_layout
#endif
{
  int *p;
};

The problem was that _has_extension(pointer_field_protection) would evaluate to false in translation units built with -pedantic-errors which resulted in PFP only being enabled in some translation units, leading to a runtime crash.

IMO, that's not an issue with __has_extension; you can run into the same situation other ways, it's a bog standard ODR violation. But I missed this RFC entirely so I may be missing some information in my scans of the discussions. PFP has no user-facing code the user has to write, does it? You enable it and you get different backend behavior? If so, that's somewhere in between extension and feature. __has_extension returns true for extensions as a way to test whether you get the functionality of the extension and as one more way in which to silence pedantic warnings (the other way is using __extension__ when you can rely on the extension being supported). If this is only impacting backend behavior, you get no pedantic warnings for using it anyway, you've opted into a different language dialect, which makes it more like a sanitizer, which is traditionally checked via __has_feature.

So maybe we want to clarify the internal and external documentation more along those lines? And then PFP can use __has_feature anyway.

We're also considering other possibilities for how to opt structs into PFP; it may be possible to avoid relying on __has_feature or __has_extension (e.g. if an attribute is used to opt in, __has_attribute could be used), but we can also imagine other cases where code needs to only do something (such as low level code that manually removes signatures from pointers) if PFP is enabled, and that code would need a reliable is-PFP-enabled macro.

The same problem seems to affect ptrauth_qualifier with the PAuth ABI. For example, consider this TU:

That's a similar ODR violation, but they also have a bug; __ptrauth is failing to emit a pedantic diagnostic for use of the extension at all. I think it's a different situation because there the user is explicitly writing code to use the extension.

__has_feature is document publicly to be about support for standard features and __has_extension is documented to be about support for extensions (https://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension). I think we do users a disservice by exposing extensions as features and vice versa, though we certainly have historical cases where we've done that because we only had one feature testing macro (or mistakes where we did it by accident).

[ojhunt]

The same problem seems to affect ptrauth_qualifier with the PAuth ABI. For example, consider this TU:

That's a similar ODR violation, but they also have a bug; __ptrauth is failing to emit a pedantic diagnostic for use of the extension at all. I think it's a different situation because there the user is explicitly writing code to use the extension.

Obviously more details in the other PRs for making the ptrauth_qualifier a feature or disabling -pedantic-errors when pointer auth is enabled: the problem here is that -pedantic-errors changes program semantics such that it results in ODR violations: if a declaration has properties that is determined by a __has_extension guard then -pedantic-errors changes the declaration semantics and creates an ODR violation (and in the case of ptrauth an ABI break).

I would argue that the bigger issue is this behavior from __has_extension: it seems that it presumes that all use of extensions will produce a warning, which gets turned into an error, so therefore anything used inside a __has_extension(...) == true block would always fail - but people can (and do) disable warnings all the time, so that assumption is incorrect. In addition to globally disabling warnings there is also __extension__ which suppresses the warning locally and so also permits the use of an extension under -pedantic-errors.

The follow on problem here is that __has_extension's behavior matches that of gcc, so I'm not sure it would be safe to change it.

[AaronBallman]

After the latest discussions, I think this PR can be closed?

[pcc]

Yes, I've closed it.