Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake toolchain file's CMAKE_CXX_COMPILER_ID_RUN=true breaks compiler feature detection #222

Closed
chenxiaolong opened this issue Oct 17, 2016 · 12 comments
Labels
Milestone

Comments

@chenxiaolong
Copy link

The ndk cmake toolchain file currently sets CMAKE_CXX_COMPILER_ID_RUN=true, which causes the CMake compiler feature detection to be skipped. This causes statements like:

set_target_properties(
    mblog-static
    PROPERTIES
    CXX_STANDARD 11
    CXX_STANDARD_REQUIRED 1
)

to have no effect because CMake doesn't know what the compiler supports. I realize that everything breaks when CMAKE_CXX_COMPILER_ID_RUN=true is omitted, so is there any other workaround that doesn't involve disabling feature checks?

Thanks!


Snippet of CMakeFiles/3.6.2/CMakeCXXCompiler.cmake in my build directory showing that compiler features are not detected:

set(CMAKE_CXX_COMPILER "/opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++")
set(CMAKE_CXX_COMPILER_ARG1 "")
set(CMAKE_CXX_COMPILER_ID "Clang")
set(CMAKE_CXX_COMPILER_VERSION "")
set(CMAKE_CXX_COMPILER_WRAPPER "")
set(CMAKE_CXX_STANDARD_COMPUTED_DEFAULT "")
set(CMAKE_CXX_COMPILE_FEATURES "")
set(CMAKE_CXX98_COMPILE_FEATURES "")
set(CMAKE_CXX11_COMPILE_FEATURES "")
set(CMAKE_CXX14_COMPILE_FEATURES "")
...
@chenxiaolong
Copy link
Author

This may not be the ideal fix, but it seems to work fine, preserving cmake's ID and feature detection.

--- a/android.toolchain.cmake
+++ b/android.toolchain.cmake
@@ -293,23 +293,21 @@
 if(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows)
    set(ANDROID_TOOLCHAIN_SUFFIX .exe)
 endif()
+
+set(ANDROID_COMPILER_FLAGS)
+set(ANDROID_COMPILER_FLAGS_CXX)
+set(ANDROID_COMPILER_FLAGS_DEBUG)
+set(ANDROID_COMPILER_FLAGS_RELEASE)
+set(ANDROID_LINKER_FLAGS)
+set(ANDROID_LINKER_FLAGS_EXE)
+
 if(ANDROID_TOOLCHAIN STREQUAL clang)
    set(ANDROID_LLVM_TOOLCHAIN_PREFIX "${ANDROID_NDK}/toolchains/llvm/prebuilt/${ANDROID_HOST_TAG}/bin/")
    set(ANDROID_C_COMPILER   "${ANDROID_LLVM_TOOLCHAIN_PREFIX}clang${ANDROID_TOOLCHAIN_SUFFIX}")
    set(ANDROID_CXX_COMPILER "${ANDROID_LLVM_TOOLCHAIN_PREFIX}clang++${ANDROID_TOOLCHAIN_SUFFIX}")
-   # Clang can fail to compile if CMake doesn't correctly supply the target and
-   # external toolchain, but to do so, CMake needs to already know that the
-   # compiler is clang. Tell CMake that the compiler is really clang, but don't
-   # use CMakeForceCompiler, since we still want compile checks. We only want
-   # to skip the compiler ID detection step.
-   set(CMAKE_C_COMPILER_ID_RUN TRUE)
-   set(CMAKE_CXX_COMPILER_ID_RUN TRUE)
-   set(CMAKE_C_COMPILER_ID Clang)
-   set(CMAKE_CXX_COMPILER_ID Clang)
-   set(CMAKE_C_COMPILER_TARGET   ${ANDROID_LLVM_TRIPLE})
-   set(CMAKE_CXX_COMPILER_TARGET ${ANDROID_LLVM_TRIPLE})
-   set(CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN   "${ANDROID_TOOLCHAIN_ROOT}")
-   set(CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN "${ANDROID_TOOLCHAIN_ROOT}")
+   list(APPEND ANDROID_COMPILER_FLAGS
+       -target ${ANDROID_LLVM_TRIPLE}
+       -gcc-toolchain ${ANDROID_TOOLCHAIN_ROOT})
 elseif(ANDROID_TOOLCHAIN STREQUAL gcc)
    set(ANDROID_C_COMPILER   "${ANDROID_TOOLCHAIN_PREFIX}gcc${ANDROID_TOOLCHAIN_SUFFIX}")
    set(ANDROID_CXX_COMPILER "${ANDROID_TOOLCHAIN_PREFIX}g++${ANDROID_TOOLCHAIN_SUFFIX}")
