CMake Improvements - Githubissues

stellar-aria commented 4 months ago

Over the past year I've done some work on improving the state of the CMake build system, initially inspired by fixing building on native Windows, and then expanding in scope.

This is part 1 of the work towards fully migrating to CMake as per #606.

Changes in this PR include:

Fixing the native build on Windows
Adding support for LLVM/Clang
Adopting a similar build configuration process as the current Makefile (see example below)
Auto-detection of embedded build toolchains
Build the examples with the composite target examples (these are not part of the default all target)
Support for all the GoogleTest unit tests via CTest
Adding matrix toolchain testing to the Github Actions so we can test for regressions on older toolchains
Cleaning up the GitHub Actions a bit to use cached tools (for both GCC and LLVM). This makes running the test builds much faster.
Switch from using TOOLCHAIN_PREFIX to the CMake standard CMAKE_C_COMPILER for manually selecting a toolchain

Example project:

cmake_minimum_required(VERSION 3.26)
cmake_policy(SET CMP0048 NEW)

# Fetch libDaisy
include(FetchContent)
FetchContent_Declare(daisy
  GIT_REPOSITORY https://github.com/stellar-aria/libDaisy
  GIT_TAG cmake-only
)
FetchContent_MakeAvailable(daisy)

# Our project declaration

project(daisytest VERSION 0.0.1)

set(FIRMWARE_NAME daisytest)
set(FIRMWARE_SOURCES
  ${CMAKE_CURRENT_LIST_DIR}/src/test.cpp
)
#set(FIRMWARE_DEBUG_OPT_LEVEL -O0) # (optional) to customize Debug optimization level
#set(FIRMWARE_RELEASE_OPT_LEVEL -O2) # (optional) to customize Release optimization level

# DaisyProject uses FIRMWARE_NAME and FIRMWARE_SOURCES to build a target called ${FIRMWARE_NAME}
include(DaisyProject)

target_include_directories(${FIRMWARE_NAME} PUBLIC include)

munshkr commented 3 months ago

Thank you so much for working on this! I did a quick test and for some reason found that the binary cmake is creating is much larger than one done using the old Makefile. I haven't digged into it yet but here's the memory map for both:

Cmake

           FLASH:      104468 B       128 KB     79.70%
         DTCMRAM:          0 GB       128 KB      0.00%
            SRAM:       13644 B       512 KB      2.60%
          RAM_D2:         16 KB       288 KB      5.56%
          RAM_D3:          0 GB        64 KB      0.00%
     BACKUP_SRAM:          12 B         4 KB      0.29%
         ITCMRAM:          0 GB        64 KB      0.00%
           SDRAM:          0 GB        64 MB      0.00%
       QSPIFLASH:          0 GB         8 MB      0.00%

Makefile

           FLASH:       63104 B       128 KB     48.14%
         DTCMRAM:          0 GB       128 KB      0.00%
            SRAM:       12436 B       512 KB      2.37%
          RAM_D2:         16 KB       288 KB      5.56%
          RAM_D3:          0 GB        64 KB      0.00%
     BACKUP_SRAM:          12 B         4 KB      0.29%
         ITCMRAM:          0 GB        64 KB      0.00%
           SDRAM:          0 GB        64 MB      0.00%
       QSPIFLASH:          0 GB         8 MB      0.00%

stellar-aria commented 3 months ago

Thank you so much for working on this! I did a quick test and for some reason found that the binary cmake is creating is much larger than one done using the old Makefile. I haven't digged into it yet but here's the memory map for both:

Is this on the Debug or the Release config? The Makefile currently builds Release by default with an optimization level of -O3. CMake defaults to Debug builds, which with the current toolchain files in this PR have an optimization level of -Og, equivalent to -O1 except for those options that may interfere with debugging. The Release is currently built with -Os, and while that should probably be changed for performance reasons, currently it should be smaller than the Makefile build.

