cmake: Adjust LMA for user-specified sections #68040

57300 · 2024-01-24T08:42:54Z

Add support for a new Kconfig symbol: BUILD_OUTPUT_ADJUST_LMA_SECTIONS. This is supplemental to the existing BUILD_OUTPUT_ADJUST_LMA setting, which normally adjusts all output sections' LMA by the provided offset. Defining the new symbol will narrow down the set of applicable sections to a user-specified CMake list of name patterns.

Example usage:

DT_CHOSEN_Z_FLASH = zephyr,flash
DT_CHOSEN_Z_SRAM = zephyr,sram
config BUILD_OUTPUT_ADJUST_LMA
        default "$(dt_chosen_reg_addr_hex,$(DT_CHOSEN_Z_FLASH)) - \
                 $(dt_chosen_reg_addr_hex,$(DT_CHOSEN_Z_SRAM))"

config BUILD_OUTPUT_ADJUST_LMA_SECTIONS
        default "*;!bss;!noinit"

Supported values for BUILD_OUTPUT_ADJUST_LMA_SECTIONS are aligned with objcopy, since this feature has only been supported with GNU binutils thus far.

Fixes zephyrproject-rtos#64149 Add support for a new Kconfig symbol: BUILD_OUTPUT_ADJUST_LMA_SECTIONS. This is supplemental to the existing BUILD_OUTPUT_ADJUST_LMA setting, which normally adjusts all output sections' LMA by the provided offset. Defining the new symbol will narrow down the set of applicable sections to a user-specified CMake list of name patterns. Example usage: DT_CHOSEN_Z_FLASH = zephyr,flash DT_CHOSEN_Z_SRAM = zephyr,sram config BUILD_OUTPUT_ADJUST_LMA default "$(dt_chosen_reg_addr_hex,$(DT_CHOSEN_Z_FLASH)) - \ $(dt_chosen_reg_addr_hex,$(DT_CHOSEN_Z_SRAM))" config BUILD_OUTPUT_ADJUST_LMA_SECTIONS default "*;!bss;!noinit" Supported values for BUILD_OUTPUT_ADJUST_LMA_SECTIONS are aligned with objcopy, since this feature has only been supported with GNU binutils thus far. Signed-off-by: Grzegorz Swiderski <[email protected]>

tejlmand

Thanks for this proposal.

Very useful, indeed.

This PR raises a more fundamental question related to the toolchain infrastructure and support for 3rd party toolchains.

The idea behind https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/bintools/bintools_template.cmake is that other toolchains can describe what argument to use.

For example arc mwdt (similar for armclang), can within https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/bintools/arcmwdt/target_bintools.cmake set

set_property(TARGET bintools PROPERTY elfconvert_flag_lma_adjust "-DLMA_ADJUST=")

and update the script with details on how to pass on a single size information.
There could be other toolchains which could be called directly with right set of flags, not needing an intermediate script.

But as LMA adjustment is no longer just a size being passed on, but instead a pattern, like !foo+4092, then chances are low that such a parameter can be passed on as-is.

So we need to be really careful here.

I believe that keeping a list of section names, including the ! as option to invert is acceptable, as other toolchains would need to support section names / not section name.
But the concatenation of secname, +, value is bit more problematic.

Note, the abstraction is also a reason why the property is called lma_adjust, and not named identical to the objcopy flag.

It could be considered if section adjustment should have its own property name, so we leave the general image adjustment as-is, as the use-case for that feature was quite different, ref: dee5a7e, #41685, #41579

tejlmand · 2024-01-24T10:00:22Z

Kconfig.zephyr

@@ -704,6 +704,23 @@ config BUILD_OUTPUT_ADJUST_LMA
 	    default "$(dt_chosen_reg_addr_hex,$(DT_CHOSEN_IMAGE_M4))-\
 	             $(dt_chosen_reg_addr_hex,$(DT_CHOSEN_Z_FLASH))"

+config BUILD_OUTPUT_ADJUST_LMA_SECTIONS
+	def_string "*"
+	depends on BUILD_OUTPUT_ADJUST_LMA!=""


Suggested change

depends on BUILD_OUTPUT_ADJUST_LMA!=""

depends on BUILD_OUTPUT_ADJUST_LMA != ""

this comment seems to have been overlooked.

tejlmand · 2024-01-24T10:28:31Z

