[test] Add ability to get error messages from CMake for errc substitution

[zero9178]

Summary

Visual Studios implementation of the C++ Standard Library does not use strerror to produce a message for std::error_code unlike other standard libraries such as libstdc++ or libc++ that might be used.

This patch adds a cmake script that through running a C++ program gets the error messages for the possibly POSIX error codes and passes them onto lit through an optional config parameter.

If the config parameter is not set, or getting the messages failed, due to say a cross compiling configuration without an emulator, it will fall back to using pythons strerror functions.

[jhenderson]

Just to confirm, the problem is when using libc++ that the previous attempt doesn't fix?

If so, I come back to my previous thought, namely that we could try leveraging https://cmake.org/cmake/help/latest/command/exec_program.html to run a program, and check its output to determine what pattern the messages will be. I'm not a CMake expert however, so I don't know how to get CMake to build that program at configuration time, prior to executing it. However, if we can get this to work, it would solve the issue no matter what C++ library is being used.

[zero9178]

In D98278#2616022, @jhenderson wrote:

Just to confirm, the problem is when using libc++ that the previous attempt doesn't fix?

If so, I come back to my previous thought, namely that we could try leveraging https://cmake.org/cmake/help/latest/command/exec_program.html to run a program, and check its output to determine what pattern the messages will be. I'm not a CMake expert however, so I don't know how to get CMake to build that program at configuration time, prior to executing it. However, if we can get this to work, it would solve the issue no matter what C++ library is being used.

The previous attempt only fixes it on Windows when using MSVC. In my case I am running a MinGW clang toolchain which uses libc++, but one may also use libstdc++ or even libc++ in an MSVC environment.

Regarding the latter, since this is functionality of lit in general and is used in other projects in the Mono repo, not just llvm, is there some CMake config that configures lit, regardless of the test-suite run?
A quick grep shows that %errc_* is eg. used by clang and lld as well. I was under the impression that projects may only influence the lit config through their lit.site.cfg.py.in. So if we were to add those checks to the cmake config of llvm/test those would not affect other test suites. Patching those projects lit.size.cfg.py.in to also pass the used Standard Library implementation would obviously not be a problem however.

[ASDenysPetrov]

I've checked this change. It works on my configuration at least.

[abhina.sreeskantharajan]

This check is simpler than the previous version. If it doesn't cause any new platform issues, I think it LGTM until we find a better replacement such as using cmake if possible.

[zero9178]

Add GetErrcMessages.cmake, which contains a cmake function to automatically get the error messages of various posix error codes needed by lit by running a small C++ program.
Currently ENOENT, EISDIR, EINVAL and EACCES are supplied.
These error messages are then currently supplied to clang, llvm and lld as the errc_messages config parameter.

Regarding Cross compiling: the function uses try_run which when cross compiling may use the CMAKE_CROSSCOMPILING_EMULATOR to run the code.

One thing I definitely need feedback on:
Where should the function be called and how should it be supplied to lit? I currently do so in config-ix.cmake but this eg. does not get installed with LLVM and I feel like is almost definitely wrong.
Should it be maybe be called and passed by each project to the configure_lit_site_cfg?

[mstorsjo]

In D98278#2616916, @zero9178 wrote:

Add GetErrcMessages.cmake, which contains a cmake function to automatically get the error messages of various posix error codes needed by lit by running a small C++ program.
Currently ENOENT, EISDIR, EINVAL and EACCES are supplied.
These error messages are then currently supplied to clang, llvm and lld as the errc_messages config parameter.

Regarding Cross compiling: the function uses try_run which when cross compiling may use the CMAKE_CROSSCOMPILING_EMULATOR to run the code.

How does it behave if such a thing isn't hooked up? Ideally it'd fall back silently and these parts of tests would just fail, but not block things overall.

[zero9178]

In D98278#2616932, @mstorsjo wrote:

In D98278#2616916, @zero9178 wrote:

Add GetErrcMessages.cmake, which contains a cmake function to automatically get the error messages of various posix error codes needed by lit by running a small C++ program.
Currently ENOENT, EISDIR, EINVAL and EACCES are supplied.
These error messages are then currently supplied to clang, llvm and lld as the errc_messages config parameter.

Regarding Cross compiling: the function uses try_run which when cross compiling may use the CMAKE_CROSSCOMPILING_EMULATOR to run the code.

How does it behave if such a thing isn't hooked up? Ideally it'd fall back silently and these parts of tests would just fail, but not block things overall.

It will fall back to using Python's strerror, potentially failing if pythons strerror would not return the same strings (only the case for MSVC I believe).

[abhina.sreeskantharajan]

I like the idea of falling back to python's strerror if the LLVM_LIT_ERRC_MESSAGES is not defined.

Where should the function be called and how should it be supplied to lit? I currently do so in config-ix.cmake but this eg. does not get installed with LLVM and I feel like is almost definitely wrong.
Should it be maybe be called and passed by each project to the configure_lit_site_cfg?

I'm not an expert on this part, but glancing at how the other modules are used in this folder llvm/cmake/modules/ it seems that they are included in the CMakeList.txts instead of config-ix.cmake.
For example, UseLibTool.cmake is included in llvm/CMakeLists.txt. HandleLLVMOptions.cmake is included in many CMakeLists.txt including clang/CMakeLists.txt, lld/CMakeLists.txt, llvm/CMakeLists.txt. For this patch, I think we only need it for clang, lld and llvm at the moment.

[zero9178]

Rebased and reformatted python code to 80 column limit.

Moved the call to get_errc_messages into the various subprojects using it (llvm, clang and lld). LLVM calls it unconditionally, clang and lld do in standalone builts.
HandleLLVMOptions was considered as it's included by all these functions, but I felt like it's not really an option of LLVM.
Feedback very much welcome however.

[abhina.sreeskantharajan]

These changes look good. One more thought I had is that instead of defaulting to python's os.strerror we can emit an error or warning saying that the errc_message is not defined for the project. What do you think?

[zero9178]

In D98278#2626868, @abhina.sreeskantharajan wrote:

These changes look good. One more thought I had is that instead of defaulting to python's os.strerror we can emit an error or warning saying that the errc_message is not defined for the project. What do you think?

I like that idea a lot actually! One problem I see however, is that it would probably create noise in every project, even those not using errc substitutions. An initial implementation of mine warned inside of add_err_msg_substitution, which is called in basically every project. Ideally it'd warn on the first usage of an errc substitution but that seems rather non-trivial and/or ugly to implement.

[abhina.sreeskantharajan]

In D98278#2627032, @zero9178 wrote:

In D98278#2626868, @abhina.sreeskantharajan wrote:

These changes look good. One more thought I had is that instead of defaulting to python's os.strerror we can emit an error or warning saying that the errc_message is not defined for the project. What do you think?

I like that idea a lot actually! One problem I see however, is that it would probably create noise in every project, even those not using errc substitutions. An initial implementation of mine warned inside of add_err_msg_substitution, which is called in basically every project. Ideally it'd warn on the first usage of an errc substitution but that seems rather non-trivial and/or ugly to implement.

Good point, I forgot about that issue. Adding a warning is probably something that should be considered in the future if more projects' tests start to use them. For now, this LGTM from my end, thanks for fixing!

[jhenderson]

Thanks for working on this. A couple of post-commit comments to look at, otherwise looks good. Thanks for figuring out how to do this in a portable manner!

llvm/cmake/modules/GetErrcMessages.cmake

1 ↗

