Should libc++ .fail.cpp tests use -verify?

Hi all,

After the following commit

"commit 2fd7d364cd999c743b5bdefa7ee9e5630f3564ad
Author: Louis Dionne <ldionne@apple.com>

[libc++] Make the verify-support feature implicit

Tests that require support for Clang-verify are already marked as such
explicitly by their extension, which is .verify.cpp. Requiring the use
of an explicit Lit feature is, after thought, not really helpful.

This is a change in design: we have been bitten in the past by tests not
being enabled when we thought they were. However, the issue was mostly
with file extensions being ignored. The fix for that is not to blindly
require explicit features all the time, but instead to report all files
that are in the suite but that don't match any known test format. This
can be implemented in a follow-up patch."

All '.fail.cpp' started being run without the -verify command line
option. This reduced test coverage: the test suite is now only checking
that compilation of the .fail.cpp tests fails, but does not check the
locations of the actual errors and warnings.

It this intentional? Just in case: we are using the 'use_old_format' lit
feature because the new format does not support custom executors (e.g.
simulators) yet.

Hi,

That’s only true with the old format – the new format handles .fail.cpp tests properly (i.e. clang-verify when supported, otherwise not). Support for the old format should be removed and folks should move to the new format, which handles custom executors much better actually. For example, the new format will honour custom executors when performing feature tests to see whether e.g. locales or non-lockfree atomics are supported on the target.

The issue up to now was how to specify a custom executor from the CMake build, but that should be solved since this commit:

commit 96e6cbbf941d0f937b7e823433d4c222967a1817
Author: Louis Dionne <ldionne@apple.com>

[libc++] Allow specifying arbitrary custom executors with the new format

The integration between CMake and executor selection in the new format
wasn’t very flexible – only the default executor and SSH executors were
supported.

This patch makes it possible to specify arbitrary executors with the new
format. With the new testing format, a custom executor is just a script
that gets called with a command-line to execute, and some arguments like
–env, --codesign_identity and --execdir. As such, the default executor
is just run.py.

Remote execution with the SSH executor can be achived by specifying
LIBCXX_EXECUTOR="<path-to-ssh.py> --host ". Similarly, arbitrary
scripts can be provided.

It should now be more flexible and easier to specify arbitrary executors. You will need to change a few things since executors are now arbitrary scripts instead of subclasses of the Python executor, but that should be fairly straightforward. Can we work together to make sure you’re able to migrate to the new format?

Cheers,
Louis

Thanks a lot for the patch. We will start working on migration and will ask for
help (providing patches for review when possible) if it turns out that some
functionality that we need is missing.

Hi Louis,

While working on the migration of our infrastructure to the new format I noticed
that several '.sh.cpp' tests are failing, e.g.
libcxx/selftest/newformat/remote-substitutions.sh.cpp and
std/input.output/iostream.objects/narrow.stream.objects/cin.sh.cpp.

This happens because in our case we are targeting bare-metal platforms and the
${exec} substitution runs the provided command on a simulator. This obviously
fails when the command includes Unix shell constructs, such as I/O redirections,
pipes and shell built-ins (e.g. echo, !).

I propose to modify the set of substitutions as follows:
- add %{exec_sh} meaning "execute a shell command" (e.g. locally or on a remote
   machine). Everything after %{exec_sh} is shell-quoted and passed to the
   corresponding script.
- add %{exec_bin} meaning "execute a target binary". The corresponding script
   accepts the binary and its command-line arguments.
- make %{exec} an alias for '%{exec_sh} %{exec_bin}'

Usage example:
// RUN: %{exec_sh} echo "123" \| %{exec_bin} %t.exe > %t.out

My prototype implementation worked fine for our use cases. Does this approach
look sensible to you? If it does, I will start working on a patch.

From: libcxx-dev <libcxx-dev-bounces@lists.llvm.org> On Behalf Of Mikhail
Maltsev via libcxx-dev

...
...

This happens because in our case we are targeting bare-metal platforms and
the ${exec} substitution runs the provided command on a simulator. This
obviously fails when the command includes Unix shell constructs, such as I/O
redirections, pipes and shell built-ins (e.g. echo, !).

I propose to modify the set of substitutions as follows:
- add %{exec_sh} meaning "execute a shell command" (e.g. locally or on a
remote
  machine). Everything after %{exec_sh} is shell-quoted and passed to the
  corresponding script.
- add %{exec_bin} meaning "execute a target binary". The corresponding script
  accepts the binary and its command-line arguments.
- make %{exec} an alias for '%{exec_sh} %{exec_bin}'

Usage example:
// RUN: %{exec_sh} echo "123" \| %{exec_bin} %t.exe > %t.out

My prototype implementation worked fine for our use cases. Does this
approach look sensible to you? If it does, I will start working on a patch.

Do you have the full set of failing tests in that environment? I'd like to take a look at whether it's possible to change those tests to avoid using these constructs altogether. Furthermore, I have an incomplete patch (locally) that runs the tests in the internal Lit shell, which would also alleviate some of these issues. But ideally everything that comes after %{exec} would not require Unix shell capabilities.

I'm asking because I'd like to avoid introducing another substitution, if at all possible.

Louis

Do you have the full set of failing tests in that environment? I'd like to take a look at whether it's possible to change those tests to avoid using these constructs altogether.

Yes, the following tests are failing due to the %{exec} problem I mentioned earlier:
- libcxx/selftest/remote-substitutions.sh.cpp
- libcxx/selftest/shell-escape-pipes.sh.cpp
- libcxx/selftest/shell-escape.sh.cpp
- std/input.output/iostream.objects/narrow.stream.objects/cerr.sh.cpp
- std/input.output/iostream.objects/narrow.stream.objects/cin.sh.cpp
- std/input.output/iostream.objects/narrow.stream.objects/clog.sh.cpp
- std/input.output/iostream.objects/wide.stream.objects/wcerr.sh.cpp
- std/input.output/iostream.objects/wide.stream.objects/wcin.sh.cpp
- std/input.output/iostream.objects/wide.stream.objects/wclog.sh.cpp

Hi Louis, hi Mikhail,

Is there any progress regarding this? We have the exact same setup (bare-metal target, simulator / remote execution, failing tests due to unix constructs). For now we simply marked all those tests as XFAIL, but it would be nice to have an actual solution.

Cheers,

Dominik