toolchain abstraction

[tejlmand]

The main purpose of this proposal is to minimize the work needed when implementing support for an additional toolchain.

Currently, when implementing support for a new toolchain in Zephyr or when
implementing downstream support for a custom / vendor specific toolchain
there are several macros to be defined, even if the are empty.

The macro based approach also offers challenges as well as additional
maintenance burden when new macros are introduced, as all downstream users with
custom toolchains must implement those (even if empty).

Furthermore, it also requires additional split of code into cmake files that
simple implements a single macro, in order to allow a toolchain to source
only a subset. (as example: target_base.cmake, target_freestanding.cmake,
target_barmetal.cmake)
This is due to the fact that macros cannot be overwritten, once they are defined.
(In fact CMake allows for this, but let's avoid that path, as it leads to other
risks and complications).
Now, separating parts into dedicated cmake files might always be a good idea, seen from design perspective, but it should be done because it makes sense, and not just splitting because of macro reuse / lack of reusability.

This draft instead propose the use of dedicated targets and properties on those
targets.

The benefits are:

• Possibility of defining a default flag (can be cleared / overwritten later)
  (currently default is no flag)

• Possibility of inheriting a set of flags from other compiler / linker and
  overwrite only parts that differs. For example clang can inherit all of gcc
  compiler flag and only overload warnings and coverage, and clear
  security_fortify and hosted flags. See cmake/compiler/clang/compiler_flags.cmake.
  https://github.com/tejlmand/zephyr/blob/prototyping/16031_toolchain_abstraction/cmake/compiler/clang/compiler_flags.cmake

• More uniform set and use design

• Ability to manually overwrite a flag AFTER it has been used on a compile target.
  For example in app CMakeLists.txt and find_package(Zephyr)

• Clearer separation:

  • compiler flags goes to: compiler/<compiler-name>/compiler_flags.cmake
  • linker flags goes to: linker/<linker-name>/linker_flags.cmake
  • bintools flags / commands goes to: bintools/<bintools-name> cmake files.

A couple of comments related to approach chosen to questions that might arise:

• Why not use normal variables instead of target properties ?
  It would be possible to achieves almost similar behavior with variables, but
  not completely.
  But the split into a target with property ensures:

  • There will be no name collision with other variables used in the build system
  • Variables are dependent on process order, target properties are not.
    For example, if you do:

    zephyr_compile_options(${MY_VAR})
    set(MY_VAR value)
    

    will not work, but doing:

    zephyr_compile_options($<TARGET_PROPERTY:compiler,my_property>)
    set_property(TARGET compiler PROPERTY my_property value)
    

    will work.

• We ensure a more common usage principle.
  Current solution has a couple of different ways for the macros,
  x_flag vs x_flags as well as parameter handling:

  • macro(toolchain_cc_warning_base) - Sets flags directly as compile options
  • macro(toolchain_cc_optimize_for_debug_flag dest_var_name) - Sets the compile flag on dest_var_name, unless already set.
  • macro(toolchain_cc_cpp_base_flags dest_list_name) - Appends the compile flag dest_var_name, keeping any existing values on the list.
    Some macros base the behavior on the parameter.
    Some macros set linker options in the compiler macros:

  macro(toolchain_cc_asan)
    zephyr_compile_options(-fsanitize=address)
    zephyr_link_libraries(-lasan)
    zephyr_ld_options(-fsanitize=address)
  endmacro(toolchain_cc_asan)
  

  bintools macros sometimes set linker flags:

  macro(bintools_print_mem_usage)
    ...
    target_link_libraries_ifdef(${check} ${ZEPHYR_PREBUILT_EXECUTABLE} ${option})
  endmacro(bintools_print_mem_usage)
  

[daor-oti]

Looks like a good direction and starting point for simplifying the add of a new toolchain.
The reduction of the number of files needed and the possibility for inheriting the properties, and override/set those properties need and supported, reduces and simplifies the steps needed to add a new toolchain.

[mped-oticon]

Very nice, only a few nit picks.

Properties appear the way to go, as a real "data structure" rather than order-dependent variable expansion.

I like that the skeletal templates also serves nicely as a central point of Documentation.

[daor-oti]

Some initial observations.

[daor-oti]

Need the ability to add a space, in our case, or some other assignment character between the imacros property and the string result from ${AUTOCONF_H}.
Setting the imacros property as follows, seems to work in our case, but the resulting option gets enclosed in " which is probably not desirable.
set_compiler_property(PROPERTY imacros "-imacros ")

[tejlmand]

thanks for letting me know.
This is exactly why it's still a draft, in order to collect all the knowledge needed to make this abstraction as useful as possible.

Will try to include in next update round.

[daor-oti]

