raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.26k stars 838 forks source link

[Possible Fix] PICO_DEOPTIMIZED_DEBUG has no effect after first configuration (causes breakpoint issues) #1618

Closed recursivenomad closed 2 weeks ago

recursivenomad commented 5 months ago

Prefacing that I am new to using CMake and new to using debuggers, so take what I suggest with a grain of salt - but I believe I've identified a bug and a solution to it.

The problem:

In learning how to use picoprobe and VSCode's debugger, I configured a simple project with the Debug config using a minimal CMakeLists.txt that did not explicitly set PICO_DEOPTIMIZED_DEBUG. When I tried setting a breakpoint on the line:

        gpio_put(PIN_LED, true);

The breakpoint is greyed out in VSCode, delivering the error:

"No line 16 in file "...\main.c"

After a while of searching, it looked like this was a problem with compiler optimizations, so I tried enabling PICO_DEOPTIMIZED_DEBUG, but reconfiguring didn't change any behaviour. To my surprise, however, deleting the build directory and configuring the project fresh produced the desired results; which got me digging...

Likely cause:

My understanding is that setting CMAKE_${LANG}_FLAGS_DEBUG_INIT specifically only sets the flags for the Debug config the first time it is configured. This would mean the debug flags get saved to CMakeCache.txt when you initially configure the project, and then all subsequent re-configurations that try to set CMAKE_${LANG}_FLAGS_DEBUG_INIT have no change to the already-cached value.

What this means in this situation (I think) is that whatever PICO_DEOPTIMIZED_DEBUG is set to / defaults to during your first configuration is what will dictate all subsequent debug flags until CMakeCache.txt is deleted. I believe you can see this being the case by observing how the value of CMAKE_C_FLAGS_DEBUG in CMakeCache.txt does not change after the initial configuration, regardless of changing the value of PICO_DEOPTIMIZED_DEBUG.

Proposed solution:

I think that this section in cmake/preload/toolchains/set_flags.cmake:

    if (PICO_DEOPTIMIZED_DEBUG)
        set(CMAKE_${LANG}_FLAGS_DEBUG_INIT "-O0")
    else()
        set(CMAKE_${LANG}_FLAGS_DEBUG_INIT "-Og")
    endif()

Needs to change to:

    if (PICO_DEOPTIMIZED_DEBUG)
        set(CMAKE_${LANG}_FLAGS_DEBUG "-O0 ${CMAKE_${LANG}_FLAGS_DEBUG}")
    else()
        set(CMAKE_${LANG}_FLAGS_DEBUG "-Og ${CMAKE_${LANG}_FLAGS_DEBUG}")
    endif()

These changes remove _INIT on both commands and append the default debug flags (which is -g in this case, required for proper debugging). Appending the default flags keeps functional parity with CMAKE_${LANG}_FLAGS_DEBUG_INIT's intended behaviour.

Additionally: What is the reasoning behind defaulting to an optimized debug, if optimizing breaks certain debug functionality? Perhaps this could be an opportunity to consider making deoptimized debug builds the default for debug builds with PICO_DEOPTIMIZED_DEBUG defaulting to 1 when it is declared, in combination with the above proposed change.

I'd appreciate the eyes of someone who has been familiar with CMake for longer than a single week to validate that this is a real bug and a valid/safe solution. If this does sound reasonable, I'd be happy to submit a pull request with this simple change 🙂

peterharperuk commented 5 months ago

Pass --fresh when running cmake and it ignores the cache

recursivenomad commented 5 months ago

I'd prefer not to override a core feature of CMake when the solution proposed above addresses the issue directly.

Is there a reason I'm not aware of to preserve the use of CMAKE_${LANG}_FLAGS_DEBUG_INIT, and ask users to overwrite the cache?

peterharperuk commented 5 months ago

No. If have a change to propose, raise a PR.

kilograham commented 2 weeks ago

merged into develop