Kconfig.zephyr

+	  By default, all sections will have their LMA adjusted. The following
+	  example excludes one section produced by the code relocation feature:
+	  config BUILD_OUTPUT_ADJUST_LMA_SECTIONS
+	    default "*;!.extflash_text_reloc"


this becomes very GNU bintools centric, something we try to abstract away in order to ease support for 3rd party toolchains.

The value format is indeed GNU-centric, but I don't necessarily see this as a blocker:

For LLVM, I don't believe llvm-objcopy supports --change-section-lma today, but if it were added, then I'm guessing it would be compatible.

For the toolchains that already use CMake wrapper scripts, they can always translate the values freely. They can also throw an error if they recognize some part of a value to be unsupported, e.g., wildcards.

The value format is indeed GNU-centric, but I don't necessarily see this as a blocker:

I didn't say it was a blocker, but we should always be careful in such cases, and always consider if code can be made in a way which abstracts such setting in more generic way.

And keeping values separated often ease later processing.

accepted for now, although this probably has to be reworked if toolchains like ARM Compiler 6 or arc metaware is going to work with this.

tejlmand · 2024-01-24T10:31:02Z

CMakeLists.txt

@@ -1524,11 +1524,14 @@ endif()

 if(CONFIG_BUILD_OUTPUT_ADJUST_LMA)
  math(EXPR adjustment "${CONFIG_BUILD_OUTPUT_ADJUST_LMA}" OUTPUT_FORMAT DECIMAL)
+  set(args_adjustment ${CONFIG_BUILD_OUTPUT_ADJUST_LMA_SECTIONS})
+  list(TRANSFORM args_adjustment PREPEND $<TARGET_PROPERTY:bintools,elfconvert_flag_lma_adjust>)
+  list(TRANSFORM args_adjustment APPEND +${adjustment})


Appending + here makes this quite GNU bintools centric, thus harder for other 3rd party toolchains to add support for this feature.

I think the real blocker here, now that I realize it, is the lma_adjust flag being repeated.

For GNU, this translates to:

objcopy --change-section-lma *+<value> --change-section-lma !bss+<value> ...

But if a CMake wrapper were involved, then this wouldn't work at all:

cmake -P elfconvert.cmake -DLMA_ADJUST=*+<value> -DLMA_ADJUST=!bss+<value> ...

One solution could be to port existing CMake wrappers to Python, and have argparse take care of "append" style arguments, but that's probably overkill.

Another solution could be to have separate flags for lma_adjust (number) and lma_adjust_sections (list). This would require translation on the part of a CMake wrapper for objcopy, which might be odd as well.

As it stands, I'm not sure how the current toolchain abstraction scheme should handle this feature.

But if a CMake wrapper were involved, then this wouldn't work at all:

you wouldn't believe the kind of tricks one can do in CMake 😉

$ cmake -DLMA_ADJUST="*+1024" -DLMA_ADJUST="!bss+2048" -P elfconvert.cmake Elfconvert lma adjustment: -DLMA_ADJUST=*+1024 Elfconvert lma adjustment: -DLMA_ADJUST=!bss+2048

Code:

cmake_minimum_required(VERSION 3.20) foreach(i RANGE ${CMAKE_ARGC}) if(CMAKE_ARGV${i} MATCHES "^-DLMA_ADJUST(.*)") message("Elfconvert lma adjustment: ${CMAKE_ARGV${i}}") endif() endforeach()

but as this requires special CMake skills, then it's harder for others to implement such code.

Note, it's generally required to have all -Ds before -P <script> when invoking CMake in script mode.

As it stands, I'm not sure how the current toolchain abstraction scheme should handle this feature.

yep, regardless of the path, it's gonna add even extra complexity on top of the complexity we already have.

tejlmand · 2024-01-24T10:34:37Z

cmake/bintools/gnu/target_bintools.cmake

@@ -41,7 +41,7 @@ set_property(TARGET bintools PROPERTY elfconvert_flag_section_remove "--remove-s
 set_property(TARGET bintools PROPERTY elfconvert_flag_section_only "--only-section=")
 set_property(TARGET bintools PROPERTY elfconvert_flag_section_rename "--rename-section;")

-set_property(TARGET bintools PROPERTY elfconvert_flag_lma_adjust "--change-section-lma;*+")
+set_property(TARGET bintools PROPERTY elfconvert_flag_lma_adjust "--change-section-lma;")


the old property allowed to pass just the value of the adjustment to the property, thus any other bintool implementation provided by the toolchain infrastructure would have the ability to just process the value, but now such value may suddenly contain section patterns and +, thus making it harder to process.

github-actions · 2024-03-31T00:29:42Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

carlescufi · 2024-07-10T13:17:53Z

@57300 will you resume work on this?

57300 · 2024-07-10T13:34:55Z

@57300 will you resume work on this?

I'm not sure how to proceed. @tejlmand had raised a few concerns and said "we need to be really careful", but there don't seem to be any blockers. If this solution is not accepted, the alternative might be to refactor the GNU abstraction, or the toolchain abstraction system as a whole, to accommodate this one feature.

carlescufi · 2024-07-10T13:43:50Z

@57300 will you resume work on this?

I'm not sure how to proceed. @tejlmand had raised a few concerns and said "we need to be really careful", but there don't seem to be any blockers. If this solution is not accepted, the alternative might be to refactor the GNU abstraction, or the toolchain abstraction system as a whole, to accommodate this one feature.

Understood, I will ask @tejlmand for additional input there then.

tejlmand

I'm not sure how to proceed. @tejlmand had raised a few concerns and said "we need to be really careful", but there don't seem to be any blockers.

only reason for no blockers is that no other toolchain than Zephyr / GNU (binutils) defines a value for elfconvert_flag_lma_adjust property:

zephyr/cmake/bintools/gnu/target_bintools.cmake

Line 47 in 1c689fc

    
           set_property(TARGET bintools PROPERTY elfconvert_flag_lma_adjust "--change-section-lma;*+")

This means that if trying to build a target which defines BUILD_OUTPUT_ADJUST_LMA to a value != 0, then such a toolchain would be given a number as argument today (and thus fail).

Thus, because the existing implementation is currently not expected to work with non-GNU toolchains then the changes in this PR is approved.

@57300 would be nice to get formatting fixed, see #68040 (comment)

57300 requested a review from tejlmand January 24, 2024 08:42

zephyrbot added area: Build System area: Toolchains Toolchains area: Kconfig labels Jan 24, 2024

zephyrbot requested review from jeremybettis, nashif, nordicjm and stephanosio January 24, 2024 08:43

zephyrbot assigned tejlmand Jan 24, 2024

tejlmand requested changes Jan 24, 2024

View reviewed changes

tejlmand requested review from abrodkin and evgeniy-paltsev January 24, 2024 11:02

github-actions bot added the Stale label Mar 31, 2024

github-actions bot closed this Apr 14, 2024

carlescufi removed the Stale label Jul 10, 2024

carlescufi reopened this Jul 10, 2024

tejlmand approved these changes Aug 12, 2024

View reviewed changes

carlescufi approved these changes Aug 12, 2024

View reviewed changes

carlescufi merged commit f2dc4a0 into zephyrproject-rtos:main Aug 12, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Adjust LMA for user-specified sections #68040

cmake: Adjust LMA for user-specified sections #68040

57300 commented Jan 24, 2024

tejlmand left a comment

tejlmand Jan 24, 2024

tejlmand Aug 12, 2024

tejlmand Jan 24, 2024

57300 Jan 26, 2024 •

edited

Loading

tejlmand Jan 30, 2024

tejlmand Aug 12, 2024

tejlmand Jan 24, 2024

57300 Jan 26, 2024

tejlmand Jan 30, 2024

tejlmand Jan 24, 2024

github-actions bot commented Mar 31, 2024

carlescufi commented Jul 10, 2024

57300 commented Jul 10, 2024

carlescufi commented Jul 10, 2024

tejlmand left a comment

	depends on BUILD_OUTPUT_ADJUST_LMA!=""
	depends on BUILD_OUTPUT_ADJUST_LMA != ""

cmake: Adjust LMA for user-specified sections #68040

cmake: Adjust LMA for user-specified sections #68040

Conversation

57300 commented Jan 24, 2024

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

57300 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 31, 2024

carlescufi commented Jul 10, 2024

57300 commented Jul 10, 2024

carlescufi commented Jul 10, 2024

tejlmand left a comment

Choose a reason for hiding this comment

57300 Jan 26, 2024 •

edited

Loading