As agreed at the last Zephyr Toolchain Working Group meeting on May 14th, I have looked into our Toolchain/compiler support for having a single -imacros include file. This file should then #include both the autoconf.h and the zephyr_stdint.h files, thus needing only one -imacros compiler option and header.
The test was performed with the current macro/function based toolchain abstraction, and thus not with the property based toolchain abstraction from this PR, but that makes no difference in this case.

The toolchain_cc_imacros was modified to look as follows:

macro(toolchain_cc_imacros header_file)
  # Generate the common imacros include file, together with a verification header
  #  which simply produces a warning every time the file is included...
  if(NOT ${header_file} MATCHES "/zephyr_.*\.h") \
    file(WRITE /tmp/z_imacros_verf.h "#warning \"-imacros include z_imacros_verf.h\"\n")
    file(WRITE /tmp/z_imacros.h "/* Auto generated, do not edit */\n\n"
                                "#include </tmp/z_imacros_verf.h>\n")
    # Add the -imacros option to the compiler flags
    zephyr_compile_options(-imacros /tmp/z_imacros.h)
  endif()
  # Append the #include of the given header file
  file(APPEND /tmp/z_imacros.h "#include <${header_file}>\n")
endmacro()

The above results in a generated /tmp/z_imacros.h which #include autoconf.h and zephyr_stdint.h.
For our compiler, this works fine, it spits out the warning for the z_imacros_verf.h together with including the information needed from the autoconf.h and zephyr_stdint.h header files.
If other compilers supports this as well, it could be that a common imacros header should be generated, similar to above macro, but just more generic. If it turns out that not all compilers support the #include in the -imacros include file, then perhaps there should exist a way a given toolchain could have such a header file generated and added as compile option, similar to the flexibility of the toolchain_cc_imacros macro.

On a side note, I actually discovered that the reason our compiler does not accept -imacros inclusion of the two header files, is not that it does not accept two -imacros options, but rather that the zephyr_compile_options, hence target_compile_options, strips one of the -imacros options leaving only one -imacros option with two header files separated by spaces, and that the compiler does not like. The zephyr_compile_options strips the last -imacros option, as it sees it as a duplicate, since we have to have a space between the -imacros and the header file.
Adding the two -imacros options as zephyr_compile_options("-include ${header_file}") actually works for our compiler, but it adds " around each -imacros option.

[marc-hb]

Need the ability to add a space, in our case, or some other assignment character between the imacros property and the string result from ${AUTOCONF_H}.

Is this a compiler-specific requirement? Just making sure.

the reason our compiler does not accept -imacros inclusion of the two header files, is not that it does not accept two -imacros options, but rather that the zephyr_compile_options, hence target_compile_options, strips one of the -imacros options leaving only one -imacros option with two header files separated by spaces,

I understood CMake performs flag deduplication, correct? I mean not related and not specific to Zephyr. Not specific to -imacros either.

That's why there's no space, as a workaround: #7216 (comment)

[daor-oti]

Is this a compiler-specific requirement? Just making sure.

Yes, it is a compiler specific requirement. In our case, the compiler only accepts a space between the option and the header file name, nothing else.

I understood CMake performs flag deduplication, correct? I mean not related and not specific to Zephyr. Not specific to -imacros either.

That is true, CMake performs flag deduplication on any duplicated flags, not specific to -imacros, nor to Zephyr. Perhaps @tejlmand could confirm that?

That's why there's no space, as a workaround: #7216 (comment)

Yes, but as stated previously, not all compilers support the 'no space' workaround. Which is why we need a way to circumvent the CMake flag deduplication, and also handle those cases where the compiler only supports one -imacros option.

[tejlmand]

Is this a compiler-specific requirement? Just making sure.

Yes, it is a compiler specific requirement. In our case, the compiler only accepts a space between the option and the header file name, nothing else.

I understood CMake performs flag deduplication, correct? I mean not related and not specific to Zephyr. Not specific to -imacros either.

That is true, CMake performs flag deduplication on any duplicated flags, not specific to -imacros, nor to Zephyr. Perhaps @tejlmand could confirm that?

Well, that was true for older CMake versions, but since CMake v3.12, you can avoid the deduplication:

While beneficial for individual options, the de-duplication step can break up option groups. For example, -D A -D B becomes -D A B. One may specify a group of options using shell-like quoting along with a SHELL: prefix. The SHELL: prefix is dropped and the rest of the option string is parsed using the separate_arguments() UNIX_COMMAND mode. For example, "SHELL:-D A" "SHELL:-D B" becomes -D A -D B.

https://cmake.org/cmake/help/v3.12/command/target_compile_options.html

[tejlmand]

finally had time to try it out in this PR, and here is the result when compiling main.c:

