[libc++] Run the test suite with optimizations by default

[ldionne]

In production, libc++ is used with optimizations enabled 99.9% of the time. Therefore, I think it makes sense to run our tests with optimizations enabled by default, to be closer to what people actually use in production. It's still possible to disable optimizations for debugging purposes or other reasons.

This also removes the pitfall of trying to run the benchmarks but forgetting to set the optimization level.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

In production, libc++ is used with optimizations enabled 99.9% of the time. Therefore, I think it makes sense to run our tests with optimizations enabled by default, to be closer to what people actually use in production. It's still possible to disable optimizations for debugging purposes or other reasons.

This also removes the pitfall of trying to run the benchmarks but forgetting to set the optimization level.

---

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

1 Files Affected:

• (modified) libcxx/utils/libcxx/test/params.py (+1-1)

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index 2c5cb169c0a9a3..8ecd1a32799a31 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -193,7 +193,7 @@ def getSuitableClangTidy(cfg):
         choices=["none", "speed", "size"],
         type=str,
         help="The optimization level to use when compiling the test suite.",
-        default="none",
+        default="speed",
         actions=lambda opt: filter(None, [
             AddCompileFlag(lambda cfg: getSpeedOptimizationFlag(cfg)) if opt == "speed" else None,
             AddCompileFlag(lambda cfg: getSizeOptimizationFlag(cfg)) if opt == "size" else None,

[philnik777]

This significantly increases the time it takes to run the testsuite and might actually be detrimental to finding bugs, since the sanitizers tend to be less effective due to the optimizer removing UB in some cases. What is the benefit here?

[ldionne]

This significantly increases the time it takes to run the testsuite and might actually be detrimental to finding bugs, since the sanitizers tend to be less effective due to the optimizer removing UB in some cases. What is the benefit here?

The benefit is that we're testing what people actually ship in production. I'd be OK with not making it the default, but I think we should at least have a job that enables optimizations. For example, it looks like this breaks the CI as-is.

[philnik777]

This significantly increases the time it takes to run the testsuite and might actually be detrimental to finding bugs, since the sanitizers tend to be less effective due to the optimizer removing UB in some cases. What is the benefit here?

The benefit is that we're testing what people actually ship in production. I'd be OK with not making it the default, but I think we should at least have a job that enables optimizations. For example, it looks like this breaks the CI as-is.

Shouldn't the generic-optimized-speed already do this? I'm not sure exactly what the failures are, but it looks like the C++03 ones are most likely unwarranted assumptions about operator now being always called (which I agree is a bug in the test suite) and GCC seems to have a bunch of false-positive warnings. This seems like a rather marginal yield for making the CI even slower. I'd be very happy with something like once a week we check all configurations with optimizations enabled if you think it'd be worth the effort to set this up.

[ldionne]

This significantly increases the time it takes to run the testsuite and might actually be detrimental to finding bugs, since the sanitizers tend to be less effective due to the optimizer removing UB in some cases. What is the benefit here?

The benefit is that we're testing what people actually ship in production. I'd be OK with not making it the default, but I think we should at least have a job that enables optimizations. For example, it looks like this breaks the CI as-is.

Shouldn't the generic-optimized-speed already do this?

I didn't remember that we already had that. That solves most of my concerns. If I had remembered that, I don't think I'd have created this patch.