Page MenuHomePhabricator

[libc++] [test] Add support for `ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED` directive.
Needs ReviewPublic

Authored by curdeius on Jul 26 2021, 8:17 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Another spin-off of D79555. This directive will help keeping tests simple and adding -fconstexpr-steps only for clang-based compilers that need it.

Diff Detail

Unit TestsFailed

TimeTest
190 mslibcxx CI GCC 11 / C++latest > libc++.libcxx/selftest/dsl::dsl.sh.py
Script: -- : 'RUN: at line 13'; /usr/bin/python3.6 /home/libcxx-builder/.buildkite-agent/builds/5a05a36815ae-1/llvm-project/libcxx-ci/libcxx/test/libcxx/selftest/dsl/dsl.sh.py x/home/libcxx-builder/.buildkite-agent/builds/5a05a36815ae-1/llvm-project/libcxx-ci/libcxx/test/libcxx/selftest/dsl x/home/libcxx-builder/.buildkite-agent/builds/5a05a36815ae-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/test/libcxx/selftest/dsl/Output/dsl.sh.py.dir xIi91c3IvYmluL3B5dGhvbjMuNiIgL2hvbWUvbGliY3h4LWJ1aWxkZXIvLmJ1aWxka2l0ZS1hZ2VudC9idWlsZHMvNWEwNWEzNjgxNWFlLTEvbGx2bS1wcm9qZWN0L2xpYmN4eC1jaS9saWJjeHgvdGVzdC8uLi91dGlscy9ydW4ucHkgLS1leGVjZGlyICVUIC0tY29kZXNpZ25faWRlbnRpdHkgIiIgLS1lbnYgIC0tIA== xL3Vzci9iaW4vZysrLTEx xLXY= xLWluY2x1ZGUgL2hvbWUvbGliY3h4LWJ1aWxkZXIvLmJ1aWxka2l0ZS1hZ2VudC9idWlsZHMvNWEwNWEzNjgxNWFlLTEvbGx2bS1wcm9qZWN0L2xpYmN4eC1jaS9saWJjeHgvdGVzdC9zdXBwb3J0L25hc3R5X21hY3Jvcy5oIC1ub3N0ZGluYysrIC1JL2hvbWUvbGliY3h4LWJ1aWxkZXIvLmJ1aWxka2l0ZS1hZ2VudC9idWlsZHMvNWEwNWEzNjgxNWFlLTEvbGx2bS1wcm9qZWN0L2xpYmN4eC1jaS9idWlsZC9nZW5lcmljLWdjYy9pbmNsdWRlL2MrKy92MSAtSS9ob21lL2xpYmN4eC1idWlsZGVyLy5idWlsZGtpdGUtYWdlbnQvYnVpbGRzLzVhMDVhMzY4MTVhZS0xL2xsdm0tcHJvamVjdC9saWJjeHgtY2kvYnVpbGQvZ2VuZXJpYy1nY2MvcHJvamVjdHMvbGliY3h4L2luY2x1ZGUvYysrYnVpbGQgLURfX1NURENfRk9STUFUX01BQ1JPUyAtRF9fU1REQ19MSU1JVF9NQUNST1MgLURfX1NURENfQ09OU1RBTlRfTUFDUk9TIC1JL2hvbWUvbGliY3h4LWJ1aWxkZXIvLmJ1aWxka2l0ZS1hZ2VudC9idWlsZHMvNWEwNWEzNjgxNWFlLTEvbGx2bS1wcm9qZWN0L2xpYmN4eC1jaS9saWJjeHgvdGVzdC9zdXBwb3J0IC1zdGQ9YysrMmIgLVdlcnJvciAtV2FsbCAtV2V4dHJhIC1Xc2hhZG93IC1XdW5kZWYgLVduby11bnVzZWQtY29tbWFuZC1saW5lLWFyZ3VtZW50IC1Xbm8tYXR0cmlidXRlcyAtV25vLXBlc3NpbWl6aW5nLW1vdmUgLVduby1jKysxMS1leHRlbnNpb25zIC1Xbm8tdXNlci1kZWZpbmVkLWxpdGVyYWxzIC1Xbm8tbm9leGNlcHQtdHlwZSAtV25vLWFsaWduZWQtYWxsb2NhdGlvbi11bmF2YWlsYWJsZSAtV25vLWF0b21pYy1hbGlnbm1lbnQgLVduby1zaXplZC1kZWFsbG9jYXRpb24gLVdzaWduLWNvbXBhcmUgLVd1bnVzZWQtdmFyaWFibGUgLVd1bnVzZWQtcGFyYW1ldGVyIC1XdW5yZWFjaGFibGUtY29kZSAtV25vLXVudXNlZC1sb2NhbC10eXBlZG…

Event Timeline

curdeius created this revision.Jul 26 2021, 8:17 AM
curdeius requested review of this revision.Jul 26 2021, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2021, 8:17 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@ldionne, is this acceptable or is it too hackish?
I'm not very proud of importing dsl in format.py, but couldn't find a better solution.

@ldionne, is this acceptable or is it too hackish?

It's a bit hackish IMO. I'm trying to think of better solutions. To be sure, importing dsl.py into format.py isn't much worse than what we do for _supportsVerify.

So this is to add -fconstexpr-steps=N, correct? Clang supports that flag, GCC doesn't. Does GCC have a sufficiently large number of constexpr steps by default?

@ldionne, is this acceptable or is it too hackish?

So this is to add -fconstexpr-steps=N, correct? Clang supports that flag, GCC doesn't. Does GCC have a sufficiently large number of constexpr steps by default?

Exactly. GCC is okay by default.

@ldionne, is this acceptable or is it too hackish?

So this is to add -fconstexpr-steps=N, correct? Clang supports that flag, GCC doesn't. Does GCC have a sufficiently large number of constexpr steps by default?

Exactly. GCC is okay by default.

That would just be side-stepping the problem, but perhaps that means that Clang's default limit on constexpr steps is too low? What do you think?

@ldionne, is this acceptable or is it too hackish?

So this is to add -fconstexpr-steps=N, correct? Clang supports that flag, GCC doesn't. Does GCC have a sufficiently large number of constexpr steps by default?

Exactly. GCC is okay by default.

That would just be side-stepping the problem, but perhaps that means that Clang's default limit on constexpr steps is too low? What do you think?

I'm not sure if it's the way to go. Clang has already a limit of 1Mi (1<<20), which seems reasonably high for normal code.
However, GCC has higher limits, -fconstexpr-loop-limit=n is 1<<18 and -fconstexpr-ops-limit=n (equivalent of -fconstexpr-steps=n) is 1<<25 so about 32Mi.
Also, changing the limit in clang will mean that we need to wait like 1 year to be able to use it in all supported versions of clang.

Another option, not sure if there's anything similar there already, having a directive that will map to the appropriate compiler option.
E.g. // MAX_CONSTEXPR_STEPS: 123456 would map to -fconstexpr-steps=123456 on Clang and -fconstexpr-ops-limit=123456 on GCC, and (possibly) /fconstexpr:steps123456 on MSVC.
It could be probably made more generic to just have one directive for many flags. E.g. // MAP_COMPILE_FLAGS: constexpr-steps=123456, other-flag=something.
The advantage would be to error out on unsupported flags.
In comparison, ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED would just ignore e.g. a flag with a typo silently on all compilers.