The size of examples/AudioOutput built off:

HEAD w/ make: 99704 bytes
PR w/ cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DDAISY_BUILD_EXAMPLES=YES && cmake --build build: 79172 bytes
cmake but with -O3 in ArmGNUToolchain.cmake: 90980 bytes

So I think this is a config issue, the sizes shouldn't be that different otherwise.

Sizes were checked with arm-none-eabi-size

munshkr commented 3 months ago

Thanks for the thorough explanation, after posting I figured it was probably an optimization flag :) edit tested with -DCMAKE_BUILD_TYPE=Release and built something even smaller as you said

stellar-aria commented 3 months ago

Oh perfect. Yeah, I probably need to think of a better way to have both sane default optimization levels and propagate user-selected ones. I'll poke at that today.

github-actions[bot] commented 3 months ago

Test Results

151 tests ± 0 151 :white_check_mark: ±0 0s :stopwatch: ±0s 1 suites - 15 0 :zzz: ±0 1 files ± 0 0 :x: ±0

Results for commit 096de597. ± Comparison against base commit f7727edb.

This pull request removes 151 and adds 151 tests. Note that renamed tests count towards both.

``` MAX11300TestFixture ‑ verifyAdcPinConfiguration MAX11300TestFixture ‑ verifyAutoUpdate MAX11300TestFixture ‑ verifyDacPinConfiguration MAX11300TestFixture ‑ verifyDriverInitializationRoutine MAX11300TestFixture ‑ verifyGpiPinConfiguration MAX11300TestFixture ‑ verifyGpoPinConfiguration MAX11300TestFixture ‑ verifyHeterogeneousPinBehavior MAX11300TestFixture ‑ verifyReadAnalogPin MAX11300TestFixture ‑ verifyReadAnalogPinMultiple MAX11300TestFixture ‑ verifyReadDigitalPin … ``` ``` MAX11300TestFixture.verifyAdcPinConfiguration ‑ MAX11300TestFixture.verifyAdcPinConfiguration MAX11300TestFixture.verifyAutoUpdate ‑ MAX11300TestFixture.verifyAutoUpdate MAX11300TestFixture.verifyDacPinConfiguration ‑ MAX11300TestFixture.verifyDacPinConfiguration MAX11300TestFixture.verifyDriverInitializationRoutine ‑ MAX11300TestFixture.verifyDriverInitializationRoutine MAX11300TestFixture.verifyGpiPinConfiguration ‑ MAX11300TestFixture.verifyGpiPinConfiguration MAX11300TestFixture.verifyGpoPinConfiguration ‑ MAX11300TestFixture.verifyGpoPinConfiguration MAX11300TestFixture.verifyHeterogeneousPinBehavior ‑ MAX11300TestFixture.verifyHeterogeneousPinBehavior MAX11300TestFixture.verifyReadAnalogPin ‑ MAX11300TestFixture.verifyReadAnalogPin MAX11300TestFixture.verifyReadAnalogPinMultiple ‑ MAX11300TestFixture.verifyReadAnalogPinMultiple MAX11300TestFixture.verifyReadDigitalPin ‑ MAX11300TestFixture.verifyReadDigitalPin … ```

:recycle: This comment has been updated with latest results.

stellar-aria commented 3 months ago

Changes:

Made a separate "daisy-tests" CMake project for test building/running. This allows IDEs such as VSCode and CLion to be able to build (and run) both the unit tests and the main library side-by-side with different compilers, ideal for TDD/BDD development.
Fixes the github actions changes arising from this
No longer relies on "CMAKE_CROSSCOMPILING" flag, which was a bit of hack anyways, since it's usually set by the toolchain file with CMAKE_SYSTEM_NAME when it's "Generic"
Removed the "GoogleTest Adapter" extension and swapped it out for the much better integrated "testMate" extension (which already had configuration options in the settinsg.json, but was not part of the recommended workspace extensions)

asavartsov commented 3 months ago

@stellar-aria Thank you very much for your work! I tested it on my project and got an undefined reference to 'USBH_MIDI_SetReceiveCallback' linker error. Also there is a warning about changing start of .text section, not sure if it is relevant, but something that I haven't seen with Makefile.

Here is barebones example that builds fine with the Makefile approach and libDaisy from master but fails witch CMake using latest commit from the PR branch.

#include "daisy_seed.h"

using namespace daisy;

DaisySeed hardware;
MidiUsbHandler midiUsb;

int main(void)
{
    hardware.Configure();
    hardware.Init();

    MidiUsbHandler::Config midiUsbConfig;
    midiUsbConfig.transport_config.periph = MidiUsbTransport::Config::INTERNAL;
    midiUsb.Init(midiUsbConfig);

    for(;;)
    {
    }
}

cmake_minimum_required(VERSION 3.26)
cmake_policy(SET CMP0048 NEW)

# Fetch libDaisy
include(FetchContent)
FetchContent_Declare(daisy
  GIT_REPOSITORY https://github.com/stellar-aria/libDaisy
  GIT_TAG 2826863c573c30c87975b46780df81ec425259b2
)
FetchContent_MakeAvailable(daisy)

# Our project declaration

project(daisytest VERSION 0.0.1)

set(FIRMWARE_NAME daisytest)
set(FIRMWARE_SOURCES
  main.cpp
)

#set(FIRMWARE_DEBUG_OPT_LEVEL -O0) # (optional) to customize Debug optimization level
#set(FIRMWARE_RELEASE_OPT_LEVEL -O2) # (optional) to customize Release optimization level

# DaisyProject uses FIRMWARE_NAME and FIRMWARE_SOURCES to build a target called ${FIRMWARE_NAME}
include(DaisyProject)

target_include_directories(${FIRMWARE_NAME} PUBLIC include)

$ cmake -B build -DCMAKE_BUILD_TYPE=Release
$ cmake --build build
[  2%] Built target FatFs
[ 61%] Built target STM32H7XX_HAL_DRIVER
[ 64%] Built target STM32_USB_DEVICE_LIBRARY
[ 67%] Built target STM32_USB_HOST_LIBRARY
[ 98%] Built target daisy
[ 98%] Building CXX object CMakeFiles/daisytest.dir/main.cpp.obj
[100%] Linking CXX executable daisytest.elf
/Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: warning: start of section .text changed by 8
/Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/libdaisy.a(usb_midi.cpp.obj): in function `daisy::MidiUsbTransport::Impl::Init(daisy::MidiUsbTransport::Config)':
usb_midi.cpp:(.text._ZN5daisy16MidiUsbTransport4Impl4InitENS0_6ConfigE+0x2e): undefined reference to `USBH_MIDI_SetReceiveCallback'
Memory region         Used Size  Region Size  %age Used
           FLASH:      119496 B       128 KB     91.17%
         DTCMRAM:          0 GB       128 KB      0.00%
            SRAM:       65040 B       512 KB     12.41%
          RAM_D2:       17372 B       288 KB      5.89%
          RAM_D3:          0 GB        64 KB      0.00%
     BACKUP_SRAM:          12 B         4 KB      0.29%
         ITCMRAM:          0 GB        64 KB      0.00%
           SDRAM:          0 GB        64 MB      0.00%
       QSPIFLASH:          0 GB         8 MB      0.00%
collect2: error: ld returned 1 exit status
make[2]: *** [daisytest.elf] Error 1
make[1]: *** [CMakeFiles/daisytest.dir/all] Error 2
make: *** [all] Error 2

asavartsov commented 3 months ago

