Page MenuHomePhabricator

[Clang] Implement P2242R3
Needs ReviewPublic

Authored by cor3ntin on Oct 8 2021, 4:49 AM.

Details

Summary

Allow goto, label statements as well as
static, thread_local and non-literal variables in constexpr
functions.

The proposal is implemented as a language extension in older
language modes.

Diff Detail

Event Timeline

cor3ntin requested review of this revision.Oct 8 2021, 4:49 AM
cor3ntin created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 4:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cor3ntin updated this revision to Diff 378172.Oct 8 2021, 4:52 AM

Fix diagnostics formatting

cor3ntin updated this revision to Diff 378173.Oct 8 2021, 4:54 AM

Add missing test file

I am concerned that the general direction of this patch (to allow the previously ill-formed constexpr functions as an extension under older language levels) is based upon bugs/divergences-from-orthodoxy in Clang's implementation of C++20 (noting that the committee discussion did not agree with Clang's split between the template and non-template cases).

Evaluating "an invocation of an instantiated constexpr function that fails to satisfy the requirements for a constexpr function" causes an expression to not be a core constant expression. The ability to SFINAE on this can be demonstrated using GCC.

For the following case under C++20, GCC finds that the template candidate suffers from substitution failure (Clang doesn't):
https://godbolt.org/z/h71ffYafM

struct NonLiteral {
  NonLiteral();
  operator int();
} *pNL = 0;

template <typename T, T> struct ValueSink;

template <typename T>
struct A {
  constexpr int f() {
    return 42;
    T t;
  }
};

short *f(void *);

template <typename T>
long *f(T *, ValueSink<int, A<T>().f()> * = 0);

using retty = decltype(f(pNL));
typedef short *retty;

I am concerned that the general direction of this patch (to allow the previously ill-formed constexpr functions as an extension under older language levels) is based upon bugs/divergences-from-orthodoxy in Clang's implementation of C++20 (noting that the committee discussion did not agree with Clang's split between the template and non-template cases).

Thanks for bringing this concern up!

Evaluating "an invocation of an instantiated constexpr function that fails to satisfy the requirements for a constexpr function" causes an expression to not be a core constant expression. The ability to SFINAE on this can be demonstrated using GCC.

So your concern is that allowing this patch to be used as an extension in older language modes may change the behavior of existing code? Or is your concern more broad than just this patch?

For the following case under C++20, GCC finds that the template candidate suffers from substitution failure (Clang doesn't):
https://godbolt.org/z/h71ffYafM

struct NonLiteral {
  NonLiteral();
  operator int();
} *pNL = 0;

template <typename T, T> struct ValueSink;

template <typename T>
struct A {
  constexpr int f() {
    return 42;
    T t;
  }
};

short *f(void *);

template <typename T>
long *f(T *, ValueSink<int, A<T>().f()> * = 0);

using retty = decltype(f(pNL));
typedef short *retty;

Thank you for the example code, that helps add clarity. I think it's worth noting that implementations disagree here in a few different ways: https://godbolt.org/z/f9KnMhTGd

So your concern is that allowing this patch to be used as an extension in older language modes may change the behavior of existing code? Or is your concern more broad than just this patch?

The behaviour of Clang before this patch in older language modes is inconsistent with the specification before P2242R3 (for the template case) in such a way that it could appear that P2242R3 is already implemented for the template case (but not following the usual convention for extensions affecting SFINAE); however, the mechanism by which that occurs might be something other than an implementation of P2242R3.

My concern is that the status quo of the Clang implementation with respect to this area is broken, which in turn makes it possible for this patch to exacerbate the issue by building on top of the brokenness and then making a fix more complicated. At the very least, this patch does not demonstrate that the "extension" does not affect SFINAE.

In other words, my concern is that this patch is necessarily incomplete unless if the situation around the template case is resolved.

Thank you for the example code, that helps add clarity. I think it's worth noting that implementations disagree here in a few different ways: https://godbolt.org/z/f9KnMhTGd

ICC fails to SFINAE -- but at least it is consistent with GCC in considering the expression to be non-constant.

So your concern is that allowing this patch to be used as an extension in older language modes may change the behavior of existing code? Or is your concern more broad than just this patch?

The behaviour of Clang before this patch in older language modes is inconsistent with the specification before P2242R3 (for the template case) in such a way that it could appear that P2242R3 is already implemented for the template case (but not following the usual convention for extensions affecting SFINAE); however, the mechanism by which that occurs might be something other than an implementation of P2242R3.

My concern is that the status quo of the Clang implementation with respect to this area is broken, which in turn makes it possible for this patch to exacerbate the issue by building on top of the brokenness and then making a fix more complicated. At the very least, this patch does not demonstrate that the "extension" does not affect SFINAE.

In other words, my concern is that this patch is necessarily incomplete unless if the situation around the template case is resolved.

Thank you for the explanation, that's helpful! I'm sympathetic to not wanting to build on top of an unstable foundation, but I'm also a bit worried that we're asking a lot of @cor3ntin in terms of this PR because it sounds like this is a general request to fix template instantiation before doing more constexpr work (because anything constexpr is generally SFINAE-able). I think we need to fix the template instantiation issues, but I'm also not certain we should gate constexpr work on those fixes.

I'd definitely like to hear thoughts from @rsmith on the right way to proceed.

Thank you for the example code, that helps add clarity. I think it's worth noting that implementations disagree here in a few different ways: https://godbolt.org/z/f9KnMhTGd

ICC fails to SFINAE -- but at least it is consistent with GCC in considering the expression to be non-constant.

Yeah, and MSVC agrees with current Clang. (I hope that doesn't mean we additionally need to think about -fms-compatibility).

A different "ouch" (and, yes, this is a regression from this patch):

auto f = [](bool b) {
  if (b) return 42;
  static int x = 0;
  return x;
};
constexpr int x = f(true);
const int *p = &x;

Generates no diagnostics with this patch even with -std=c++17 -Wall -Wextra -pedantic-errors.

Lambda capture semantics mean that extensions or inconsistencies in constexpr evaluation result in binary-compatibility issues.

struct NonLit {
  NonLit();
};
template <typename T>
constexpr int foo() {
  return 42;
  T t;
}
extern int g(void *);
inline int f(void *p) {
  const int x = foo<NonLit>();
  auto ff = [=] { return x; };
  using ty = decltype(ff);
  if (p) {
    return (*(ty *)p)();
  }
  return g(&ff);
}
int g(void *p) { if (!p) return 0; return f(p); }

Notice that GCC reads from the closure object in C++20: https://godbolt.org/z/vYs63h8vb.

So your concern is that allowing this patch to be used as an extension in older language modes may change the behavior of existing code? Or is your concern more broad than just this patch?

The behaviour of Clang before this patch in older language modes is inconsistent with the specification before P2242R3 (for the template case) in such a way that it could appear that P2242R3 is already implemented for the template case (but not following the usual convention for extensions affecting SFINAE); however, the mechanism by which that occurs might be something other than an implementation of P2242R3.

My concern is that the status quo of the Clang implementation with respect to this area is broken, which in turn makes it possible for this patch to exacerbate the issue by building on top of the brokenness and then making a fix more complicated. At the very least, this patch does not demonstrate that the "extension" does not affect SFINAE.

In other words, my concern is that this patch is necessarily incomplete unless if the situation around the template case is resolved.

Thank you for the explanation, that's helpful! I'm sympathetic to not wanting to build on top of an unstable foundation, but I'm also a bit worried that we're asking a lot of @cor3ntin in terms of this PR because it sounds like this is a general request to fix template instantiation before doing more constexpr work (because anything constexpr is generally SFINAE-able). I think we need to fix the template instantiation issues, but I'm also not certain we should gate constexpr work on those fixes.

I'd definitely like to hear thoughts from @rsmith on the right way to proceed.

Pinging @rsmith -- I'd like to unblock @cor3ntin.

So your concern is that allowing this patch to be used as an extension in older language modes may change the behavior of existing code? Or is your concern more broad than just this patch?

The behaviour of Clang before this patch in older language modes is inconsistent with the specification before P2242R3 (for the template case) in such a way that it could appear that P2242R3 is already implemented for the template case (but not following the usual convention for extensions affecting SFINAE); however, the mechanism by which that occurs might be something other than an implementation of P2242R3.

My concern is that the status quo of the Clang implementation with respect to this area is broken, which in turn makes it possible for this patch to exacerbate the issue by building on top of the brokenness and then making a fix more complicated. At the very least, this patch does not demonstrate that the "extension" does not affect SFINAE.

In other words, my concern is that this patch is necessarily incomplete unless if the situation around the template case is resolved.

Thank you for the explanation, that's helpful! I'm sympathetic to not wanting to build on top of an unstable foundation, but I'm also a bit worried that we're asking a lot of @cor3ntin in terms of this PR because it sounds like this is a general request to fix template instantiation before doing more constexpr work (because anything constexpr is generally SFINAE-able). I think we need to fix the template instantiation issues, but I'm also not certain we should gate constexpr work on those fixes.

I'd definitely like to hear thoughts from @rsmith on the right way to proceed.

Pinging @rsmith -- I'd like to unblock @cor3ntin.

If the issue is regarding the support and extension warning in C++20 and older modes, it's something I can address by conserving the status quo in older versions. I did add them to try to be consistent with guidelines I have received in the past
If the ask is a more involved modification of how clang does SFINAE in general, i don't think that i can take that on.
Personally, I do not think going out of our ways to support something that will be broken once people update to c++23 is critical, as it does involve some fairly convoluted code, but i also understand the desire to be as conforming as possible.

If the issue is regarding the support and extension warning in C++20 and older modes, it's something I can address by conserving the status quo in older versions.

I think preserving the status quo in older modes is fine.

If the ask is a more involved modification of how clang does SFINAE in general, i don't think that i can take that on.

As long as this patch isn't making a change to the older modes, then there's nothing it's doing that requires special attention of the "disable if checking for SFINAE purposes" variety.