@@ -323,13 +321,6 @@
    message(FATAL_ERROR "Invalid Android sysroot: ${CMAKE_SYSROOT}.")
 endif()

-set(ANDROID_COMPILER_FLAGS)
-set(ANDROID_COMPILER_FLAGS_CXX)
-set(ANDROID_COMPILER_FLAGS_DEBUG)
-set(ANDROID_COMPILER_FLAGS_RELEASE)
-set(ANDROID_LINKER_FLAGS)
-set(ANDROID_LINKER_FLAGS_EXE)
-
 # Generic flags.
 list(APPEND ANDROID_COMPILER_FLAGS
    -g

chenxiaolong pushed a commit to chenxiaolong/DualBootPatcher that referenced this issue Oct 17, 2016
This will require a custom toolchain file until
android/ndk#222 is fixed.
@DanAlbert
Copy link
Member

It isn't clear to me how this avoids the problem mentioned in the deleted comment. My cmake-fu is weak though. I've passed this on to the person that owns our cmake toolchain, but he's OOO for a few more days.

@chenxiaolong
Copy link
Author

It avoids the problem of having CMake detect the target and external toolchain by manually supplying those values as part of CFLAGS/CXXFLAGS. With this change, CMake is able to automatically determine that the compiler is clang and perform its full set of compiler feature detection scripts.

@piwinux
Copy link

piwinux commented Dec 1, 2016

I believe configuring project in CMake to no language will skip the compiler detection, for example:

project(foobar LANGUAGES NONE)

That way one can invoke project() again with CXX as language later on.

@ligfx
Copy link

ligfx commented Jan 22, 2017

The above proposed fix did not work for me when testing on Windows 10 :(

EDIT: Nevermind, upon setting up a minimal example the above fix does work. However, I would use a slightly different fix:

--- a/android.toolchain.cmake
+++ b/android.toolchain.cmake
 if(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows)
    set(ANDROID_TOOLCHAIN_SUFFIX .exe)
 endif()
 if(ANDROID_TOOLCHAIN STREQUAL clang)
    set(ANDROID_LLVM_TOOLCHAIN_PREFIX "${ANDROID_NDK}/toolchains/llvm/prebuilt/${ANDROID_HOST_TAG}/bin/")
    set(ANDROID_C_COMPILER   "${ANDROID_LLVM_TOOLCHAIN_PREFIX}clang${ANDROID_TOOLCHAIN_SUFFIX}")
    set(ANDROID_CXX_COMPILER "${ANDROID_LLVM_TOOLCHAIN_PREFIX}clang++${ANDROID_TOOLCHAIN_SUFFIX}")
-   # Clang can fail to compile if CMake doesn't correctly supply the target and
-   # external toolchain, but to do so, CMake needs to already know that the
-   # compiler is clang. Tell CMake that the compiler is really clang, but don't
-   # use CMakeForceCompiler, since we still want compile checks. We only want
-   # to skip the compiler ID detection step.
-   set(CMAKE_C_COMPILER_ID_RUN TRUE)
-   set(CMAKE_CXX_COMPILER_ID_RUN TRUE)
-   set(CMAKE_C_COMPILER_ID Clang)
-   set(CMAKE_CXX_COMPILER_ID Clang)
    set(CMAKE_C_COMPILER_TARGET   ${ANDROID_LLVM_TRIPLE})
    set(CMAKE_CXX_COMPILER_TARGET ${ANDROID_LLVM_TRIPLE})
    set(CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN   "${ANDROID_TOOLCHAIN_ROOT}")
    set(CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN "${ANDROID_TOOLCHAIN_ROOT}")
 elseif(ANDROID_TOOLCHAIN STREQUAL gcc)
    set(ANDROID_C_COMPILER   "${ANDROID_TOOLCHAIN_PREFIX}gcc${ANDROID_TOOLCHAIN_SUFFIX}")
    set(ANDROID_CXX_COMPILER "${ANDROID_TOOLCHAIN_PREFIX}g++${ANDROID_TOOLCHAIN_SUFFIX}")
@@ -323,13 +321,6 @@
    message(FATAL_ERROR "Invalid Android sysroot: ${CMAKE_SYSROOT}.")
 endif()

As far as I can tell, the removed comment is false for all versions of CMake since at least 3.0. Both CMAKE_{LANG}_COMPILER_TARGET and CMAKE_{LANG}_COMPILER_EXTERNAL_TOOLCHAIN are only referred to in after the code that detects CMAKE_{LANG}_COMPILER_ID. You can see this in a short excerpt from CMakeDetermineCXXCompiler.cmake:

# This section does the work to set up CMAKE_CXX_COMPILER_ID, CMAKE_CXX_COMPILER_VERSION, and other useful variables.
if(NOT CMAKE_CXX_COMPILER_ID_RUN)
include(${CMAKE_ROOT}/Modules/CMakeDetermineCompilerId.cmake)
CMAKE_DETERMINE_COMPILER_ID(CXX CXXFLAGS CMakeCXXCompilerId.cpp)
endif()

# Sole reference to CMAKE_CXX_COMPILER_TARGET
if (CMAKE_CROSSCOMPILING  AND NOT  _CMAKE_TOOLCHAIN_PREFIX)
  if("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
    if(CMAKE_CXX_COMPILER_TARGET)
      set(_CMAKE_TOOLCHAIN_PREFIX ${CMAKE_CXX_COMPILER_TARGET}-)
    endif()
  endif()
endif()

# CMakeFindBinUtils.cmake includes the sole reference to CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN
include(CMakeFindBinUtils)

So CMAKE_{LANG}_COMPILER_ID doesn't need to be set at all in the toolchain file!

As a quick overview, this bug:

  1. PreventsCMAKE_DETERMINE_COMPILER_ID from being called in CMakeDetermineCXXCompiler.cmake, which...
  2. Prevents CMAKE_CXX_COMPILER_VERSION from being set in CMAKE_DETERMINE_COMPILER_ID, which...
  3. Causes the if (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.1) check in cmake_record_cxx_compile_features in Clang-CXX.cmake to fail, and thus
  4. Avoids calling _record_compiler_features_cxx

EDIT: Submitted a patch at https://android-review.googlesource.com/#/c/326383/

@DanAlbert
Copy link
Member

Thanks to @ligfx for fixing this! https://android-review.googlesource.com/c/326383/

@DanAlbert DanAlbert added this to the r14 milestone Jan 24, 2017
@DanAlbert
Copy link
Member

We had to revert because it breaks the following test (caught by Studio's testing) https://android-review.googlesource.com/c/328500/

@DanAlbert DanAlbert reopened this Jan 26, 2017
@DanAlbert DanAlbert modified the milestones: unplanned, r14 Feb 16, 2017
@DanAlbert
Copy link
Member

Full details in the conversation on https://android-review.googlesource.com/c/328404/, but tl;dr this needed a fix to be upstreamed to cmake. It's been committed for 3.9, but until we set that as the minimum supported cmake version we can't fix this. Moving to the unplanned bucket until we can do that.

@khirowatari
Copy link

CXX_STANDARD flag seems to be working as intended on NDK r14.
This issue still open, but https://code.google.com/p/android/issues/detail?id=227915 is fixed on NDK r14 ?

@DanAlbert
Copy link
Member

There's a partial fix. CMAKE_CXX_STANDARD_COMPUTER_DEFAULT and co were hard coded to the current values (https://android.googlesource.com/platform/ndk/+/8e8a3efb0d99c7443ec50e813c0d41367c8840d9%5E%21/#F0).

This bug is still open to track removing all the cruft some day when CMake 3.9 is our minimum so we can pick up @ligfx's fixes instead.

@khirowatari
Copy link

Thanks for response.
I can use CXX_STANDARD flag from now.

a252539783 pushed a commit to a252539783/aosp-platform-ndk that referenced this issue May 3, 2017
Bug: android/ndk#222 : CMake toolchain
file's `CMAKE_CXX_COMPILER_ID_RUN=true` breaks compiler feature
detection
Bug: android/ndk#253 : cmake toolchain
file does not set CMAKE_CXX_COMPILER_VERSION
Bug: https://gitlab.kitware.com/cmake/cmake/issues/16587 : Adding
"-march=" to flags breaks determining compiler ID when cross-compiling
with Clang

The current comment reads:

> Clang can fail to compile if CMake doesn't correctly supply the target
> and external toolchain, but to do so, CMake needs to already know
> that the compiler is clang. Tell CMake that the compiler is really
> clang, but don't use CMakeForceCompiler, since we still want
> compile checks. We only want to skip the compiler ID detection
> step.

The issue arises in CMakeDetermineCompilerId.cmake, which passes along
user-specified flags like `-march=armv5te` but not the specified
compiler target. On Clang, this fails, since the validity of `-march`
depends on the current target.

I've filed a bug for CMake to fix this (allow determining the compiler
ID without using user flags). In the meantime, I think a more robust
solution than overriding CMAKE_CXX_COMPILER_ID_RUN is to run the
compiler ID checks before passing along any flags. The
CMakeDetermineCCompiler.cmake and CMakeDetermineCXXCompiler.cmake
modules are not marked for internal-use-only, and this method takes a
more proactive approach to the problem.

Change-Id: I2c250ebc2999b3f251417a9e3730b5e093e446d1
(cherry picked from commit 15909f2)
miodragdinic pushed a commit to MIPS/ndk that referenced this issue Apr 17, 2018
Bug: android/ndk#222 : CMake toolchain
file's `CMAKE_CXX_COMPILER_ID_RUN=true` breaks compiler feature
detection
Bug: android/ndk#253 : cmake toolchain
file does not set CMAKE_CXX_COMPILER_VERSION
Bug: https://gitlab.kitware.com/cmake/cmake/issues/16587 : Adding
"-march=" to flags breaks determining compiler ID when cross-compiling
with Clang

The current comment reads:

> Clang can fail to compile if CMake doesn't correctly supply the target
> and external toolchain, but to do so, CMake needs to already know
> that the compiler is clang. Tell CMake that the compiler is really
> clang, but don't use CMakeForceCompiler, since we still want
> compile checks. We only want to skip the compiler ID detection
> step.

The issue arises in CMakeDetermineCompilerId.cmake, which passes along
user-specified flags like `-march=armv5te` but not the specified
compiler target. On Clang, this fails, since the validity of `-march`
depends on the current target.

I've filed a bug for CMake to fix this (allow determining the compiler
ID without using user flags). In the meantime, I think a more robust
solution than overriding CMAKE_CXX_COMPILER_ID_RUN is to run the
compiler ID checks before passing along any flags. The
CMakeDetermineCCompiler.cmake and CMakeDetermineCXXCompiler.cmake
modules are not marked for internal-use-only, and this method takes a
more proactive approach to the problem.

Change-Id: I2c250ebc2999b3f251417a9e3730b5e093e446d1
@DanAlbert
Copy link
Member

#463 is the real fix for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants