[CMAKE] Fix cross-compilation build

[vinograd47]

Summary

Call get_errc_messages only if LLVM_INCLUDE_TESTS was set.

[zero9178]

Just wondering, what's the current behaviour without these patches? I am aware that if CMAKE_CROSSCOMPILING_EMULATOR is not set, it can't be executed but does configuration automatically fail or does an error appear elsewhere down the line?

llvm/cmake/modules/GetErrcMessages.cmake

7

I believe this should also check that CMAKE_CROSSCOMPILING_EMULATOR is not set. An emulator can still be used to run the executables.
Typical example for such a platform is emscripten, where it'd use nodejs to run the produced executable. See https://cmake.org/cmake/help/v3.13/command/try_run.html#behavior-when-cross-compiling

[vinograd47]

According to https://cmake.org/cmake/help/latest/command/try_run.html#behavior-when-cross-compiling:

If the CMAKE_CROSSCOMPILING_EMULATOR is not set, the try_run will create cache variables which must be filled by the user or by presetting them in some CMake script file to the values the executable would have produced if it had been run on its actual target platform. If the cache variables were not provided, CMake configuration fails with the following errors:

CMake Error: TRY_RUN() invoked in cross-compiling mode, please set the following cache variables appropriately:
    errc_exit_code (advanced)
    errc_exit_code__TRYRUN_OUTPUT (advanced)

[jhenderson]

Is this approach going to work for all the previous cases we've identified as problematic (and hence why we ended up with this approach)? In other words, is there a risk that this will actually cause things to regress in some cases?

[abhina.sreeskantharajan]

In D98861#2634455, @jhenderson wrote:

Is this approach going to work for all the previous cases we've identified as problematic (and hence why we ended up with this approach)? In other words, is there a risk that this will actually cause things to regress in some cases?

I think if we cross-compile for a Windows platform we may see failures. We will default to python's os.strerror() which is incorrect casing on Windows (This won't affect Cygwin and MSYS environments though).

[vinograd47]

@zero9178 I've extended the condition to check only the problematic case

[zero9178]

In D98861#2634455, @jhenderson wrote:

Is this approach going to work for all the previous cases we've identified as problematic (and hence why we ended up with this approach)? In other words, is there a risk that this will actually cause things to regress in some cases?

Compared to all the previous states this does not change a whole lot. One case that @abhina.sreeskantharajan has mentioned is when cross compiling LLVM, so that the compiler runs on Windows. If one were to run tests in such a configuration, which I believe is possible if lit uses ssh or similar to go into a remote Windows host to execute the binaries, then it would lead to pythons strerror messages being used (since lit is still used on the non-Windows host I believe) and therefore getting wrong results.

There are actually ways around this problem in it's current state, but each require an extra action. One may set CMAKE_CROSSCOMPILING_EMULATOR to an "emulator" like wine. It would then compile a Windows executable and wine would execute it and give back the correct results.
Alternatively one way also set errc_exit_code and errc_exit_code__TRYRUN_OUTPUT to the stdout as if the code had been run on Windows manually.

Another thing to consider however, is that the above is only applicable in an MSVC environment. That is, if you are using MinGW to cross compile from say, a Linux to a Windows machine, we won't encounter any issues. So one would have to use wine or similar already to be running MSVC or least installed all build tools with it for clang-cl.

Functionality wise I think the patch in it's current form is good. The above situation can't really be solved, but maybe it could be documented? I am not sure.

[zero9178]

After taking a further look, I think part of the problem is that in the CMakeLists.txt of llvm, get_errc_messages get run unconditionally. I assume there are many users out there who want to cross compile, not want to set such variables and are also not interested in running tests. Maybe you could move that invocation into a block guarded by LLVM_INCLUDE_TESTS? That'd be all from me unless anyone else has any inquiries.

[vinograd47]

@zero9178 Applied your suggestion.

[jhenderson]

This latest version makes sense to me. A cross compilation without the ability to build tools to run on the host won't be able to run the tests on the host either (because they use the tools that are built). It's only where they can run those tests that we need the error message information.

LGTM, but please wait for others.

[zero9178]

LGTM as well

[abhina.sreeskantharajan]

LGTM too

[ASDenysPetrov]

I'll check the patch a bit later on my Env.

llvm/cmake/modules/GetErrcMessages.cmake

7

A bit nitting. Just don't forget to restore this line before the load to remove the file from the change list keeping the commit tidy.

[mstorsjo]

Sorry but I'm not satisfied with this direction.

As LLVM_INCLUDE_TESTS defaults to enabled, cross builds that used to work without setting any option now hard fail on the configure stage. I might not have intended to actually run the tests, and I never had any need to even think about the LLVM_INCLUDE_TESTS option before, as I could just configure and build just fine even if I never intended to build or run the tests.

This change (D98278) was introduced with the explicit motivation that "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.". That behaviour is something I was ok with - and if I would end up running the built tests somehow and 1% of them fail due to missing error texts, that's also ok. But explicitly failing the build, due to a fix intended for one single environment only, is not nice - and if that would have been clear during the review of D98278, I would have spoken up louder at that stage.

[mstorsjo]

FWIW, I would have been ok with either of the previous versions of this patch, but not this particular form of it.

[mstorsjo]

(That said, moving the check into the LLVM_INCLUDE_TESTS block surely is fine too, but it's not enough. The feature was introduced under the pretense that things would just fall back silentely if it can't be handled, not be a hard failure ever anywhere.)

[zero9178]

The issues @mstorsjo has mentioned should be addressed. Merging the second and current version of that patch would probably do the trick.

Sorry for the inconvenience.

[vinograd47]

Merged two patch variants.

[jhenderson]

LGTM, but please wait for @zero9178.

[mstorsjo]

LGTM too, thanks for making the change! But yeah, please wait for @zero9178 still.

[zero9178]

LGTM

[vinograd47]

Moved get_errc_messages call prior to all add_subdirectory commands.

[jhenderson]

llvm/CMakeLists.txt

776–779 ↗

(On Diff #332556)

You seem to have placed this between two separate blokcs both related to the Tensorflow code. It probably makes sense to move it a little earlier, to before the Tensorflow stuff.

[vinograd47]

@jhenderson Done

[jhenderson]

llvm/CMakeLists.txt

762 ↗

(On Diff #332557)

Thanks for moving. That being said, on further review, why isn't this where it was before (I assume there's a good reason for it)?

Also, perhaps you could add a comment so that users don't have to go digging to understand why this if is necessary etc.

[vinograd47]

llvm/CMakeLists.txt

762 ↗

(On Diff #332557)

I've moved this lines, because LLVM_INCLUDE_TESTS variable was defined below. I'm not sure about this comment. This if is not necessary, it is vice versa, the get_errc_messages call is needed for tests only.

llvm/cmake/modules/GetErrcMessages.cmake

 # This function returns the messages of various POSIX error codes as they are returned by std::error_code.
 # The purpose of this function is to supply those error messages to llvm-lit using the errc_messages config.
 # Currently supplied and needed error codes: ENOENT, EISDIR, EINVAL and EACCES.
 # Messages are semi colon separated.
 # Keep amount, order and tested error codes in sync with llvm/utils/lit/lit/llvm/config.py.
 function(get_errc_messages outvar)
+    if(CMAKE_CROSSCOMPILING)
+        set(${outvar} "" PARENT_SCOPE)
+        message(STATUS "Can't get errc messages in cross-compilation mode")
+        return()
+    endif()
 
     set(errc_test_code ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/CMakeTmp/getErrc.cpp)
 
     file(WRITE ${errc_test_code} "
         #include <cerrno>
         #include <iostream>
         #include <string>
         #include <system_error>