This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by zero9178 on Mar 9 2021, 11:41 AM.

Details

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.

Diff Detail

Event Timeline

zero9178 created this revision.Mar 9 2021, 11:41 AM
zero9178 requested review of this revision.Mar 9 2021, 11:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 11:41 AM

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.

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 added a comment.EditedMar 10 2021, 1:53 AM

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

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 updated this revision to Diff 329651.Mar 10 2021, 7:42 AM

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?

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.

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

zero9178 retitled this revision from [test] Only use hardcoded errno messages when compiling with an MSVC implementation to [test] Add ability to get error messages from CMake for errc substitution.Mar 11 2021, 1:57 AM
zero9178 edited the summary of this revision. (Show Details)

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 updated this revision to Diff 330653.Mar 15 2021, 7:56 AM

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.

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?

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.

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!

This revision is now accepted and ready to land.Mar 15 2021, 12:51 PM
This revision was landed with ongoing or failed builds.Mar 15 2021, 12:57 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 12:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

Delete this blank line?

3–6

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 marked an inline comment as done.Mar 16 2021, 2:26 AM
zero9178 added inline comments.
llvm/cmake/modules/GetErrcMessages.cmake
3–6

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 added inline comments.Mar 16 2021, 2:55 AM
llvm/cmake/modules/GetErrcMessages.cmake
3–6

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

zero9178 marked 4 inline comments as done.Mar 16 2021, 3:10 AM

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

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

Thanks!

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 added a comment.EditedMar 16 2021, 3:38 AM

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.

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!

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 added a subscriber: ashi1.Mar 18 2021, 10:52 AM

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

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)

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?

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

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?

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.

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.

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.

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!