Add CMake functions for time_critical section placement of target sources

[steffenyount]

I've broken out from my original PR for #2653 as @will-v-pi suggested.

This change introduces two new CMake functions:

pico_sections_time_critical(TARGET [SOURCES])

Which will prefix the target's object file sections with ".time_critical" based on matching them to source files.

pico_sections_not_in_flash(TARGET [SOURCES])

Which will prefix the target's object file sections with ".time_critical_ram" based on matching them to source files.

These function's names mirror those of their code macro counterparts in the sections.h file. They are intended to be compatible with the "using XIP cache as SRAM" changes being introduced in #2660 but don't require those changes to be used with the existing binary type builds.

[will-v-pi]

Testing this with the pico_stdio_test_uart test in the SDK, by adding pico_sections_not_in_flash(pico_stdio_test_uart), I'm getting an error

CMake Error at src/rp2_common/pico_platform_sections/CMakeLists.txt:56 (add_custom_command):
  Error evaluating generator expression:

    $<LIST:TRANSFORM,$<LIST:FILTER,$<LIST:TRANSFORM,$<TARGET_OBJECTS:pico_stdio_test_uart>,REPLACE,/./,/>,INCLUDE,$<LIST:JOIN,$<LIST:TRANSFORM,$<LIST:TRANSFORM,$<LIST:TRANSFORM,$<LIST:FILTER,$<LIST:TRANSFORM,$<IF:$<STREQUAL:,>,$<TARGET_PROPERTY:pico_stdio_test_uart,SOURCES>,>,REPLACE,^$<TARGET_PROPERTY:pico_stdio_test_uart,SOURCE_DIR>/,>,EXCLUDE,^/|.h$>,PREPEND,/$<TARGET_NAME:pico_stdio_test_uart>.dir/>,APPEND,.o$>,REPLACE,\.,\\.>,|>>,PREPEND,COMMAND /usr/bin/arm-none-eabi-objcopy --prefix-alloc-sections .time_critical_ram >

  Expression did not evaluate to a known generator expression
Call Stack (most recent call first):
  test/pico_stdio_test/CMakeLists.txt:8 (pico_sections_not_in_flash)

My CMake version is 3.25.1, and it looks like most of the LIST expressions used here were only added in 3.27, so that might be the reason - note that the current SDK supports minimum version 3.13 (or 3.17 if using the automatic picotool download & build)

[steffenyount]

Testing this with the pico_stdio_test_uart test in the SDK, by adding pico_sections_not_in_flash(pico_stdio_test_uart), I'm getting an error

My CMake version is 3.25.1, and it looks like most of the LIST expressions used here were only added in 3.27, so that might be the reason - note that the current SDK supports minimum version 3.13 (or 3.17 if using the automatic picotool download & build)

Bummer, but thanks for testing and figuring this out!

I'm new to CMake and had no idea I was using such recent features. My CMake version is 3.28.1, and the 3.27 release notes from July 18, 2023 support your observation about the LIST expression's introduction. I assume this is indeed the cause of those errors.

I also assume it's an unreasonable ask to have the pico-sdk's minimum version bumped to 3.27, is that correct? Is it an important goal for the pico-sdk to support older versions of CMake down to version 3.13 from November 20, 2018 or version 3.17 from March 20, 2020? What determines the pico-sdk's minimum version?

In Java projects that I've worked on using Gradle based builds it's common practice to follow the latest releases of the Gradle toolchain and even to specify a particular Gradle version to be used within a project for stability and build reproducibility reasons. Is that strategy also common in the CMake world? Is that strategy just not applicable here to the pico-sdk since an sdk needs to support a wide range of CMake versions being specified in the projects that consume it?

This seems like a deal breaker for merging my CMake functions in their current form, is that correct?

If so, do you have any ideas for an alternate but similarly straight forward and portable implementation that calls objcopy with a selected list of object file names?

[will-v-pi]

We want to support the CMake versions installed by major Linux operating systems (including Raspberry Pi OS), which are generally a bit behind the latest version (eg Ubuntu 22.04 uses 3.22, Debian Bullseye uses 3.18 and Bookworm uses 3.25), so 3.27 would be too recent for that.

You could probably do it with regular list and string functions rather than generator expressions, which are present in older versions - that would require you call the function after adding all your sources, but I think that’s a reasonable compromise

Also, rather than echoing execute_process to a file then executing that file, I think you could probably just execute that command directly with add_custom_command

Also, as these functions would probably share common functionality (just different section names), it would be better to have a common function they both call, rather than duplicating the internals

[steffenyount]

We want to support the CMake versions installed by major Linux operating systems (including Raspberry Pi OS), which are generally a bit behind the latest version (eg Ubuntu 22.04 uses 3.22, Debian Bullseye uses 3.18 and Bookworm uses 3.25), so 3.27 would be too recent for that.

That makes a lot of sense.

You could probably do it with regular list and string functions rather than generator expressions, which are present in older versions - that would require you call the function after adding all your sources, but I think that’s a reasonable compromise

Would it be acceptable for merge if we just don't make these utility functions available to projects that use CMake versions < 3.27, while leaving them available for use in projects that do use CMake versions >= 3.27?

I've updated the PR to use this approach.

Also, rather than echoing execute_process to a file then executing that file, I think you could probably just execute that command directly with add_custom_command

I settled on the current implementation with the external file because of two limitations that I ran into:

1. I believe I require the add_custom_command()'s PRE_LINK time for correct $<TARGET_OBJECTS:${TARGET}> evaluation
2. I couldn't get my expression generated list property to be parsed as multiple COMMAND invocations when evaluated directly within the add_custom_command() block.

Maybe your CMake-foo is stronger than mine. Do you see a path around those limitations?

Also, as these functions would probably share common functionality (just different section names), it would be better to have a common function they both call, rather than duplicating the internals

I've consolidated their common implementation into a new prefix_alloc_sections_for_linker_placement() function. See the PR for details.

[lurch]

Would it be acceptable for merge if we just don't make these utility functions available to projects that use CMake versions < 3.27, while leaving them available for use in projects that do use CMake versions >= 3.27?

That would probably lead to a lot of user-confusion (and possibly bug-reports against pico-sdk 🐛 ) if somebody (with a newer version of CMake installed) created an "example project" that made use of one of these new functions; and then somebody else (with an older version of CMake) downloaded and tried to build that project? 🤔

pico-sdk tends to put a lot of effort into maintaining backwards-compatibility.

[will-v-pi]

Yes, I agree with @lurch that we wouldn’t want to merge this so it requires CMake 3.27, especially given it can probably be implemented in a way that still supports earlier CMake

I settled on the current implementation with the external file because of two limitations that I ran into:

1. I believe I require the add_custom_command()'s PRE_LINK time for correct $<TARGET_OBJECTS:${TARGET}> evaluation
2. I couldn't get my expression generated list property to be parsed as multiple COMMAND invocations when evaluated directly within the add_custom_command() block.

Maybe your CMake-foo is stronger than mine. Do you see a path around those limitations?

Ah yes, I hadn’t twigged that you were generating multiple commands for execute_process - in that case I think your method is the best way to do it with generator expressions

But if you use regular get_target_property etc for earlier CMake support you could instead have a for loop calling add_custom_command for each command, which I think would be cleaner and more readable