(On Diff #330769)

Delete this blank line?

3–6 ↗

(On Diff #330769)

Each of these lines look like they need a trailing full stop added.

llvm/utils/lit/lit/llvm/config.py

14

Seems like this variable is unused and got leftover from an experiment at some point?

[zero9178]

llvm/cmake/modules/GetErrcMessages.cmake

3–6 ↗

(On Diff #330769)

Just to double check, that means adding a . at the end of the lines right?

llvm/utils/lit/lit/llvm/config.py

14

Correct, that was accidently left in. I immediately followed up with a commit removing it here: https://reviews.llvm.org/rG68e4084bf68ae7517491a6a0cbfd6d3b6f93cef7

[jhenderson]

llvm/cmake/modules/GetErrcMessages.cmake

3–6 ↗

(On Diff #330769)

Yes, that's right. Full stop (British English) == period (American English) == .

[zero9178]

I addressed your comments in https://reviews.llvm.org/rG4a17ac0387f078529da02e355a24df99f645d364. Hope it should be alright now

[jhenderson]

In D98278#2628433, @zero9178 wrote:

I addressed your comments in https://reviews.llvm.org/rG4a17ac0387f078529da02e355a24df99f645d364. Hope it should be alright now

Thanks!

[uabelho]

Hi,

I'm seeing a problem with this. Compiling with gcc 9.3.0 the compilation of the test program works, but then when I run it I get

/repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188)

and this causes everything to fail with

llvm-lit: /repo/uabelho/master-github/llvm/utils/lit/lit/TestingConfig.py:100: fatal: unable to parse config file '/repo/uabelho/master-github/llvm/build-all-bbigcc/tools/clang/test/lit.site.cfg.py', traceback: Traceback (most recent call last):
  File "/repo/uabelho/master-github/llvm/build-all-bbigcc/bin/../../utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/repo/uabelho/master-github/llvm/build-all-bbigcc/tools/clang/test/lit.site.cfg.py", line 19
    config.errc_messages = "/repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188)

so perhaps there should be some additional error handling when running the compiled program doesn't work?

[zero9178]

In D98278#2628477, @uabelho wrote:

Hi,

I'm seeing a problem with this. Compiling with gcc 9.3.0 the compilation of the test program works, but then when I run it I get

/repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188)

and this causes everything to fail with

llvm-lit: /repo/uabelho/master-github/llvm/utils/lit/lit/TestingConfig.py:100: fatal: unable to parse config file '/repo/uabelho/master-github/llvm/build-all-bbigcc/tools/clang/test/lit.site.cfg.py', traceback: Traceback (most recent call last):
  File "/repo/uabelho/master-github/llvm/build-all-bbigcc/bin/../../utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/repo/uabelho/master-github/llvm/build-all-bbigcc/tools/clang/test/lit.site.cfg.py", line 19
    config.errc_messages = "/repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /repo/uabelho/master-github/llvm/build-all-bbigcc/CMakeFiles/CMakeTmp/cmTC_00188)

so perhaps there should be some additional error handling when running the compiled program doesn't work?

Could you try changing line 32 of llvm/cmake/modules/GetErrcMessages.cmake from if (errc_compiled) to if (errc_compiled AND "${errc_exit_code}" STREQUAL "0") and report back if it changes anything? I think that should fix your issue, although it'll fall back to using python's strerror messages. Indeed an oversight of mine.

[uabelho]

In D98278#2628482, @zero9178 wrote:

In D98278#2628477, @uabelho wrote:

so perhaps there should be some additional error handling when running the compiled program doesn't work?

Could you try changing line 32 of llvm/cmake/modules/GetErrcMessages.cmake from if (errc_compiled) to if (errc_compiled AND "${errc_exit_code}" STREQUAL "0") and report back if it changes anything? I think that should fix your issue, although it'll fall back to using python's strerror messages. Indeed an oversight of mine.

Yes that helps, please submit. Thanks!

[zero9178]

In D98278#2628527, @uabelho wrote:

In D98278#2628482, @zero9178 wrote:

In D98278#2628477, @uabelho wrote:

so perhaps there should be some additional error handling when running the compiled program doesn't work?

Could you try changing line 32 of llvm/cmake/modules/GetErrcMessages.cmake from if (errc_compiled) to if (errc_compiled AND "${errc_exit_code}" STREQUAL "0") and report back if it changes anything? I think that should fix your issue, although it'll fall back to using python's strerror messages. Indeed an oversight of mine.

Yes that helps, please submit. Thanks!

Pushed in https://reviews.llvm.org/rG953bb5e5c8f60dc769942a3615d800fe166ffd1d

[ashi1]

In D98278#2618936, @zero9178 wrote:

In D98278#2616932, @mstorsjo wrote:

In D98278#2616916, @zero9178 wrote:

Add GetErrcMessages.cmake, which contains a cmake function to automatically get the error messages of various posix error codes needed by lit by running a small C++ program.
Currently ENOENT, EISDIR, EINVAL and EACCES are supplied.
These error messages are then currently supplied to clang, llvm and lld as the errc_messages config parameter.

Regarding Cross compiling: the function uses try_run which when cross compiling may use the CMAKE_CROSSCOMPILING_EMULATOR to run the code.

How does it behave if such a thing isn't hooked up? Ideally it'd fall back silently and these parts of tests would just fail, but not block things overall.

It will fall back to using Python's strerror, potentially failing if pythons strerror would not return the same strings (only the case for MSVC I believe).

Hi, this patch is causing a failure when cmake runs try_run() with a CMAKE_TOOLCHAIN_FILE defined. It seems related to having a fall back if the try_run fails.
Here is the error, and the build uses MSVC. What do you think we could do about this? Thanks.

[06:48:06][Step 1/4] CMake Error: TRY_RUN() invoked in cross-compiling mode, please set the following cache variables appropriately:
[06:48:06][Step 1/4]    errc_exit_code (advanced)
[06:48:06][Step 1/4]    errc_exit_code__TRYRUN_OUTPUT (advanced)
[06:48:06][Step 1/4] For details see <manually removed>/TryRunResults.cmake

[zero9178]

In D98278#2635190, @ashi1 wrote:

In D98278#2618936, @zero9178 wrote:

In D98278#2616932, @mstorsjo wrote:

In D98278#2616916, @zero9178 wrote:

Add GetErrcMessages.cmake, which contains a cmake function to automatically get the error messages of various posix error codes needed by lit by running a small C++ program.
Currently ENOENT, EISDIR, EINVAL and EACCES are supplied.
These error messages are then currently supplied to clang, llvm and lld as the errc_messages config parameter.

Regarding Cross compiling: the function uses try_run which when cross compiling may use the CMAKE_CROSSCOMPILING_EMULATOR to run the code.

How does it behave if such a thing isn't hooked up? Ideally it'd fall back silently and these parts of tests would just fail, but not block things overall.

It will fall back to using Python's strerror, potentially failing if pythons strerror would not return the same strings (only the case for MSVC I believe).

Hi, this patch is causing a failure when cmake runs try_run() with a CMAKE_TOOLCHAIN_FILE defined. It seems related to having a fall back if the try_run fails.
Here is the error, and the build uses MSVC. What do you think we could do about this? Thanks.

[06:48:06][Step 1/4] CMake Error: TRY_RUN() invoked in cross-compiling mode, please set the following cache variables appropriately:
[06:48:06][Step 1/4]    errc_exit_code (advanced)
[06:48:06][Step 1/4]    errc_exit_code__TRYRUN_OUTPUT (advanced)
[06:48:06][Step 1/4] For details see <manually removed>/TryRunResults.cmake

Hello! There is actually an ongoing discussion about this here https://reviews.llvm.org/D98861. For the time being you have two options if you have a cross compiling setup that can't natively run Windows programs.

1. Set CMAKE_CROSSCOMPILING_EMULATOR to an emulator that could execute the Windows program on your host. Wine comes to my mind as an example
2. You should be able to set the variables errc_exit_code to the value 0 and then set errc_exit_code__TRYRUN_OUTPUT to the stdout of the Windows program (alternatively leave it empty, but a few tests will fail)

[mstorsjo]

Btw, while this change does explain _what_ it does, it doesn't actually say the exact reason _why_. Cleanliness? Sure, that's nice... Or is it a case where e.g. some translations produce different error messages?

[zero9178]

In D98278#2637826, @mstorsjo wrote:

Btw, while this change does explain _what_ it does, it doesn't actually say the exact reason _why_. Cleanliness? Sure, that's nice... Or is it a case where e.g. some translations produce different error messages?

Now that you mention it, it's indeed not as clear as I thought. But yes, in the case of MSVCs STL, the messages from std::error_codes which are used by various LLVM tools produce different strings then using strerror (the C function also called by Python) with the same error codes (Specifically, it has different casing).

[mstorsjo]

In D98278#2637866, @zero9178 wrote:

In D98278#2637826, @mstorsjo wrote:

Btw, while this change does explain _what_ it does, it doesn't actually say the exact reason _why_. Cleanliness? Sure, that's nice... Or is it a case where e.g. some translations produce different error messages?

Now that you mention it, it's indeed not as clear as I thought. But yes, in the case of MSVCs STL, the messages from std::error_codes which are used by various LLVM tools produce different strings then using strerror (the C function also called by Python) with the same error codes (Specifically, it has different casing).

Ok, but would e.g. a case insensitive comparison have worked instead of this?

And didn't the python script have hardcoded strings, specifically for the MSVC case? Why weren't they written with the right casing for the case that they're supposed to match? I.e. was it an issue with the existing hardcoded strings, or did they work in one case but not another one?

[mstorsjo]

In D98278#2637866, @zero9178 wrote:

In D98278#2637826, @mstorsjo wrote:

Btw, while this change does explain _what_ it does, it doesn't actually say the exact reason _why_. Cleanliness? Sure, that's nice... Or is it a case where e.g. some translations produce different error messages?

Now that you mention it, it's indeed not as clear as I thought. But yes, in the case of MSVCs STL, the messages from std::error_codes which are used by various LLVM tools produce different strings then using strerror (the C function also called by Python) with the same error codes (Specifically, it has different casing).

Or turning the question another way around: We have a couple different bots that build and run the tests, successfully, with MSVC configurations. Are there tests that failed for you in your configuration, that succeed in the setup of the bots? Or are there other tests that aren't run as part of bots that you're fixing? It's all still very vague to me.

[zero9178]

In D98278#2638005, @mstorsjo wrote:

In D98278#2637866, @zero9178 wrote:

In D98278#2637826, @mstorsjo wrote:

Btw, while this change does explain _what_ it does, it doesn't actually say the exact reason _why_. Cleanliness? Sure, that's nice... Or is it a case where e.g. some translations produce different error messages?

Now that you mention it, it's indeed not as clear as I thought. But yes, in the case of MSVCs STL, the messages from std::error_codes which are used by various LLVM tools produce different strings then using strerror (the C function also called by Python) with the same error codes (Specifically, it has different casing).

Ok, but would e.g. a case insensitive comparison have worked instead of this?

And didn't the python script have hardcoded strings, specifically for the MSVC case? Why weren't they written with the right casing for the case that they're supposed to match? I.e. was it an issue with the existing hardcoded strings, or did they work in one case but not another one?

Changes in this patch are based on this one https://reviews.llvm.org/D97472. In the discussion there it was deemed not a good solution to use case insensitive comparison as that would make any other matches case insensitive as well, which might be a source of bugs.

In D98278#2638023, @mstorsjo wrote:

In D98278#2637866, @zero9178 wrote:

In D98278#2637826, @mstorsjo wrote:

Btw, while this change does explain _what_ it does, it doesn't actually say the exact reason _why_. Cleanliness? Sure, that's nice... Or is it a case where e.g. some translations produce different error messages?

Now that you mention it, it's indeed not as clear as I thought. But yes, in the case of MSVCs STL, the messages from std::error_codes which are used by various LLVM tools produce different strings then using strerror (the C function also called by Python) with the same error codes (Specifically, it has different casing).

Or turning the question another way around: We have a couple different bots that build and run the tests, successfully, with MSVC configurations. Are there tests that failed for you in your configuration, that succeed in the setup of the bots? Or are there other tests that aren't run as part of bots that you're fixing? It's all still very vague to me.

Prior to this patch it worke with an MSVC configuration already as the strings were correctly hardcoded for the MSVC STL. Problem was when using any other compiler configuration on Windows. In my case I am using your llvm-mingw distribution to build all of LLVM and since it uses libc++ it produces different error messages from the one in MSVC STL. I observed the same problem when using GCC on Windows and if one were to theoretically use libc++ with clang-cl in a MSVC environment, tests would be failing as well due to a mismatch in error strings.

[mstorsjo]

In D98278#2639481, @zero9178 wrote:

Changes in this patch are based on this one https://reviews.llvm.org/D97472. In the discussion there it was deemed not a good solution to use case insensitive comparison as that would make any other matches case insensitive as well, which might be a source of bugs.

Prior to this patch it worke with an MSVC configuration already as the strings were correctly hardcoded for the MSVC STL. Problem was when using any other compiler configuration on Windows. In my case I am using your llvm-mingw distribution to build all of LLVM and since it uses libc++ it produces different error messages from the one in MSVC STL. I observed the same problem when using GCC on Windows and if one were to theoretically use libc++ with clang-cl in a MSVC environment, tests would be failing as well due to a mismatch in error strings.

Ok, I think I see - so longterm (i.e. up until to a few months ago), we've just had some hardcoded (case insensitive) regexes in some testcases, and these didn't match for z/OS. As part of efforts to make it work for z/OS, a number of different incarnations have been used, and using a build-time tool to extract the real message looks like the most flexible solution in the end. And that also allows making it work properly for cases on Windows, where it's hard to know exactly which stdlib is going to be used at runtime and which kind of messages it produces.

Ok, so that makes sense to me, thanks!

llvm/utils/lit/lit/llvm/config.py

 import itertools
 import os
 import platform
 import re
 import subprocess
 import sys
 import errno
 
 import lit.util
 from lit.llvm.subst import FindTool
 from lit.llvm.subst import ToolSubst
 
 lit_path_displayed = False
 
 class LLVMConfig(object):
 
     def __init__(self, lit_config, config):
         self.lit_config = lit_config
         self.config = config
 
         features = config.available_features
 
def add_tool_substitutions(self, tools, search_dirs=None):
                 return False
 
             substitutions.append((subst_key, tool_pipe + command))
 
         self.config.substitutions.extend(substitutions)
         return True
 
     def add_err_msg_substitutions(self):
-        host_cxx = getattr(self.config, 'host_cxx', '')
-        # On Windows, python's os.strerror() does not emit the same spelling as the C++ std::error_code.
-        # As a workaround, hardcode the Windows error message.
-        if (sys.platform == 'win32' and 'MSYS' not in host_cxx):
+        # When using Visual Studio, python's os.strerror() does not emit the same spelling as the C++ std::error_code.
+        # As a workaround, hardcode the Visual Studio error messages.
+        host_triple = getattr(self.config, 'host_triple', '')
+        if re.search(r'windows-msvc', host_triple):
             self.config.substitutions.append(('%errc_ENOENT', '\'no such file or directory\''))
             self.config.substitutions.append(('%errc_EISDIR', '\'is a directory\''))
             self.config.substitutions.append(('%errc_EINVAL', '\'invalid argument\''))
             self.config.substitutions.append(('%errc_EACCES', '\'permission denied\''))
         else:
             self.config.substitutions.append(('%errc_ENOENT', '\'' + os.strerror(errno.ENOENT) + '\''))
             self.config.substitutions.append(('%errc_EISDIR', '\'' + os.strerror(errno.EISDIR) + '\''))
             self.config.substitutions.append(('%errc_EINVAL', '\'' + os.strerror(errno.EINVAL) + '\''))