Skip to content
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

scripts: code_relocate: support section filter #74402

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

schouleu
Copy link
Contributor

One might want to select the symbols to be relocated inside a file or a library.
To do this, one can use the FILTER argument of zephyr_code_relocate which
must contain a regular expression of the section names to be selected for relocation.

This pull-request covers the issue #64148

About the dependency on -ffunction-sections and -fdata-sections flags, those are enabled by default in arch/common//home/schouleur/GML/zephyr_upstream/zephyr/arch/common/CMakeLists.txt:37

In case the flags are not enabled, this feature can also be used to select some specific sections.

@MaureenHelm
Copy link
Member

@tejlmand can you take a look please?

Comment on lines 540 to 541
# Replace pipes used in symbol filter regular expression
input_rel_dict = input_rel_dict.replace("\\|", "##PIPE##")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the | is used widely in regex'es, so it's natural to support them directly, but going through this substitution seems less than optimal to me.

This PR #60999 changed the input relocation argument from being a string to be a file.

Being a file actually means that there is no reason for a single string because a file can be multi-line.
I suggest to make a #60999 follow-up commit which remove the use of | as separator and instead create a multi-line input file.

Doing so will free the use of | and thereby remove the need for special handling in the regex.

@@ -514,12 +520,13 @@ def parse_input_string(line):
phdr, rest = rest.split(':', 1)

# Split lists by semicolons, in part to support generator expressions
flag_list, file_list = (lst.split(';') for lst in rest.split(':', 1))
flag_list, file_list, symbol_filter = (lst.split(';') for lst in rest.split(':', 2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fails on windows, most likely because drive letters contains :.

c:/users/torsten/zephyr-sdk-0.16.8/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/../../../../arm-zephyr-eabi/bin/ld.bfd.exe: app/libapp.a(test_file1.c.obj): in function `code_relocation_test_function_in_sram2':
C:/projects/github/ncs/zephyr/tests/application_development/code_relocation/src/test_file1.c:80: undefined reference to `__sram2_data_reloc_start'
c:/users/torsten/zephyr-sdk-0.16.8/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/../../../../arm-zephyr-eabi/bin/ld.bfd.exe: C:/projects/github/ncs/zephyr/tests/application_development/code_relocation/src/test_file1.c:80: undefined reference to `__sram2_data_reloc_end'
....

Also note the existing warning in this script:

# Be careful when splitting by : to avoid breaking absolute paths on Windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tejlmand do you have a way to test on windows without windows?

Copy link
Collaborator

Choose a reason for hiding this comment

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

unfortunately not.

@carlescufi
Copy link
Member

@schouleu do you have plans to address @tejlmand's comments?

@schouleu
Copy link
Contributor Author

schouleu commented Sep 6, 2024

Hi @carlescufi
Yes, I don't have a lot of time right now but I'll implement @tejlmand's suggestions

Copy link

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.

@github-actions github-actions bot added the Stale label Nov 12, 2024
@github-actions github-actions bot closed this Nov 26, 2024
@carlescufi carlescufi reopened this Jan 9, 2025
@carlescufi
Copy link
Member

Hi @carlescufi Yes, I don't have a lot of time right now but I'll implement @tejlmand's suggestions

@schouleu it's been a while, would you mind if we take over the PR?

@github-actions github-actions bot removed the Stale label Jan 10, 2025
@carlescufi
Copy link
Member

@schouleu would you mind if we take over your PR as per the process described here:
https://docs.zephyrproject.org/latest/contribute/modifying_contributions.html

@schouleu
Copy link
Contributor Author

@schouleu would you mind if we take over your PR as per the process described here: https://docs.zephyrproject.org/latest/contribute/modifying_contributions.html

Sure, go ahead

@tejlmand tejlmand force-pushed the code_relocate_filter branch from 7f75913 to 0545d26 Compare February 12, 2025 21:47
@tejlmand tejlmand force-pushed the code_relocate_filter branch 2 times, most recently from 31cfd0d to b8e5f54 Compare February 12, 2025 22:09
@tejlmand
Copy link
Collaborator

tejlmand commented Feb 12, 2025

@schouleu would you mind if we take over your PR as per the process described here: https://docs.zephyrproject.org/latest/contribute/modifying_contributions.html

Sure, go ahead

PR rebased and updated in following areas:

tejlmand
tejlmand previously approved these changes Feb 12, 2025
pdgendt
pdgendt previously approved these changes Feb 13, 2025
@tejlmand tejlmand dismissed stale reviews from pdgendt and themself via 78e7519 February 13, 2025 10:34
@tejlmand tejlmand force-pushed the code_relocate_filter branch from b8e5f54 to 78e7519 Compare February 13, 2025 10:34
Copy link
Contributor

@57300 57300 left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, @schouleu and @tejlmand

@tejlmand, could you add a few tests to tests/application_development/code_relocation?

I haven't tested this PR iteration, but I'm noticing some oddities in the code.

scripts/build/gen_relocate_app.py Outdated Show resolved Hide resolved
scripts/build/gen_relocate_app.py Outdated Show resolved Hide resolved
scripts/build/gen_relocate_app.py Outdated Show resolved Hide resolved
scripts/build/gen_relocate_app.py Outdated Show resolved Hide resolved
scripts/build/gen_relocate_app.py Outdated Show resolved Hide resolved
@tejlmand
Copy link
Collaborator

@tejlmand, could you add a few tests to tests/application_development/code_relocation?

Extended the file2.c containing function_in_sram() to also include function_not_relocated(), and made a verification that function_in_sram() is actually being relocated to ram.
I discovered that current test on zephyr/main is actually not verifying the location of the function, meaning that even in the case that he function was not properly relocated then the test would still pass 😮 .

This is now fixed, together with the new, non-relocated, function.

tejlmand and others added 2 commits February 13, 2025 17:45
With code relocation directives passed to the gen_relocate_app.py script
using generated file, then each directive can be place on individual
line in the file and thus free up the `|` character as separator.

Furthermore, a multi-line file with each directive on separate line is
also more user-readable, making debugging easier.

Signed-off-by: Torsten Rasmussen <[email protected]>
One might want to select the symbols to be relocated inside a file or
a library. To do this, one can use the FILTER argument of
zephyr_code_relocate which must contain a regular expression of the
section names to be selected for relocation.

The test_function_in_sram2 test case in
`tests/application_development/code_relocation` has been updated to
verify that only one function `function_in_sram()` is relocated to ram
and that the function `function_not_relocated()` is not being relocated
when using relocation filter.

Signed-off-by: Sylvain Chouleur <[email protected]>
Signed-off-by: Torsten Rasmussen <[email protected]>
@tejlmand tejlmand force-pushed the code_relocate_filter branch from ebc6ae5 to 416e531 Compare February 13, 2025 16:46
@kartben kartben merged commit 4454734 into zephyrproject-rtos:main Feb 14, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants