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

[ldionne]

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, __has_extension(...) always returns 0 when -pedantic-errors is used. This means that the semantics of programs can actually depend on whether -pedantic-errors is passed, which seems really confusing because few people would expect compiler diagnostics to impact semantics. For example, it is possible to end up in a situation where ODR is violated (via a #if __has_extension(...) check) based solely on whether -pedantic-errors is passed, which illustrates very well why this is undesirable.

This led to e.g. the issue fixed in #84065, and more discussion is available on that review.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Louis Dionne (ldionne)

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, __has_extension(...) always returns 0 when -pedantic-errors is used. This means that the semantics of programs can actually depend on whether -pedantic-errors is passed, which seems really confusing because few people would expect compiler diagnostics to impact semantics. For example, it is possible to end up in a situation where ODR is violated (via a #if __has_extension(...) check) based solely on whether -pedantic-errors is passed, which illustrates very well why this is undesirable.

This led to e.g. the issue fixed in #84065, and more discussion is available on that review.

[AaronBallman]

I agree in principle with the idea that __has_extension should not be tied to -pedantic-errors because we should not be allowing warning flags (which this one morally is, e.g., -Wpedantic -Werror) to impact program semantics. However, there is evidence that changing this will break code in practice: https://sourcegraph.com/github.com/ziglang/zig@1e67f5021159ed1d9888cf2d0b9f04ef73222f7d/-/blob/lib/include/sgxintrin.h?L17 and this would be changing the documented behavior of a feature testing macro that's been around for a long time. So we cannot proceed without some amount of protection for existing uses. 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.

[AaronBallman]

Note, GCC 14.x has added support for this feature and follows Clang's current behavior: gcc-mirror/gcc@b73fa20 so we definitely need to coordinate with GCC folks (and sooner rather than later, in case they want to change behavior before their release).

CC @fweimer-rh @pinskia

[AaronBallman]

The current behavior has surprising security implications for us: https://github.com/llvm/llvm-project/blob/f07157eb2e8212068cb9469d77f9bc3779ef2ea4/clang/lib/Headers/sgxintrin.h#L17C4-L17C29

Passing -pedantic-errors will silently disable SGX (https://en.wikipedia.org/wiki/Software_Guard_Extensions).

CC @andykaylor

[ldionne]

The current behavior has surprising security implications for us: https://github.com/llvm/llvm-project/blob/f07157eb2e8212068cb9469d77f9bc3779ef2ea4/clang/lib/Headers/sgxintrin.h#L17C4-L17C29

Passing -pedantic-errors will silently disable SGX (https://en.wikipedia.org/wiki/Software_Guard_Extensions).

😱

That's a good example of what I meant by "it's really surprising that what is morally a diagnostic would change the semantics of code".

[pinskia]

Maybe @alexcoplan and @iains can comment on the GCC side of things. Alex implemented it in the end; Iain is one of the few who has been working on the darwin part.

[fweimer-rh]

Sorry, I don't see why this is a bug, conceptually speaking. Surely -pedantic-errors removes some extensions, which means that __has_extension should not report them because using them would lead to a compilation failure?

In some cases it may be possible to use __extension__ to disable the error:

void f(void);
void g(void)
{
  __extension__ ({ return f(); });
}

So maybe we need a way to query for the availability of extensions under __extension__, too?

[philnik777]

@fweimer-rh In #84065 we've had a discussion about this. #84065 (comment) is why I think this is basically impossible to implement correctly.

[zygoloid]

This is essentially the difference between -pedantic-errors and -Werror=pedantic -- the former disables extensions, whereas the latter is just a warning flag.

[ldionne]

Should we deprecate that flag then? It seems like a huge footgun.

[AaronBallman]

-pedantic-errors is used pretty extensively in the wild: https://sourcegraph.com/search?q=context:global+-pedantic-errors+-file:.*clang.*+-file:.*gcc.*+-file:.*%5C.md+-file:.*%5C.c.*+count:10000&patternType=keyword&sm=0 so I'd be wary of deprecating it because of how disruptive it would be. That said, the name is terrible IMO and I'm not strongly opposed to pushing people to a differently-named option that behaves the same way, like -fstrict-conformance, which makes it clear that this is not a warning option but is actually a conformance-related flag that can change the meaning of code. But I think we'll end up having to support -pedantic-errors for a decade+ after it's deprecated, so it might not be worth it.

I don't think it's a good user experience for -pedantic-errors to be different from -Werror=pedantic, or for -pedantic to be different from -Wpedantic, and we have evidence that this is actually causing security-related bugs when we throw __has_extension into the mix. I think it's important that we strive to ensure that warning flags don't change program semantics but a corollary to that is that it should be crystal clear to users what is and isn't a warning flag. I think -pedantic and -pedantic-errors are usually thought of as warning flags that are spelled funny because they're old, not as somewhat different language modes.

I wonder if it's worth exploring an idea where we issue a diagnostic if -pedantic-errors is used in conjunction with #if __has_extension or #elif __has_extension when there's no #else clause for a fallback behavior?

Also, does anyone else find it a bit odd that -pedantic-errors, a mode which is used to say "extensions are disabled" actually allows use of __has_extension at all (without additional diagnostic), given that it's a non-portable extension?