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.

Sign up for GitHub

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

Jump to bottom

cmake issues #412

Closed
ghost opened this issue Jun 5, 2020 · 24 comments · Fixed by #568
Closed

cmake issues #412

ghost opened this issue Jun 5, 2020 · 24 comments · Fixed by #568
Labels
advanced 🤯 enhancement 🌈 feature 🚧 toolchain ⚙️

Comments

@ghost
Copy link

ghost commented Jun 5, 2020 •
edited by rleh
Loading

I had some issues with file globbing and other things in the generated cmake files so i ended up rewrite the hole thing based on this project.
It's a more modern use of cmake and it was really worth the effort. It is much simpler to include external modules and it gives tremendously faster compile times (recompiles in milliseconds then cached) and it has multi core support.

It includes the following features:

  • ccache
  • link time optimization
  • precompiled headers
  • doxygen

Static checks:

  • clang-tidy
  • cppcheck
  • include what you use

is there any interest in implementing it in lbuild?
@salkinium
Copy link
Member

salkinium commented Jun 5, 2020 •
edited
Loading

is there any interest in implementing it in lbuild?

YES! I know the CMake generator isn't the best way to do it, but I just don't know better. There's so much conflicting information on the interwebs regarding CMake best practice and I lack the experience to judge it. The CMake generator is basically a clone of the SCons generator, that's why everything is set globally.

I'd be very, very happy if you took a look at it, I will of course help you with the lbuild stuff! I'm particularly interested to see an easier way to include external modules, as this problem has come up many times before.

@ghost
Copy link
Author

ghost commented Jun 6, 2020 via email

@salkinium
Copy link
Member

Thanks, I'll give it a try in the next few days, currently fairly busy with work/uni.

Does LTO work for you? I was under the impression that custom linkerscript support was lacking, but that was >2 years ago…

@ghost
Copy link
Author

ghost commented Jun 6, 2020 via email

@salkinium
Copy link
Member

The listings file is completely optional, it's just there to create a disassembly so you can check how the compiler translates your code. It's very helpful for understanding C/C++ overheads etc.
The .bin and .hex commands are also optional, they are only shortcuts for creating bootloader compatible files. I wasn't sure how to properly expose this optionality in CMake.

I'd very much like LTO enabled in modm, I'll have a closer look next week. Ideally the toolchain is fixed and it just works™, then we can just set the flags as default.

@ghost
Copy link
Author

ghost commented Jun 6, 2020 via email

@salkinium
Copy link
Member

so --Wl,--wrap is not guaranteed to work with LTO for now.

That's a shame, but we only use it to wrap newlib malloc with our own implementation in :platform:heap, which is optional. LTO will be most useful on tiny devices like STM32L011, where you don't want to have a heap anyways, so I think that limitation is fine, so we can simply enable LTO if :platform:heap is not selected.

ext/gcc/cabi.c is replaced with ext/gcc/cabi.cpp and nosys.specs are dropped

modm_assert works in C too, you can therefore keep it cabi.c instead of going to C++ land. You should define all functions as modm_weak, so that the application may still overwrite them.

_sbrk is also weakly defined in modm/platform/core/no_heap.c, and assigned a specific section in the linker script to trigger a link error. This is to prevent accidental use of the heap. Your nosys replacement therefore should not define _sbrk.

@ghost
Copy link
Author

ghost commented Jun 7, 2020 via email

@salkinium
Copy link
Member

I really hate hard tabs

Yeah, me too. We briefly considered changing them, but we would need to touch every file in modm, and that would mess with git blame…

Has modm any coding guide

Yes, but it's not written in stone and clang-format cannot do this exact style, so we'll have to adapt it to what clang-format can do.
https://github.com/modm-io/modm/blob/develop/docs/coding_convention.md

@ghost
Copy link
Author

ghost commented Jun 7, 2020 via email

@salkinium
Copy link
Member

I tried your repo, but there are issues with cross-compiling.

arm-none-eabi-gcc: error: unrecognized command line option '-mmacosx-version-min=10.14'

I remember this issue, it has something to do with need to set CMAKE_SYSTEM_NAME to Generic before calling project() or something.

include(cmake/StandardProjectSettings.cmake)
include(cmake/ToolChain.cmake)
include(cmake/TargetSettings.cmake)

# Set the project name to your project name, my project isn't very descriptive
project(myproject CXX C)

But that doesn't work either. It tries to use my system clang instead of arm-none-eabi-gcc.

clang: warning: optimization flag '-finline-limit=10000' is not supported [-Wignored-optimization-argument]

Regardless of these issues, I think your CMake files are much cleaner and more modular that what we have right now, so I would like to integrate your changes into modm.

Do you want to port your changes to lbuild and open a PR?

@ghost
Copy link
Author

ghost commented Jun 9, 2020 via email

@ghost
Copy link
Author

ghost commented Jun 9, 2020 via email

@salkinium
Copy link
Member

I found this in your cmake config but it doesn't make any sense to me. I basically replaces a non existing string (on my platform) with nothing.

CMake on macOS is wierd I guess… Google told me to put this there and then it just worked.

