Skip to content

Disable constexpr function body checking in more situations #94347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

AaronBallman
Copy link
Collaborator

Before C++23, we would check a constexpr function body to diagnose if the function can never be evaluated in a constant expression context. This was previously required standards behavior, but C++23 relaxed the restrictions with P2448R2. While this checking is useful, it is also quite expensive, especially in pathological cases (see #92924 for an example), because it means the mere presence of a constexpr function definition will require constant evaluation even if the function is not used within the TU.

Clang suppresses diagnostics in system headers by default and system headers (like STL implementations) can be full of constexpr function bodies. Now we suppress the check for a diagnostic if the function definition is in a system header or if the -Winvalid-constexpr diagnostic is disabled. This should have some mild compile time performance improvements.

Also, the previous implementation would disable the diagnostic in C++23 mode entirely. Due to the benefit of the check, this patch now makes it possible to enable the diagnostic explicitly in C++23 mode.

While this is useful functionality, it means the mere *presence* of a
constexpr function will impact compile time overhead.

This is currently an experiment to see whether removing this causes a
noticable difference in compile time overhead.
This leaves the code in place but disables it for system headers and
when the diagnostic itself is disabled. It also corrects the logic for
C++23 so that we don't check for potential evaluation in C++23 mode or
later.
@AaronBallman AaronBallman added clang Clang issues not falling into any other category clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer c++23 labels Jun 4, 2024
@AaronBallman AaronBallman requested a review from damyanp June 4, 2024 12:48
@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jun 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 4, 2024

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

Before C++23, we would check a constexpr function body to diagnose if the function can never be evaluated in a constant expression context. This was previously required standards behavior, but C++23 relaxed the restrictions with P2448R2. While this checking is useful, it is also quite expensive, especially in pathological cases (see #92924 for an example), because it means the mere presence of a constexpr function definition will require constant evaluation even if the function is not used within the TU.

Clang suppresses diagnostics in system headers by default and system headers (like STL implementations) can be full of constexpr function bodies. Now we suppress the check for a diagnostic if the function definition is in a system header or if the -Winvalid-constexpr diagnostic is disabled. This should have some mild compile time performance improvements.

Also, the previous implementation would disable the diagnostic in C++23 mode entirely. Due to the benefit of the check, this patch now makes it possible to enable the diagnostic explicitly in C++23 mode.


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

6 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+14)
  • (modified) clang/include/clang/Basic/LangOptions.def (+3)
  • (modified) clang/include/clang/Driver/Options.td (+4)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+6)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+13-4)
  • (added) clang/test/SemaCXX/constexpr-never-constant.cpp (+26)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 32515fbac64f6..6f599bbe11a2f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -207,6 +207,9 @@ C++23 Feature Support
 - Implemented `P1774R8: Portable assumptions <https://wg21.link/P1774R8>`_.
 
 - Implemented `P2448R2: Relaxing some constexpr restrictions <https://wg21.link/P2448R2>`_.
+  Note, the ``-Winvalid-constexpr`` diagnostic is now disabled in C++23 mode,
+  but can be explicitly specified to retain the old diagnostic checking
+  behavior.
 
 - Added a ``__reference_converts_from_temporary`` builtin, completing the necessary compiler support for
   `P2255R2: Type trait to determine if a reference binds to a temporary <https://wg21.link/P2255R2>`_.
@@ -323,6 +326,17 @@ Non-comprehensive list of changes in this release
 - Builtins ``__builtin_shufflevector()`` and ``__builtin_convertvector()`` may
   now be used within constant expressions.
 
