Skip to content

cmake: Adjust LMA for user-specified sections #68040

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 12, 2024
Merged

cmake: Adjust LMA for user-specified sections #68040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

list(APPEND
post_build_commands
COMMAND $<TARGET_PROPERTY:bintools,elfconvert_command>
$<TARGET_PROPERTY:bintools,elfconvert_flag_final>
$<TARGET_PROPERTY:bintools,elfconvert_flag_lma_adjust>${adjustment}
${args_adjustment}
$<TARGET_PROPERTY:bintools,elfconvert_flag_infile>${KERNEL_ELF_NAME}
$<TARGET_PROPERTY:bintools,elfconvert_flag_outfile>${KERNEL_ELF_NAME}
)
Expand Down
17 changes: 17 additions & 0 deletions Kconfig.zephyr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!=""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
depends on BUILD_OUTPUT_ADJUST_LMA!=""
depends on BUILD_OUTPUT_ADJUST_LMA != ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment seems to have been overlooked.

help
This determines the output sections to which the above LMA adjustment
will be applied.
The value can be the name of a section in the final ELF, like "text".
It can also be a pattern with wildcards, such as "*bss", which could
match more than one section name. Multiple such patterns can be given
as a ";"-separated list. It's possible to supply a 'negative' pattern
starting with "!", to exclude sections matched by a preceding pattern.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@57300 57300 Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


config BUILD_OUTPUT_INFO_HEADER
bool "Create a image information header"
help
Expand Down
2 changes: 1 addition & 1 deletion cmake/bintools/gnu/target_bintools.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Note, placing a ';' at the end results in the following param to be a list,
# and hence space separated.
Expand Down