I hope you can help me out, I don't have a MAC available.

I'll try my best, however, our CI has a macOS, so you don't have to wait on me for the PR. Still lacking a Windows CI though.

@ghost
Copy link
Author

ghost commented Jun 10, 2020

New status on Mac see: https://github.com/jasa/modm_cmake

@salkinium
Copy link
Member

I can confirm it works not only in the CI but also locally on my machine! Yay, this is very cool! Let's integrate this into modm!

@salkinium
Copy link
Member

I found this in your cmake config but it doesn't make any sense to me. I basically replaces a non existing string (on my platform) with nothing.

I remember, this was for compiling modm for Hosted using Homebrew gcc instead of clang. We use this to run the unittests on the CI/locally instead of on the target.

@salkinium salkinium mentioned this issue Sep 19, 2020
Merged
20 tasks
@ghost
Copy link
Author

ghost commented Feb 24, 2021

This came to a halt in a previous project when I could use modm. It is almost implemented so why not finish it.

There are a few issues in the current design there isn't compatible with cmake and needs to be solved for a successful implementation. ideas, wishes and criticisms are welcome.

  • Optimization flags (-Ox) is controlled by cmake and should be excluded, any idea howto handle them without breaking scons.
  • Warning flags needs a separate group instead of being in ccflags. I would like to make them private for modm so it's possible to define another set of warnings for your own code (easy to fix)
  • Example projects don't have a src folder. It's not best practice (probably very bad) to have source file in the root folder with cmake and different ide's, It's not possible to have a CMakeLists.txt for the user source.
  • External modules, complications???

@salkinium salkinium linked a pull request Feb 25, 2021 that will close this issue
cmake implementation #568
Merged
@rleh
Copy link
Member

rleh commented Feb 26, 2021

Optimization flags (-Ox) is controlled by cmake and should be excluded, any idea howto handle them without breaking scons.

CMake has to behave the same way scons currently does:

  • Size optimization -Os: scons profile=release for CMake cmake -DCMAKE_BUILD_TYPE=Release
  • Debug optimization -Og: scons profile=debug for CMake cmake -DCMAKE_BUILD_TYPE=Debug

Warning flags needs a separate group instead of being in ccflags.

That seems to make sense to me.

Example projects don't have a src folder. It's not best practice (probably very bad) to have source file in the root folder with cmake and different ide's, It's not possible to have a CMakeLists.txt for the user source.

This is only the view of CMake. Other build systems do it differently.
With our example collection this makes no sense, so CMake has to adapt here somehow.

@salkinium
Copy link
Member

Regarding LTO: enabling LTO for embedded targets will break all our custom linkerscripts for either GCC or LLVM.
https://llvm.org/devmtg/2017-10/slides/LTOLinkerScriptsEdlerVonKoch.pdf

Unfortunately it's still not fixed:
http://llvm.1065342.n5.nabble.com/llvm-dev-LTO-with-Linker-Scripts-td142857.html

Has this work been upstreamed or implemented in any toolchains?

AFAIK this effort stalled when Tobias changed jobs.

@ghost
Copy link
Author

ghost commented Mar 5, 2021 •
edited by ghost
Loading

Regarding LTO: enabling LTO for embedded targets will break all our custom linkerscripts for either GCC or LLVM.

Not, if LTO is disabled for these 2 file listed below and all other source file using HARDWARE TABLES like src/modm/platform/gpio/enable.cpp. I have done some tests with tlfs and it seems to work properly. I think linker script tables should be avoided as much as possible, they are not particular compatible with C++.

src/modm/platform/core/vectors.c
src/modm/platform/heap/heap_tlsf.cpp

@salkinium
Copy link
Member

Assertions also use linker tables, so does the heap tables and .ramcode and .fastcode. These are all fairly important for the API.

@ghost
Copy link
Author

ghost commented Mar 6, 2021

My answer was maybe to bad. Tables initialized from linker script itself is not a problem like heap_table. The optimizer will remove the code from MODM_HARDWARE_INIT macro with LTO since it seems unused. To prevent this a simple attribute as [[gnu::used]] or a pragma can solve the problem. This is not a LTO problem, but has to be solved i the modm source.

The linker wrapper issue is bug where probably will be solved i the future.

In new designs i cannot see why a thing like MODM_HARDWARE_INIT should be used, the same functionality can be achieved in pure C++.

@salkinium
Copy link
Member

In new designs i cannot see why a thing like MODM_HARDWARE_INIT should be used, the same functionality can be achieved in pure C++.

Unfortunately it cannot, since the linker is the only place that can "collect" unknown objects from multiple compilation units. The same issue with MODM_ASSERTION_HANDLER, which also collects pointers to functions in a common linker section. It's not even a language feature, but rather a linker feature.

But it's irrelevant, we'll just test it with and without LTO and see what the differences in the binary/listing are. LTO would be really nice, but ultimately there are larger code size issues with modm, like using too many templates for everything resulting in code duplication…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
advanced 🤯 enhancement 🌈 feature 🚧 toolchain ⚙️
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cmake implementation
2 participants
@salkinium @rleh