/opt/zephyr-sdk-0.11.3/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc                                      
-DBUILD_VERSION=zephyr-v2.3.0-969-g2811e4ecdde1                                                     
-DKERNEL                                                                                            
-DNRF52840_XXAA                                                                                     
-D_FORTIFY_SOURCE=2                                                                                 
-D__PROGRAM_START                                                                                   
-D__ZEPHYR__=1                                                                                      
...
-isystem /projects/github/ncs/zephyr/lib/libc/minimal/include                                                
-isystem /opt/zephyr-sdk-0.11.3/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/9.2.0/include                 
-isystem /opt/zephyr-sdk-0.11.3/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/9.2.0/include-fixed           
-Os
-imacros /projects/github/ncs/zephyr/samples/hello_world/build/zephyr/include/generated/autoconf.h           
-ffreestanding                                                                                      
-fno-common                                                                                         
-g                                                                                                  
-mcpu=cortex-m4                                                                                     
-mthumb                                                                                             
-mabi=aapcs                                                                                         
-imacros /projects/github/ncs/zephyr/include/toolchain/zephyr_stdint.h                                       
...
-o CMakeFiles/app.dir/src/main.c.obj                                                                   
-c ../src/main.c                                      

and as it can be seen, there is not deduplications of -imacro.
And there is even a space between the -imacro and the header file.
@daor-oti Going to push an update soon, so that it can be tested with custom compilers.

[tejlmand]

@daor-oti feel free to try the updated version with your custom compiler.

[daor-oti]

@tejlmand Thank you, this looks good and is working perfectly with our compiler, hence no deduplication of -imacros. Also the, in our case, needed space is there without us having to explicitly add the space to the imacros property, nice.

[daor-oti]

As our tool chain do not support, and others may not support, more than one -imacros option, some method of excluding this second addition will be needed. Also, our tool chain does not support having more than one header file defined in one -imacros option.
The old toolchain_cc_imacros macro allowed for picking which imacros header to add and skip others, the new property based system does not.
Question is whether having one 'generated' header which then include autoconf.h and zephyr_stdint.h would work for all tool chains, not even sure about ours, but that could be investigated as a possible solution.

[tejlmand]

yes, there certainly need to be a way to handle this in a nice way.
And your toolchain is not the only one which only support one imacro.

[daor-oti]

Please see above comment on imacros include.

[tejlmand]

Please give feedback on the update PR.

[daor-oti]

For our compiler the above deduplication has resolved this issue, but still, there are compilers which can only take on -imacros option, so a method for handling that will still be needed.

[daor-oti]

For compilers that do not support -std and hence the default c99, the global property CSTD can simply be set to empty string as the following:
set_property(GLOBAL PROPERTY CSTD " ")
and the cstd can then be left to use the template setting.

[tejlmand]

I assume this is acceptable ?
Current Zephyr would require you to do:

 set_property(GLOBAL PROPERTY CSTD " ")

right ?

[daor-oti]

This is perfectly acceptable.
I only mentioned it as a hint that it might be needed by some compilers and that it was easily achievable by simply setting the global property to an empty string.

[tejlmand]

@TTornblom FYI

[vonhust]

nice to have a template. that will be a good start point to support new toolchains

[pfalcon]

@tejlmand: "cmake: toolchain disable -imacro deduplication" commit from this PR (merged as f109e68) broke "make zephyr_generated_headers" target, specifically creation of Makefile.exports.* (e.g. Makefile.exports.C) files containing CFLAGS, etc. for integration with external build systems. The effect of that can be seen e.g. at https://ci.linaro.org/job/lite-aeolus-micropython/PLATFORM=frdm_k64f,ZEPHYR_TOOLCHAIN_VARIANT=zephyr,label=docker-xenial-amd64-13/1317/console :

00:06:44.936 GEN build/genhdr/qstr.i.last
00:06:44.936 arm-zephyr-eabi-gcc: error: SHELL:: No such file or directory
00:06:44.936 arm-zephyr-eabi-gcc: error: SHELL:: No such file or directory

I.e., these SHELL: moniker leaks into CFLAGS, and of course leads to errors.

I see that there was a similar problem elsewhere, addressed in 9d501f4. I'll try to adapt it to "make zephyr_generated_headers", but not sure if I'll be able to, and may need some help.

[tejlmand]

@pfalcon The make zephyr_generated_headers target is just a pseudo target for dependencies, but I do see the issue with having CONFIG_MAKEFILE_EXPORTS=y that results in SHELL getting into the Z_CFLAGS file.

Already looking at a fix, so you don't need to spend time on that.

[pfalcon]

@tejlmand :

Already looking at a fix, so you don't need to spend time on that.

Thanks, appreciate that!

[tejlmand]

@tejlmand :

Already looking at a fix, so you don't need to spend time on that.

Thanks, appreciate that!

@pfalcon Could you file a medium issue, a brief description is enough, and assign directly to me.
To my knowledge, all PRs in stabilization period must have an issue they fix in order to be merged.

[pfalcon]

@pfalcon Could you file a medium issue

Done: #28230