-
Notifications
You must be signed in to change notification settings - Fork 7.7k
cmake: Handle MPU_ALIGN in CONFIG_CMAKE_LINKER_GENERATOR #89387
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.
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
base: main
Are you sure you want to change the base?
cmake: Handle MPU_ALIGN in CONFIG_CMAKE_LINKER_GENERATOR #89387
Conversation
To allow funcitonallity like #define MPU_ALIGN(region_size) we need some more power from @variables@. This adds two things: * Recursive expansion of variables, if V1=foo and V2=@v1@ then @v2@ will expand to foo * @WITH_ARG,arg=foo@ defines arg=foo during the expansion of WITH_ARG. Together these two features allows one to do things similar to preprocessor macros with arguments, e.g. MPU_ALIGN(reg_size): zephyr_linker_include_var(VAR MPU_ALIGN VALUE "1 << LOG2CEIL(@rs@)") zephyr_linker_section(NAME foo ALIGN @MPU_ALIGN,rs=foo_end-foo_start@) Signed-off-by: Björn Bergman <bjorn.bergman@iar.com>
Make gen_app_partitions.py pass size argument to SMEM_PARTIOTION_ALIGN Implement MPU_ALIGN for CONFIG_CMAKE_LINKER_GENERATOR for cortex-m, and setup SMEM_PARTITION_ALIGN to use MPU_ALIGN. Following the pattern of how it is setup in the non-generator scrips. Signed-off-by: Björn Bergman <bjorn.bergman@iar.com>
828c08d
to
684d252
Compare
@@ -22,16 +22,15 @@ if((NOT DEFINED CONFIG_CUSTOM_SECTION_ALIGN) AND DEFINED CONFIG_MPU_REQUIRES_PO | |||
# . = ALIGN( 1 << LOG2CEIL(region_size)) | |||
# Handling this requires us to handle log2ceil() in iar linker since the size | |||
# isn't known until then. | |||
set(MPU_ALIGN_BYTES ${region_min_align}) | |||
zephyr_linker_include_var(VAR MPU_ALIGN VALUE "MAX(${region_min_align} , 1 << LOG2CEIL(@region_size@) )") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG2CEIL
is problematic because it's not universally supported across linkers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True,
This is problematic for iar ilink too, I have a "solution" for ilink that does the calculation of log2ceil between pre1 and final linking phase, via some creative use of the @variables@ and scripts (you can see the details at 4ceaa15 not done, please be polite).
We could of course add something similar for all linkers, but in the end the problem remains, what power we want to allow in the expression language. I think it makes sense to try to let ld be the role-model and as best it is possible cover up the shortcomings of the other linkers.
Also, it is easy, if not optimal, to have conditional things in the main cmake machinery for some of these corner cases (CONFIG_CUSTOM_SECTION_ALIGN).
This PR takes CMAKE_LINKER_GENERATOR && CONFIG_USERSPACE closer to working, at least for ld. If we decide to use my iar workaround for other linkers it makes sense to use it for ld too just to make things more similar. Lets tackle that fight when the time comes, knowing that my workaround is a possible solution if we dont find any thing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tejlmand Do you consider this a blocker for the PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that if we allow to enter ld functions here then we are on a path where things will be hard to handle.
If we need such functionality, then I think a dedicated CMake function is better, and then the linker generator implementation itself can decide how best to map the requested feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked at my IAR ILink solution ( 4ceaa15 ) ?
It expands e.g. LOG2CEIL with a special tag (@IAR_EVALUATE@ ), which the generator at each linking pass puts into a json list of required expressions. This becomes an extra output from the generator, and after linking the pass a script looks at the list of expressions and evaluates them, using the linking result to lookup symbol values. In the next linking pass the values can be used, at least if not too much has changed (which it hasn't between pre1 and final, for MPU_ALIGN).
This is reasonably well hidden behind @variables@ evaluation, so the impact on the generator itself is acceptable I think.
The python script it self needs some work to be "industrial strength", but I think the overall setup works.
It is rather cumbersome though, and quite hard to follow, and not what you usually expect to happen when "just linking" a project. But I see no other alternatives really, besides removing the problem altogether and disallow log2ceil for all and just force the user to set a kcnofig variable with the approriate alignment manually. It is easy enoguh to have a link time check that it follows the requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am heading out on vacation the next two weeks (until June 30th). I might check in here, but please be patient ;)
I would very much like input or suggestions on how to do this, and bridge the gap between linkers.
When I did this I thought it would be only ILink that needed the extra help with log2ceil and such...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked at my IAR ILink solution ( 4ceaa15 ) ?
yes I did, and especially I find constructs like this problematic:
zephyr/cmake/linker_script/arm/linker.cmake
Lines 26 to 33 in 4ceaa15
set(MPU_ALIGN "MAX(${region_min_align} , 2 << LOG2CEIL(@region_size@) )") | |
if(ZEPHYR_TOOLCHAIN_VARIANT STREQUAL "iar") | |
# Ilink needs help with the align-to-power-of-2-of-region-size thing. | |
# So tag it with IAR_EVALUATE to be handled by iar_linker_evaluate.py | |
# Note that we need to escape the enclosing @ to get the evaluation | |
# order right | |
set(MPU_ALIGN "@AT@IAR_EVALUATE,undef:0,expr=${MPU_ALIGN}@AT@") | |
endif() |
because if we allow ld functions to be written directly then we cannot expect an author to know all possible linkers out there, not to mention it becomes hard to have an out-of-tree linker generator implementation.
Also it allow code to enter and fail for other linkers, for example for IAR until @bjorniuppsala becomes aware of the extra case.
You may say that relying on a function also requires for someone writing the functionality, like in case above first author will likely add the GNU ld support. An oot linker can implement their own.
And you can implement the IAR variant when you have time.
Also it avoids this if
this toolchain testing everywhere, cause the function is just called, and toolchain provides the implementation (and with failure / unsupported error message if not ready yet.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I agree 100% that the ZEPHYR_TOOLCHAIN_VARIANT check is the wrong thing to have there.
What I meant was what do you think of the basic approach of catching these expressions, and using script to evaluate them and inject the result in the next linking pass ?
With the machinery I have proposed (recursive @variables@ and all that jazz..) we could define sections like
zephyr_linker_include_var(VAR MPU_ALIGN VALUE "@EVAL,undef:4,expr=MAX(${region_min_align} , 2 << LOG2CEIL(@region_size@)" )
zephyr_linker_section( ... ALIGN "@MPU_ALIGN,region_size=foo_end - foo_start@")
(With some trick/escape-syntax to tag foo_end and foo_start as linker symbols)
and then in e.g. linker/foo/target.cmake add a defintion of what EVAL does. (for ld we could just expand to @expr@, and for others make it go through the evaluation scripts).
# linker/ld/target.cmake
# Expand @EVAL,expr=... into the expr itself
zephyt_linker_include_var( VAR EVAL VALUE "@expr@")
# linker/iar/target.cmake
# Use the ruby script to evaluate/emulate ld-expressions for @EVAL,expr=...
zephyt_linker_include_var( VAR EVAL VALUE "@AT@EVAL_USING_SCRIPT,expr=@expr@@AT@")
(Do note that in the above expression ${region_min_align} refers to a mcake variable expanded at cmake-generation time, and @region_size@ refers to an "argument" to the expansion that is evaluated at linker file generation time. The mixing is somewhat confusing, and could easily be turned into linker-file-generation-time-only by having the region_min_align value be visible as a @variable@ too.)
When running the linker-file-generator for a pass, it processes all the EVAL_USING_SCRIPT tagged variable-expansions and stores them into a json blob. After linking the pass a script crunches the json and the linker output to find symbol values, and evaluates all the expressions, the successfully evaluated expressions are then fed into the next linker pass as specially named variables @EVALUATED_@ that are picked up as expansions to the @EVAL_USING_SCRIPT...@ stuff.
Except for the horrible syntax I think the principle is sound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think about it, maybe all of the posibly-eval-expression-arguments to zephyr_linker_* functions (e.g. ALIGN, SUBALIGN, MIN_SIZE,... ) should have an implicit @EVAL_USING_SCRIPT@ thing on them ? That way the syntax gets somewhat less clumsy.
Until now it has not been possible to handle MPU_ALIGN(region_size) entierly inside the CMAKE_LINKER_GENERATOR machinery, since it has been relying on the c-preprocessor definition. This does not scale for e.g. the iar-linker, which would require a different include-file duplicating the MPU_ALIGN definition. It also makes the logic hard to follow, since the info is spread out between the cmake-files, #include files, all brought guarded with #ifdefs. Doing it all in cmake allows us to collect all this at the same place.
This patch tries to fix that by extending the already existing @variables@ to handle arguments using the syntax @MPU_ALIGN,region_size=42@ which sets @region_size@ to 42 while expanding @MPU_ALIGN@. The syntax is choosen to not interfere with cmake...