CMake: Set required toolchain and processor flags globally, instead of per-target #13987
Conversation
|
Oh also, almost forgot, this code uses an undocumented trick I discovered a while ago with CMake. If you just include the toolchain file before calling enable_language(), that's actually the same as using |
ciarmcom
added
the
release-type: patch
|
@multiplemonomials, thank you for your changes. |
tools/cmake/toolchains/GCC_ARM.cmake
Outdated
| MBED_RTOS_SINGLE_THREAD | ||
| __NEWLIB_NANO | ||
| ) |
There was a problem hiding this comment.
Dang, CLion has no setting for putting the closing paren at the outer indent level, and in fact it auto indents it whenever I paste code... Do you have any way to auto reformat these?
There was a problem hiding this comment.
We do not yet have auto formatter in place, if you got any suggestions , we would like to try it out.
tools/cmake/toolchains/GCC_ARM.cmake
Outdated
| "--specs=nano.specs" | ||
| ) |
tools/cmake/toolchain.cmake
Outdated
| list_to_space_separated(CMAKE_C_FLAGS_INIT ${common_options} ${c_cxx_compile_options}) | ||
| set(CMAKE_CXX_FLAGS_INIT ${CMAKE_C_FLAGS_INIT}) | ||
| list_to_space_separated(CMAKE_ASM_FLAGS_INIT ${common_options} ${asm_compile_options}) | ||
| list_to_space_separated(CMAKE_EXE_LINKER_FLAGS_INIT ${link_options}) |
There was a problem hiding this comment.
Please add a new line.
|
I'll fetch this tomorrow to review locally to get detail view as this changes lot of files. For consideration: _COMPILER_WORKS we defined and add We intentionally do not touch global symbols as
Is this referring to try_compile() check CMake is doing at first run? I wonder how project using target properties rather than touching directory/project settings shall work with this check? The initial check should not force us to add our flags to the global symbols ? I'll need to check this in detail. |
The whole point of this PR is to get us to the stage where the compiler and linker do work reliably when CMake needs to use them! In my testing with GCC at least, this problem is totally fixed.
This is good advice to follow in general, but unfortunately it does not apply in the specific case of cross compiling and toolchain files. They are an older feature of CMake and require the older style of setting up flags in order to make things work properly. Trust me, I've been using CMake for 6 years, this is the only correct way to add these flags and there isn't a way to do it properly using target flags only. |
|
I got counter proposal that fixes as well Gcc Arm checks: #13993 I provided information for the fix in the commit message. This is connected to We will still review this PR, I would like to test some of the changes here. |
| endif() | ||
| endfunction() | ||
| if(${MBED_TOOLCHAIN} STREQUAL "GCC_ARM") | ||
| list(APPEND common_options |
There was a problem hiding this comment.
would it make sense to introduce own list for each FLAGS?
MBED_TOOLCHAIN_<LANG>_FLAGS so C/CXX/ASM/LD, and we would add these to CMake _INIT flags.
These could be cached variable, similar as _INIT flags.
There was a problem hiding this comment.
That's already how I have it set up. link_options is for linker flags, common_options is for all compilers, c_cxx_compile_options is for C and CXX compilers, and asm_compile_options is for ASM only.
There was a problem hiding this comment.
correct the use is as it should. we use mbed prefix ( MBED_TOOLCHAIN or so), if these could be prefixed as well but they might not, see my comment below
There was a problem hiding this comment.
As these are not global symbols, it does not need to be prefixed neither with capital letters
|
This PR cannot be merged due to conflicts. Please rebase to resolve them. |
…pilers are loaded.
multiplemonomials
force-pushed
the
cmake-global-options
branch
from
f08168d
to
0972738
Compare
|
@multiplemonomials thanks for the PR, I'm currently testing it. First issue I have with multiple targets is that Applying the following patch will fix the issue: diff --git a/tools/cmake/toolchain.cmake b/tools/cmake/toolchain.cmake
index faec42f281..d91fd196e8 100644
--- a/tools/cmake/toolchain.cmake
+++ b/tools/cmake/toolchain.cmake
@@ -19,8 +19,8 @@ function(mbed_generate_options_for_linker target definitions_file)
set(_compile_definitions
"$<$<BOOL:${_compile_definitions}>:-D$<JOIN:${_compile_definitions}, -D>>"
)
- file(GENERATE OUTPUT "${CMAKE_BINARY_DIR}/compile_time_defs.txt" CONTENT "${_compile_definitions}\n")
- set(${definitions_file} @${CMAKE_BINARY_DIR}/compile_time_defs.txt PARENT_SCOPE)
+ file(GENERATE OUTPUT "${CMAKE_BINARY_DIR}/${target}_compile_time_defs.txt" CONTENT "${_compile_definitions}\n")
+ set(${definitions_file} @${CMAKE_BINARY_DIR}/${target}_compile_time_defs.txt PARENT_SCOPE)
endfunction()
# Set the system processor depending on the CPU core type
if (MBED_CPU_CORE STREQUAL Cortex-A9)With this fixed, I'm then facing the same issue about the linker script missing for one my targets. EDIT: The number of files compiled is also around |
|
@ladislas Yeah I thought that those issues were outside the scope of this PR. I fixed them in my other branch: https://github.com/multiplemonomials/mbed-os/tree/cmake-object-libs |
|
@0xc0170 https://github.com/cheshirekow/cmake_format looks like a nice tool |
|
Not sure why I can't reply to your comment @0xc0170 , but is it a requirement for contributions that local variables have to be lowercase? It's not the code style I used for any of the mbed-cmake submissions, and I don't believe it's what CMake themselves use for their scripts. But if so I can try to update it. |
|
Not sure how I replied by via review. Anyway, all fine , just reverting changes to the style that are not consistent (@hugueskamba commented on those couple days back). If you can revert them, we shall proceed to get this in for the upcoming release. Otherwise looks good to me. |
|
OK, all the formatting issues should hopefully be fixed. |
|
ARMClang: GCC Arm all fine. |
| ) | ||
| # Sets toolchain options | ||
| list(APPEND common_options | ||
| "-mthumb" |
There was a problem hiding this comment.
-c was removed as it's not required, just checking if it was intentionally removed.
There was a problem hiding this comment.
Yep that was intentional, CMake will add -c as needed.
| ) | ||
| list(APPEND asm_compile_options | ||
| -masm=auto | ||
| --target=arm-arm-none-eabi |
There was a problem hiding this comment.
there's not CMAKE_ASM_COMPILER_TARGET as we do above for C/CXX ?
There was a problem hiding this comment.
I don't believe so... though it kinda depends on how ARM implemented the CMake scripts for ARM Compiler. You could try adding it (and removing the manual --target flag) and see if it works.
| "-mfloat-abi=hard" | ||
| ) | ||
| list(APPEND asm_compile_options | ||
| "-mcpu=Cortex-M4" |
There was a problem hiding this comment.
this is probably causing ARMClang to fail as , the command for K64F startup file (asm file) contains -mcpu=Cortex-M4-mcpu=cortex-m4
|
@0xc0170 I've seen that issue before but it should have been fixed in this commit. Are you sure you deleted and recreated your build directory after updating to the latest version of the code? |
|
Just to make sure: I am on commit |
|
This is how my Cache looks like. I can see there are duplicates also for CXX flags (mcpu 2x). Looking at |
|
Found it https://github.com/Kitware/CMake/blob/master/Modules/Compiler/ARMClang.cmake#L97, it is set as we set |
|
Oh, nice catch! That's actually kind of a bug in the line you linked, it should have a space before |
|
OK @0xc0170 give my latest commit a shot, should be fixed. |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.6.0
Release-pending
and removed
release-type: patch
Summary of changes
On the surface, this change fixes this issue when using GCC Arm:
The cause of the issue is that certain key compiler flags were only being added on a per-target basis instead of to CMAKE_C_FLAGS etc. This is a problem because CMake needs to perform compile checks when configuring a project, and to do these it only uses CMAKE_C_FLAGS etc., not any of the per-target flags. And since the flags were missing, all of the compile checks were failing. Until now, this has been papered over by:
which lets CMake continue even though it can't successfully run the compiler (!). Also, I believe that the issue only manifests with GCC, not ARMClang, because ARMClang happens to work OK without any additional options provided. However, there are likely more subtle issues: CMake scripts that check if a function exists are likely to always fail because CMake cannot use the linker. Additionally, CMake scripts that test various attributes of the target processor (e.g. "how large is an int for this processor target") might return the wrong results because CMake is always testing with the default compiler target rather than the set one.
To fix the issue, I changed how the options for the target are collected. The toolchain file now builds a list of compiler and link options directly. It then passes these off to CMake via
CMAKE_<LANGUAGE>_FLAGS_INITandCMAKE_EXE_LINKER_FLAGS_INIT, which will get loaded intoCMAKE_C_FLAGSetc. the first time CMake is run. Yes, this method is undocumented, but it is the correct way to do it based on research I did into the CMake source code when I was creating mbed-cmake.One caveat of this method: If the toolchain or processor core is changed, then the build dir has to be deleted and the project must be reconfigured from scratch (just like with mbed-cmake). This is the only way to force CMake to pick up the new compiler flags. However, I think this change is a big win overall as it allows all of CMake's internal logic to actually function properly.
Also, I don't have access to ARMClang right now, so I could only test with GCC Arm. You will need to test that on your guys' end before merging.
Hope this all makes sense! Let me know if there are any questions or concerns.
Impact of changes
Migration actions required
core.cmake
Documentation
None
Pull request type
Test results
Reviewers
@0xc0170
@hugueskamba