[libc++][CI] increases constexpr evaluation limit.

[Mordante]

Summary

This was discovered as an issue in D131317.

Depends on D131835

[var-const]

libcxx/utils/libcxx/test/features.py

48

Ultranit: s/empirical/empirically/.

48

I think it would be helpful to write the default value for comparison (I think it's 1048576 for Clang -- see clang/include/clang/Basic/LangOptions.def).

50

This flag is not supported by GCC. Does this configuration know not to pass the flag to GCC (and same for the GCC flag)?

[var-const]

Thanks for working on this, by the way -- I've seen a few cases where we had to change the code to work around this largely arbitrary limit, and really wished we didn't have to do that.

[philnik]

libcxx/utils/libcxx/test/features.py

47–56

Is there a reason we don't want this in params.py? I can't really see a case where we want to restrict tests based on clang-constexpr-steps or gcc-constexpr-steps.

51

Are we sure we want to bring this right to the maximum? This could increase the built time by 100x when you actually have bad code.

[Mordante]

Thanks for the reviews!

libcxx/utils/libcxx/test/features.py

47–56

I'm not entirely sure what you mean. params.py is the file with the user configurable settings. I don't want the user to change this setting. What am I missing?

48

I don' think that's really helpful, Clang may change this at some point and then the comment is out of date. When really wanted to have the number I'd rather link to the source of truth.

50

Yes that's done by the hasCompileFlag part, when not supported it isn't added.

51

I can remove one 0 and it still works, but I like to have a bit of a margin. Lowering it further fails in the test.

[Mordante]

Address review comment.

[philnik]

libcxx/utils/libcxx/test/features.py

51

I'd be a lot more comfortable with "just" a 10x increase. We can still bump it higher if we really need to later. By that time we might want to consider adding some easy way to disable this for specific tests to not have the compiler working for ages just to let us know we have a bug in our code. The ranges.transform.pass.cpp test is already horrible to work in. I can't imagine how it would be if we needed this step limit.

[var-const]

libcxx/utils/libcxx/test/features.py

48

I don't feel very strongly, but I'd still add it. I think it's obvious to the reader that this number may be out of date, but it could be made explicit by adding a note like "at the time of writing". What I find useful here isn't so much the exact number as having a point of comparison -- i.e., are we increasing the default limit two times, ten times, a hundred times, etc.?

51

I think ideally we'd keep the default limit and then increase it as an opt-in for certain tests (that way, the increased number can be finer-grained to the needs of each test file). But currently I think there is no way to pass flags to e.g. only Clang.

[var-const]

LGTM, but I'll leave the final approval to @ldionne.

[huixie90]

I like this change. I have several range algorithm tests which I have to separate some tests into multiple test functions because of the limit. Thanks for doing that.

[ldionne]

I hate to rain on the parade, but I don't think this patch is desirable as-is. Indeed, we want to make sure that we compile our test suite with flags as similar as possible to what the users use. Otherwise, you end up not testing what matters, i.e. what users do.

For example, if we add -fconstexpr-steps=10000000 to the test suite, we could easily introduce a constexpr algorithm that always busts the default Clang limit even for small inputs, and our test suite would never catch it. Users would try to use the algorithm with a trivial example and things wouldn't work. That would be rather embarrassing.

So any approach where we turn this on for the whole test suite is a no-go IMO. Instead, we should increase the limit only for tests that actually require it, to reduce our exposure to the above bugs. One way to do it would be:

// In a test that requires more constexpr steps:
// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=100000
// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=100000

Then, in features.py:

Feature(name='has-fconstexpr-steps', when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1')),
Feature(name='has-fconstexpr-ops-limit', when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-ops-limit=1')),

I just need to implement support for this new fancy form of ADDITIONAL_COMPILE_FLAGS. I'll look into it immediately, this would be useful on its own even regardless of this review.

[ldionne]

See https://reviews.llvm.org/D132575 for the ADDITIONAL_COMPILE_FLAGS thing. I think this should work.

[Mordante]

Rebased and use the new approach suggested by @ldionne.

[Mordante]

In D131836#3746211, @ldionne wrote:

I hate to rain on the parade, but I don't think this patch is desirable as-is. Indeed, we want to make sure that we compile our test suite with flags as similar as possible to what the users use. Otherwise, you end up not testing what matters, i.e. what users do.
...
I just need to implement support for this new fancy form of ADDITIONAL_COMPILE_FLAGS. I'll look into it immediately, this would be useful on its own even regardless of this review.

As mention during our live review I disagree with the "rain on the parade". IMO this is how code review should work. With your suggestions and the new feature you added I think we have a better solution to solve this problem. Thanks!

libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp

14

This isn't needed here (yet) but will be in the future. Add it here new to test whether the changes pass the CI.

libcxx/utils/libcxx/test/features.py

48

With the suggested approach of @ldionne the hard-coded value is removed here.

51

With the suggested approach of @ldionne the limit will be per test and no longer a global value.

[philnik]

The new solution seems to me to be a lot better! This is actually a great solution, thank you both @Mordante and @ldionne for working on this!

libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 // UNSUPPORTED: !stdlib=libc++ && c++11
 // UNSUPPORTED: !stdlib=libc++ && c++14
 
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=12712420
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=12712420
+
 // <charconv>
 
 // to_chars_result to_chars(char* first, char* last, Integral value,
 //                          int base = 10)
 
 #include <charconv>
 #include "test_macros.h"
 #include "charconv_test_helpers.h"

libcxx/utils/libcxx/test/features.py

DEFAULT_FEATURES = [
   Feature(name='thread-safety',
           when=lambda cfg: hasCompileFlag(cfg, '-Werror=thread-safety'),
           actions=[AddCompileFlag('-Werror=thread-safety')]),
 
   Feature(name='diagnose-if-support',
           when=lambda cfg: hasCompileFlag(cfg, '-Wuser-defined-warnings'),
           actions=[AddCompileFlag('-Wuser-defined-warnings')]),
 
+  # Tests to validate whether the compiler has a way to set the maximum number
+  # of steps during constant evaluation. Since the flag differs per compiler
+  # store the "valid" flag as a feature. This allows passing the proper compile
+  # flag to the compiler:
+  # // ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=12345678
+  # // ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=12345678
+  Feature(name='has-fconstexpr-steps',
+          when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1')),
+
+  Feature(name='has-fconstexpr-ops-limit',
+          when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-ops-limit=1')),
+
   Feature(name='has-fblocks',                   when=lambda cfg: hasCompileFlag(cfg, '-fblocks')),
   Feature(name='-fsized-deallocation',          when=lambda cfg: hasCompileFlag(cfg, '-fsized-deallocation')),
   Feature(name='-faligned-allocation',          when=lambda cfg: hasCompileFlag(cfg, '-faligned-allocation')),
   Feature(name='fdelayed-template-parsing',     when=lambda cfg: hasCompileFlag(cfg, '-fdelayed-template-parsing')),
   Feature(name='libcpp-no-coroutines',          when=lambda cfg: featureTestMacros(cfg).get('__cpp_impl_coroutine', 0) < 201902),
   Feature(name='has-fobjc-arc',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc') and
                                                                  sys.platform.lower().strip() == 'darwin'), # TODO: this doesn't handle cross-compiling to Apple platforms.
   Feature(name='objective-c++',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc')),