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

Configure debug info to DWARF v4. #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

microbit-carlos
Copy link
Collaborator

@microbit-carlos microbit-carlos commented Sep 8, 2023

As done by @thegecko in b0ab5ff.

This should also help with some of the issues I've seen with bloaty parsing DWARF v5 files produced by arm-none-eabi-gcc v12.3.

@microbit-carlos
Copy link
Collaborator Author

@Johnn333 could you apply the same dwarf flags to the LLVM PR and ensure they compile correctly?

@thegecko
Copy link

thegecko commented Sep 8, 2023

BTW, if you set the build mode to regular 'debug' you will also need these flags set:

  • CMAKE_C_FLAGS_DEBUG
  • CMAKE_CXX_FLAGS_DEBUG
  • CMAKE_ASM_FLAGS_DEBUG

@microbit-carlos
Copy link
Collaborator Author

microbit-carlos commented Sep 8, 2023

Looking at the docs it looks like CMAKE_<lang>_FLAGS_DEBUG_INIT initialise the flags for CMAKE_<lang>_FLAGS_DEBUG, so that should be fine?
https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_CONFIG_INIT.html

However, the docs indicate that it was only introduced in CMake v3.11 and we need to support 3.6, so maybe we need to replace the CMAKE_<lang>_FLAGS_<config>_INIT entries with CMAKE_<lang>_FLAGS_<config> anyway.

@thegecko
Copy link

thegecko commented Sep 8, 2023

Looking at the docs it looks like CMAKE_FLAGS_DEBUG_INIT initialise the flags for CMAKE_FLAGS_DEBUG, so that should be fine?

This didn't work for me, perhaps because versions

@Johnn333
Copy link

Johnn333 commented Sep 8, 2023

@Johnn333 could you apply the same dwarf flags to the LLVM PR and ensure they compile correctly?

Seems okay on my end, it still compiles, full of warnings as ever. I'm hoping to spend some time at some point getting the GCC built libraries to be compatiable with LLD (currently Clang has to build all of CODAL and use those libraries), but with some of the changes to CODAL its much closer to being able to digest the default GCC ones. Wouldn't worry too much about the LLVM build :).

The _INIT variables were introduced in CMake 3.11, and this
project currently supports 3.6+.

The CMAKE_<LANG>_FLAGS_<CONFIG> are reset here instead of using
_INIT, as we want these to be the starting values, overwritting
any CMake defaults.

Debug info set to DWARF v4 because some tooling does not process
v5 yet.
@microbit-carlos
Copy link
Collaborator Author

Okay, so essentially the CMAKE_<LANG>_FLAGS_<CONFIG>_INIT CMake variables didn't seem to be working, and they weren't overwriting the default values for CMAKE_<LANG>_FLAGS_<CONFIG>.
More info can be found in:

Since we have to support older version of CMake anyway, I've replace CMAKE_<LANG>_FLAGS_<CONFIG>_INIT with CMAKE_<LANG>_FLAGS_<CONFIG> (setting the variable, rather than appending to it, as we want these to be the starting values).

With this change and comparing the before and after this patch, there are significant changes in the build. That's because until now we've been building with -O2 instead of -Os.

As a quick test, I've change the optimisation level on top of this patch to O2, built again, and compared that with a build from master.
With this patch + O2, doing a diff of this map file and the map from master, and the only difference is the size of the debug strings, everything else is the same, so that's a decent test that this PR is working as expected.

Before we can merge this PR with -Os, we should have a look at the affected components, as listed in lancaster-university/codal-microbit-v2#373 (comment) to see if anything that might be performance critical could be negatively affected.

@JohnVidler could you have a look at the list there and confirm if we are relatively safe to introduce performance issues?

@microbit-carlos microbit-carlos marked this pull request as ready for review September 12, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants