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

[philnik]

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.

[curdeius]

@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]

In D106798#2904406, @curdeius wrote:

@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?

[curdeius]

In D106798#2910503, @ldionne wrote:

In D106798#2904406, @curdeius wrote:

@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]

In D106798#2911091, @curdeius wrote:

In D106798#2910503, @ldionne wrote:

In D106798#2904406, @curdeius wrote:

@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?

[curdeius]

In D106798#2911473, @ldionne wrote:

In D106798#2911091, @curdeius wrote:

In D106798#2910503, @ldionne wrote:

In D106798#2904406, @curdeius wrote:

@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.

[philnik]

This has been superseded by D132575, so I'm abandoning this patch.

libcxx/test/libcxx/selftest/additional_compile_flags_if_supported/substitutes-in-compile-flags.sh.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
+//
+//===----------------------------------------------------------------------===//
+
+// This test greps for %t, which is expanded to a path with backslashes. When
+// that is passed to grep, those backslashes must be escaped. We escape those
+// within the pattern into a file and use this file with 'grep'.
+
+// Make sure that substitutions are performed inside additional compiler flags.
+
+// ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: -I %t.1
+// ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: -isystem %t.2 , -isysroot %t.3
+// RUN: echo "-I %t.1 -isystem %t.2 -isysroot %t.3" | sed "s/\\\/\\\\\\\/g" > %t.escaped.grep
+// RUN: echo "%{compile_flags}" | grep -e -f %t.escaped.grep

libcxx/test/libcxx/selftest/additional_compile_flags_if_supported/substitutes-in-run.sh.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
+//
+//===----------------------------------------------------------------------===//
+
+// Make sure that additional compiler flags are added to the %{compile_flags}
+// substitution if supported.
+
+// ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: -Wall
+// ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: -bar
+// ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: -baz, -Wextra
+// RUN: echo "%{compile_flags}" | grep -e '-Wall -Wextra'

libcxx/utils/libcxx/test/format.py

 #===----------------------------------------------------------------------===##
 #
 # 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
 #
 #===----------------------------------------------------------------------===##
 
 import lit
 import lit.formats
 import os
 import pipes
 import re
 import shutil
 import subprocess
+from libcxx.test.dsl import hasCompileFlag
 
 def _supportsVerify(config):
     """
     Determine whether clang-verify is supported by the given configuration.
 
     This is done by checking whether the %{cxx} substitution in that
     configuration supports certain compiler flags.
     """
def parseScript(test, preamble):
 
     # Check base substitutions and add the %{build} and %{run} convenience substitutions
     _checkBaseSubstitutions(substitutions)
     substitutions.append(('%{build}', '%{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe'))
     substitutions.append(('%{run}', '%{exec} %t.exe'))
 
     # Parse the test file, including custom directives
     additionalCompileFlags = []
+    additionalOptionalCompileFlags = []
     fileDependencies = []
     parsers = [
         lit.TestRunner.IntegratedTestKeywordParser('FILE_DEPENDENCIES:',
                                                    lit.TestRunner.ParserKind.LIST,
                                                    initial_value=fileDependencies),
         lit.TestRunner.IntegratedTestKeywordParser('ADDITIONAL_COMPILE_FLAGS:',
                                                    lit.TestRunner.ParserKind.LIST,
-                                                   initial_value=additionalCompileFlags)
+                                                   initial_value=additionalCompileFlags),
+        lit.TestRunner.IntegratedTestKeywordParser('ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED:',
+                                                   lit.TestRunner.ParserKind.LIST,
+                                                   initial_value=additionalOptionalCompileFlags),
     ]
 
     scriptInTest = lit.TestRunner.parseIntegratedTestScript(test, additional_parsers=parsers,
                                                             require_script=not preamble)
     if isinstance(scriptInTest, lit.Test.Result):
         return scriptInTest
 
     script = []
 
     # For each file dependency in FILE_DEPENDENCIES, inject a command to copy
     # that file to the execution directory. Execute the copy from %S to allow
     # relative paths from the test directory.
     for dep in fileDependencies:
         script += ['%dbg(SETUP) cd %S && cp {} %T'.format(dep)]
     script += preamble
     script += scriptInTest
 
     # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
     substitutions = [(s, x + ' ' + ' '.join(additionalCompileFlags)) if s == '%{compile_flags}'
                             else (s, x) for (s, x) in substitutions]
 
+    # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED.
+    supported = [hasCompileFlag(test.config, f)
+                 for f in additionalOptionalCompileFlags]
+    # We need to remove elements in reverse order to maintain indices unchanged.
+    supported.reverse()
+    for idx, sup in enumerate(supported):
+        if not sup:
+            del additionalOptionalCompileFlags[idx]
+    substitutions = [(s, x + ' ' + ' '.join(additionalOptionalCompileFlags)) if s == '%{compile_flags}'
+                     else (s, x) for (s, x) in substitutions]
+
     # Perform substitutions in the script itself.
     script = lit.TestRunner.applySubstitutions(script, substitutions,
                                                recursion_limit=test.config.recursiveExpansionLimit)
 
     return script
 
 
 class CxxStandardLibraryTest(lit.formats.TestFormat):
also understands the following directives inside test files:
 
         // ADDITIONAL_COMPILE_FLAGS: flag1, flag2, flag3
 
             This directive will cause the provided flags to be added to the
             %{compile_flags} substitution for the test that contains it. This
             allows adding special compilation flags without having to use a
             .sh.cpp test, which would be more powerful but perhaps overkill.
 
+        // ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: flag1, flag2, flag3
+
+            This directive behaves like ADDITIONAL_COMPILE_FLAGS, but only if
+            the compiler supports the flag. Unsupported flags will be silently
+            ignored.
+
 
     Additional provided substitutions and features
     ==============================================
     The test format will define the following substitutions for use inside tests:
 
         %{build}
             Expands to a command-line that builds the current source
             file with the %{flags}, %{compile_flags} and %{link_flags}