Also I was not able to make it work properly with LLVMEmbeddedToolchainForArm 18.0.0 on Mac. With DaisyToolchain installed cmake always detected gnu compiler no matter of TOOLCHAIN_PREFIX variable present. I removed DaisyToolchain from my system and cmake partially detected compilers and compiled project but without a bin file, only elf and hex are there. Also there is no size output with LLVM, maybe worth adding -Xlinker --print-memory-usage - it is supported by lld that is in 18.0.0 and 17.0.1 ARM LLVM toolchains.

Toolchain discovery pretty much could be my fault of not configuring project or flags properly but I tried to follow most obvious steps to make it work so others might stumble into it too.

cmake -B build -G Ninja -D CMAKE_BUILD_TYPE=Release -D TOOLCHAIN_PREFIX=/Applications/LLVMEmbeddedToolchainForArm-18.0.0-Darwin-universal
No compiler specified. Falling back to searching PATH...
Could not find arm-none-eabi-gcc. Assuming clang is part of embedded toolchain...
-- The C compiler identification is Clang 17.0.6
-- The CXX compiler identification is Clang 18.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/homebrew/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Applications/LLVMEmbeddedToolchainForArm-18.0.0-Darwin-universal/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done (62.6s)
-- Generating done (0.1s)
-- Build files have been written to: /Users/aleksei/ScratchPad/DaisyExamples/seed/Blink/build

$ cmake --build build
[a bunch of warnings from libDaisy code]
[204/204] Linking CXX executable daisytest.elf

$ ls -A1 build
.ninja_deps
.ninja_log
CMakeCache.txt
CMakeFiles
_deps
build.ninja
cmake_install.cmake
daisytest.elf
daisytest.hex

stellar-aria commented 3 months ago

@asavartsov looks like I missed a file in the merge conflict resolution, whoops. Thanks for finding that! That should fix the undefined error for the linker.

Instead of using TOOLCHAIN_PREFIX when you're using the auto-detecting toolchain files, you should use the standard CMAKE_C_COMPILER and CMAKE_CXX_COMPILER command-line settings. This integrates much better with IDEs. The autodetection script will then use that compiler instead of hunting for its own (which as you noticed, tries finding GCC first and then falling back to clang).

EDIT: So for example this is the command line used to create a configuration with LLVM on Windows:

"C:\Program Files\CMake\bin\cmake.EXE" --no-warn-unused-cli -DCMAKE_BUILD_TYPE:STRING=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=TRUE -DCMAKE_C_COMPILER:FILEPATH=C:\Developer\LLVMEmbeddedToolchainForArm\bin\clang.exe -DCMAKE_CXX_COMPILER:FILEPATH=C:\Developer\LLVMEmbeddedToolchainForArm\bin\clang++.exe -SC:/Users/Kate/GitHub/libDaisy -BC:/Users/Kate/GitHub/libDaisy/build -G Ninja

asavartsov commented 3 months ago

Thanks you! It worked perfectly both for linker error and compiler options. When you mentioned IDE I opened my project in VSCode and it gave me similar command, I should have tried it first 🤦

About bin file generation, it was as simple as set(DAISY_GENERATE_BIN true)

I cannot thank you enough again, CMake/LLVM is such a huge improvement for me because I use autogenerated DSP code which compiles forever with gcc and just in seconds with clang.

asavartsov commented 3 months ago

I also tested CMake with bootloader targets (sram and qspi) and looks like they won't boot from bootloader without -DBOOT_APP which is set by Makefile but not by CMake.

Adding target_compile_definitions(daisy PRIVATE BOOT_APP) to my project file fixes that (though it probably should be automatic based on DAISY_STORAGE value) but this define also breaks Debug CMake target if using GCC toolchain:

[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal.c.obj): in function `HAL_Init':
[build] stm32h7xx_hal.c:(.text.HAL_Init+0x6c): undefined reference to `D1CorePrescTable'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: stm32h7xx_hal.c:(.text.HAL_Init+0x70): undefined reference to `SystemD2Clock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: stm32h7xx_hal.c:(.text.HAL_Init+0x74): undefined reference to `SystemCoreClock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal.c.obj): in function `HAL_InitTick':
[build] stm32h7xx_hal.c:(.text.HAL_InitTick+0x64): undefined reference to `SystemCoreClock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_dma.c.obj): in function `HAL_DMA_IRQHandler':
[build] stm32h7xx_hal_dma.c:(.text.HAL_DMA_IRQHandler+0x1a8): undefined reference to `SystemCoreClock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_rcc.c.obj): in function `HAL_RCC_ClockConfig':
[build] stm32h7xx_hal_rcc.c:(.text.HAL_RCC_ClockConfig+0x360): undefined reference to `D1CorePrescTable'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: stm32h7xx_hal_rcc.c:(.text.HAL_RCC_ClockConfig+0x364): undefined reference to `SystemD2Clock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: stm32h7xx_hal_rcc.c:(.text.HAL_RCC_ClockConfig+0x368): undefined reference to `SystemCoreClock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_rcc.c.obj): in function `HAL_RCC_GetHCLKFreq':
[build] stm32h7xx_hal_rcc.c:(.text.HAL_RCC_GetHCLKFreq+0x54): undefined reference to `D1CorePrescTable'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: stm32h7xx_hal_rcc.c:(.text.HAL_RCC_GetHCLKFreq+0x58): undefined reference to `SystemD2Clock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: stm32h7xx_hal_rcc.c:(.text.HAL_RCC_GetHCLKFreq+0x5c): undefined reference to `SystemCoreClock'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_rcc.c.obj): in function `HAL_RCC_GetPCLK1Freq':
[build] stm32h7xx_hal_rcc.c:(.text.HAL_RCC_GetPCLK1Freq+0x28): undefined reference to `D1CorePrescTable'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_rcc.c.obj): in function `HAL_RCC_GetPCLK2Freq':
[build] stm32h7xx_hal_rcc.c:(.text.HAL_RCC_GetPCLK2Freq+0x28): undefined reference to `D1CorePrescTable'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_rcc_ex.c.obj): in function `HAL_RCCEx_GetD3PCLK1Freq':
[build] stm32h7xx_hal_rcc_ex.c:(.text.HAL_RCCEx_GetD3PCLK1Freq+0x28): undefined reference to `D1CorePrescTable'
[build] /Library/DaisyToolchain/0.2.0/arm/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: _deps/daisy-build/Drivers/libSTM32H7XX_HAL_DRIVER.a(stm32h7xx_hal_sai.c.obj): in function `SAI_Disable':
[build] stm32h7xx_hal_sai.c:(.text.SAI_Disable+0x68): undefined reference to `SystemCoreClock'

All other targets seem to be fine with GCC and all targets including Debug are fine with LLVM. Looks like GCC optimizes out some references from static lib because they are dependent on

#ifndef BOOT_APP
SystemInit();
#endif

in startup_stm32h750xx.c or something like that 🙂

stellar-aria commented 3 months ago

@asavartsov thank you so much for testing this all out! It looks like because the Makefile was building everything as one large static blob it didn't care so much about the references, but system_stm32h7xx.c actually should be compiled as part of the CMSIS Device library.

I've moved it there and changed the library type to static, as well as adding the compile definitions if the DAISY_STORAGE variable is set to anything other than flash.

This should fix the issues, please let me know if you have any more problems!

asavartsov commented 3 months ago

@stellar-aria I am really sorry if I annoy you too much! I did a little bit more testing and I have some more comments:

I tested build with all targets and different optimization options. FIRMWARE_*_OPT_LEVEL does not seem to affect libDaisy itself. Also if I build my project in Debug or Release, libDaisy sources compiled without any optimization flags at all (also no ggdb3 for Debug and no -DNDEBUG for Release). Produced ninja FLAGS for daisy files:

Release:

-std=gnu++14 [skip] -Wall

Debug:

-std=gnu++14 [skip] -Wall

MinSizeRel:

-Os -DNDEBUG -std=gnu++14 [skip] -Wall

RelWithDebInfo:

-O2 -g -DNDEBUG -std=gnu++14 [skip] -Wall

It does not look like a desired behaviour...

link_libraries(semihost) may be a little bit too bold here, it would be very hard (if even possible) to override in the parent project. For GNU toolchain you have set(CMAKE_EXE_LINKER_FLAGS "--specs=nano.specs --specs=nosys.specs" CACHE INTERNAL "Linker options") which would allow overriding, may be it is worth to have something similar for LLVM to be on par? By the way, I can confirm that semihosting works like a charm with LLVM.
I tested ArmGNUToolchain 13.2.Rel1 and LLVMEmbeddedToolchainForArm 18.0.0. GNU seems to be working fine except I could not make semihosting to work. LLVM not great news. I started to test different peripherals and I stumbled into hw.StartAudio causing hard fault with BFARVALID after the code enters SaiHandle::Impl::StartDmaTransfer if libDaisy is an optimized build. I can confirm at least -Og, -O1, -O3 triggers that rendering audio useless if compiled with LLVM toolchain. With -O0 audio starts fine and callback works. I am not sure if compiler omits something or it is CPU runtime optimization error or both. I am actively debugging it though I am not a real embedded dev 😭 but I hope I will figure it out and fix it separately. I know that it is not related to CMake support at all but more like an info for those who will try the same after this will be merged.

stellar-aria commented 3 months ago

@asavartsov no worries! I'd rather get this all worked out in the PR than have to deal with it in mainline.

1) yeah the current setup will not propagate the build flags. ~~I'll need to think on this one. I know what's causing it but it might be tricky to work around.~~ Figured it out. I've added a new patch.

2) semihost shouldn't be there, that's a whoops from my experiments

3) I have no idea what's going on there tbh. This does sound like a bug in the LLVM optimization passes which is something I have 0 experience debugging. If you're using Cortex-Debug or something like Segger's Ozone, you can step-debug the assembly output of the problem function/object file. Doing that and manually checking your registers might be the key here. Also an A/B comparison of the output object files and/or going through and manually enabling the -Og optimizations one by one to find which one is the problem.

asavartsov commented 3 months ago

Thanks! Turned out that clang optimimizes out while(1); as a loop without side effects, which is by spec but not very compatible with libDaisy code.

So my

int main() {
  hw.Init();
  hw.StartAudio(callback);

  while(1) {}
}

effectively becomes just

int main() {
  hw.Init();
  hw.StartAudio(callback);
}

and after main it tries to do some runtime on exit stuff which eventually sets sp=0x20002000 and access some offset of it which raises a hard fault.

GCC do not optimize while(1); loops so I think it would be fair to add -fno-finite-loops compiler flag to LLVM toolchain by default so it will be compatible with some default error handlers in libDaisy and probably existing code around.

Clang compiled code without a flag and with it: https://godbolt.org/z/Kx75G7hbc

stellar-aria commented 3 months ago

Yeah considering how heavily it's used in certain places of libDaisy, and even shown in the examples, I think that's a reasonable solution.

Ideally we could convert them to a static hang() function or something that's just while(1) { asm("nop"); } since that doesn't get utterly destroyed by the optimization passes, but that's a separate issue.

stellar-aria commented 2 months ago

@beserge @stephenhensley any ideas when/how this might get merged? Are there any edits that need to happen for it to be done?

beserge commented 1 month ago

Checked out and built, everything appears as it should be. I'll defer to @stephenhensley on the merge timing, but looks good to go on my end! One thing to note, I had to update my CMake install, my out of date version was giving me some trouble, but once that was done it was smooth sailing.

Segfault1602 commented 3 weeks ago

Any update on a merge timeline for this? This would be a very welcome addition!

electro-smith / libDaisy

CMake Improvements #615

Cmake

Makefile

Test Results