+- When compiling a constexpr function, Clang will check to see whether the
+  function can *never* be used in a constant expression context and issues a
+  diagnostic under the ``-Winvalid-constexpr`` diagostic flag (which defaults
+  to an error). This check can be expensive because the mere presence of a
+  function marked ``constexpr`` will cause us to undergo constant expression
+  evaluation, even if the function is not called within the translation unit
+  being compiled. Due to the expense, Clang no longer checks constexpr function
+  bodies when the function is defined in a system header file or when
+  ``-Winvalid-constexpr`` is not enabled for the function definition, which
+  should result in mild compile-time performance improvements.
+
 New Compiler Flags
 ------------------
 - ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 4061451b2150a..cd0c430850159 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -505,6 +505,9 @@ COMPATIBLE_LANGOPT(IncrementalExtensions, 1, 0, " True if we want to process sta
 
 BENIGN_LANGOPT(CheckNew, 1, 0, "Do not assume C++ operator new may not return NULL")
 
+BENIGN_LANGOPT(CheckConstexprFunctionBodies, 1, 1,
+               "True if we want to emit a diagnostics for a constexpr function "
+               "body if it can never be used in a constant expression.")
 #undef LANGOPT
 #undef COMPATIBLE_LANGOPT
 #undef BENIGN_LANGOPT
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 57f37c5023110..904547a0a13de 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -961,6 +961,10 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, Group<W_Group>,
   HelpText<"Enable warnings for deprecated constructs and define __DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group<W_Group>,
   Visibility<[ClangOption, CC1Option]>;
+def Winvalid_constexpr : Flag<["-"], "Winvalid-constexpr">, Group<W_Group>,
+  Visibility<[ClangOption, CC1Option]>;
+def Wno_invalid_constexpr : Flag<["-"], "Wno-invalid-constexpr">,
+  Group<W_Group>, Visibility<[ClangOption, CC1Option]>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, FlangOption]>,
   Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in <arg> to the linker">,
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 14ee02c4cd582..a5ced4791a8c7 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -593,6 +593,12 @@ static bool FixupInvocation(CompilerInvocation &Invocation,
   CodeGenOpts.CodeModel = TargetOpts.CodeModel;
   CodeGenOpts.LargeDataThreshold = TargetOpts.LargeDataThreshold;
 
+  // -Winvalid-constexpr is enabled by default. We want to disable it in C++23
+  // mode, but only if `-Winvalid-constexpr` is not specified on the command
+  // line.
+  LangOpts.CheckConstexprFunctionBodies = Args.hasFlagNoClaim(
+      OPT_Winvalid_constexpr, OPT_Wno_invalid_constexpr, !LangOpts.CPlusPlus23);
+
   if (LangOpts.getExceptionHandling() !=
           LangOptions::ExceptionHandlingKind::None &&
       T.isWindowsMSVCEnvironment())
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 631fd4e354927..ef34359347a95 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -2469,11 +2469,20 @@ static bool CheckConstexprFunctionBody(Sema &SemaRef, const FunctionDecl *Dcl,
   //     base class sub-objects shall be a constexpr constructor.
   //
   // Note that this rule is distinct from the "requirements for a constexpr
-  // function", so is not checked in CheckValid mode.
+  // function", so is not checked in CheckValid mode. Because the check for
+  // constexpr potential is expensive, skip the check if the diagnostic is
+  // disabled, the function is declared in a system header, or we're in C++23
+  // or later mode (see https://wg21.link/P2448).
+  auto SkipCheck = [&SemaRef, Dcl] {
+    return !SemaRef.getLangOpts().CheckConstexprFunctionBodies ||
+           SemaRef.getSourceManager().isInSystemHeader(Dcl->getLocation()) ||
+           SemaRef.getDiagnostics().isIgnored(
+               diag::ext_constexpr_function_never_constant_expr,
+               Dcl->getLocation());
+  };
   SmallVector<PartialDiagnosticAt, 8> Diags;
-  if (Kind == Sema::CheckConstexprKind::Diagnose &&
-      !Expr::isPotentialConstantExpr(Dcl, Diags) &&
-      !SemaRef.getLangOpts().CPlusPlus23) {
+  if (Kind == Sema::CheckConstexprKind::Diagnose && !SkipCheck() &&
+      !Expr::isPotentialConstantExpr(Dcl, Diags)) {
     SemaRef.Diag(Dcl->getLocation(),
                  diag::ext_constexpr_function_never_constant_expr)
         << isa<CXXConstructorDecl>(Dcl) << Dcl->isConsteval()
diff --git a/clang/test/SemaCXX/constexpr-never-constant.cpp b/clang/test/SemaCXX/constexpr-never-constant.cpp
new file mode 100644
index 0000000000000..307810ee263dd
--- /dev/null
+++ b/clang/test/SemaCXX/constexpr-never-constant.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -Winvalid-constexpr -verify -fcxx-exceptions %s
+// Note: for a diagnostic that defaults to an error, -Wno-foo -Wfoo will
+// disable the diagnostic and then re-enable it *as a warning* rather than as
+// an error. So we manually enable it as an error again with -Werror to keep
+// the diagnostic checks consistent.
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -Wno-invalid-constexpr -Winvalid-constexpr -Werror=invalid-constexpr -verify -fcxx-exceptions %s
+
+// RUN: %clang_cc1 -fsyntax-only -Wno-invalid-constexpr -verify=good -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -verify=good -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -Wno-invalid-constexpr -verify=good -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -Winvalid-constexpr -Wno-invalid-constexpr -verify=good -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-invalid-constexpr -verify=good -fcxx-exceptions %s
+// good-no-diagnostics
+
+constexpr void func() { // expected-error {{constexpr function never produces a constant expression}}
+  throw 12;             // expected-note {{subexpression not valid in a constant expression}}
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Winvalid-constexpr"
+constexpr void other_func() {
+#pragma clang diagnostic pop
+
+  throw 12;
+}

@AaronBallman AaronBallman requested review from cor3ntin, erichkeane, Endilll, Fznamznon and Sirraide and removed request for damyanp June 4, 2024 12:48
* lambda changed to a simple boolean
* reworked the way the option is exposed
@AaronBallman AaronBallman merged commit e5f7123 into llvm:main Jun 4, 2024
8 checks passed
@AaronBallman AaronBallman deleted the perf/no-constexpr-fn-body-checking branch June 4, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++23 clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants