[clang] increase default constexpr step limit #143785
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (Tsche) ChangesCurrently clang limits the number of constant evaluation steps to 1'048'576 (or 2^20) by default. This default comes from [[implimits]/1.39](https://eel.is/c++draft/implimits#1.39). This limit is easily reached - for example in libc++ tests we override this default in many places:
</details> In libc++ tests that override both
</details> So, assuming With heavy metaprogramming features like reflection still on track for C++26, I believe it is reasonable to increase this limit. For instance, recursively walking all namespaces and performing a set of checks for every reflected member of that namespace immediately hits this limit for the global namespace - which roughly yields 3000 member reflections even without descending. This patch increases the default limit to 20'000'000. With this default all but the last 3 tests in the first table would not have to override the default. I think this is a more reasonable default - on my machine 20 million steps take roughly 10 seconds. Full diff: https://github.com/llvm/llvm-project/pull/143785.diff 3 Files Affected:
diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index 62844f7e6a2fa..c948ba218c219 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -3992,7 +3992,7 @@ Controlling implementation limits
Sets the limit for the number of full-expressions evaluated in a single
constant expression evaluation. This also controls the maximum size
of array and dynamic array allocation that can be constant evaluated.
- The default is 1048576.
+ The default is 20000000.
.. option:: -ftemplate-depth=N
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 789761c1f3647..ba3c8dac8b35d 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -415,7 +415,7 @@ BENIGN_LANGOPT(InstantiationDepth, 32, 1024,
"maximum template instantiation depth")
BENIGN_LANGOPT(ConstexprCallDepth, 32, 512,
"maximum constexpr call depth")
-BENIGN_LANGOPT(ConstexprStepLimit, 32, 1048576,
+BENIGN_LANGOPT(ConstexprStepLimit, 32, 20000000,
"maximum constexpr evaluation steps")
BENIGN_LANGOPT(EnableNewConstInterp, 1, 0,
"enable the experimental new constant interpreter")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 152df89118a6a..4886e7b0a45e0 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2002,7 +2002,7 @@ def fconstexpr_depth_EQ : Joined<["-"], "fconstexpr-depth=">, Group<f_Group>,
def fconstexpr_steps_EQ : Joined<["-"], "fconstexpr-steps=">, Group<f_Group>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Set the maximum number of steps in constexpr function evaluation">,
- MarshallingInfoInt<LangOpts<"ConstexprStepLimit">, "1048576">;
+ MarshallingInfoInt<LangOpts<"ConstexprStepLimit">, "20000000">;
def fexperimental_new_constant_interpreter : Flag<["-"], "fexperimental-new-constant-interpreter">, Group<f_Group>,
HelpText<"Enable the experimental new constant interpreter">,
Visibility<[ClangOption, CC1Option]>,
|
Sirraide
left a comment
Sirraide
left a comment
There was a problem hiding this comment.
This seems reasonable to me.
This should also come with a release note (in clang/docs/ReleaseNotes.rst); not sure in what section though.
| constant expression evaluation. This also controls the maximum size | ||
| of array and dynamic array allocation that can be constant evaluated. | ||
| The default is 1048576. | ||
| The default is 20000000. |
There was a problem hiding this comment.
| The default is 20000000. | |
| The default is 20 000 000. |
Maybe format it like this because this amount of zeroes is giving me a headache.
There was a problem hiding this comment.
How do you feel about using the C++ digit separator ' to instead spell it as 20'000'000?
There was a problem hiding this comment.
Er, not sure that’s going to work too well in RST (but I don’t actually know; I don’t know much about RST candidly...), and typographically it would be rather unusual since this isn’t really code.
Actually, does RST have a non-breaking space? If so I’d recommend using that.
Sirraide
requested review from
AaronBallman,
cor3ntin,
erichkeane and
philnik777
Yeah, same. Added some more reviewers in case anyone can think of a reason why we shouldn’t do this. |
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Typically we set limits like this for at least somewhat of a reasoned-reason. Can we do some archeology to figure out when/why we decided on this limit? Many times these limits are a result of our own stack space limits, so it would be interesting to see what workload this was based on, and whether it still exists.
shafik
left a comment
shafik
left a comment
There was a problem hiding this comment.
I would also like to see some more background info on how we came up with the original limit (if it exists).
|
The linked standard says:
Is that actually what the "steps" in clang refer to? We seem to check this for all statements: llvm-project/clang/lib/AST/ExprConstant.cpp Lines 5466 to 5467 in 9963853 And we check the limit for array sizes: llvm-project/clang/lib/AST/ExprConstant.cpp Lines 1077 to 1088 in 9963853 ... which is probably irrelevant for the discussion, but better to clarify. |
I’m not sure it does, but my guess is that that’s where that number came from. |
|
Personally, I am not in favor of increasing the step limit without more compelling evidence for the need. The purpose to having the step count in the first place is that reaching that limit is FAR more likely due to the user making a mistake and hitting infinite loops or quadratic loops they didn't intend on hitting than not. So having the step count somewhere reasonable means we break out of the compiler's process quicker so the user can repair their code and rerun the compiler. So I don't think we want to set the default limit based on "it's possible to hit the limit, look at this example which does" but instead base it on the (high end of) average workload. I can see a point to allowing for (Btw, also worth keeping in mind that the implementation needs to test limits. So if we increase the limits in Clang, our constexpr stress tests will get slower for every precommit and postcommit CI pipeline as will any stress tests in libc++. So picking an arbitrarily high limit will have some negative impacts on our infrastructure as well. That's not a reason to not raise the limit, but it is something we need to keep in mind when picking new limits.) |
To address @AaronBallman's feedback from #143785 this patch implements an explicit opt-out for `-fconstexpr-steps` by setting `-fconstexpr-steps=0`. This does not change any defaults, but gives users an easy way to opt out of this limit altogether (and instead let the compiler reach the system's resource limits). Currently users set `constexpr-steps` to some arbitrary high number (and I mean _arbitrary_ - see the tables in the previous PR). This isn't actually opting out of the limit though - you're still bound by the upper bound of the counter's type. If you have enough resources to evaluate more than 18446744073709551615 steps that's bad news. In any case, `=0` conveys the intent clearer. This is in line with how we handle other flags, ie `-ftemplate-backtrace-limit` or `-ferror-limit`.
To address @AaronBallman's feedback from llvm/llvm-project#143785 this patch implements an explicit opt-out for `-fconstexpr-steps` by setting `-fconstexpr-steps=0`. This does not change any defaults, but gives users an easy way to opt out of this limit altogether (and instead let the compiler reach the system's resource limits). Currently users set `constexpr-steps` to some arbitrary high number (and I mean _arbitrary_ - see the tables in the previous PR). This isn't actually opting out of the limit though - you're still bound by the upper bound of the counter's type. If you have enough resources to evaluate more than 18446744073709551615 steps that's bad news. In any case, `=0` conveys the intent clearer. This is in line with how we handle other flags, ie `-ftemplate-backtrace-limit` or `-ferror-limit`.
To address @AaronBallman's feedback from llvm#143785 this patch implements an explicit opt-out for `-fconstexpr-steps` by setting `-fconstexpr-steps=0`. This does not change any defaults, but gives users an easy way to opt out of this limit altogether (and instead let the compiler reach the system's resource limits). Currently users set `constexpr-steps` to some arbitrary high number (and I mean _arbitrary_ - see the tables in the previous PR). This isn't actually opting out of the limit though - you're still bound by the upper bound of the counter's type. If you have enough resources to evaluate more than 18446744073709551615 steps that's bad news. In any case, `=0` conveys the intent clearer. This is in line with how we handle other flags, ie `-ftemplate-backtrace-limit` or `-ferror-limit`.
7 participants
Currently clang limits the number of constant evaluation steps to 1'048'576 (or 2^20) by default. This default comes from [implimits]/1.39.
This limit is easily reached - for example in libc++ tests we override this default in many places:
overrides in libc++ tests
In libc++ tests that override both
-fconstexpr-stepsand GCC's-fconstexpr-ops-limit, we see a factor of 0.25 applied in most cases:overrides in libc++ tests
-fconstexpr-steps-fconstexpr-ops-limitSo, assuming
0.25as a conversion factor, this would require increasing the limit to at least 8'388'608 to match gcc's default of 33'554'432.With heavy metaprogramming features like reflection still on track for C++26, I believe it is reasonable to increase this limit. For instance, recursively walking all namespaces and performing a set of checks for every reflected member of that namespace immediately hits this limit for the global namespace - which roughly yields 3000 member reflections even without descending.
This patch increases the default limit to 20'000'000. With this default all but the last 3 tests in the first table would not have to override the default.
I think this is a more reasonable default - on my machine 20 million steps take roughly 10 seconds.