Skip to content

Conversation

@kokas-a
Copy link

@kokas-a kokas-a commented Aug 30, 2023

#58388 breaks ARCMWDT building.

ARCMWDT linker generates extended section names
(ex.: .rela.z_init_POST_KERNEL40_0_.__init_k_sys_work_q_init).

Default script "build/check_init_priorities.py" is unable to parse such format. This fixup adds limitation for reading only 2 parameters from priority string.

@kokas-a kokas-a marked this pull request as ready for review August 30, 2023 17:38
@kokas-a
Copy link
Author

kokas-a commented Aug 30, 2023

@evgeniy-paltsev , could you also take a look?

abrodkin
abrodkin previously approved these changes Aug 31, 2023
@evgeniy-paltsev evgeniy-paltsev added the area: ARC ARC Architecture label Aug 31, 2023
@carlescufi carlescufi requested a review from gmarull September 1, 2023 07:53
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

first question is why ARCMWDT linker is behaving differently, can you please explain?

@kokas-a
Copy link
Author

kokas-a commented Sep 1, 2023

first question is why ARCMWDT linker is behaving differently, can you please explain?

MWDT Toolchain is not GCC-based toolchain. Even is has good compatibility in many aspects, it also has some differences.
Such type of names generation is linker peculiarity and it's impossible to tune it. We have faced with such behavior earlier and some of linker scripts has already take it in account (ex: * wildcards at the end of sections name, i.e. in https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/linker/intlist.ld#L37).

@evgeniy-paltsev
Copy link
Contributor

@gmarull do you have some other questions about this PR? If no, could you please approve?

Thanks!

tejlmand
tejlmand previously approved these changes Sep 11, 2023
@kokas-a
Copy link
Author

kokas-a commented Sep 18, 2023

Hi @gmarull,

Could you please revisit this PR?

Comment on lines 81 to 85
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be much safer/readable to move to regexes instead.

import re
...
# before checking each level
expr = re.compile(rf".*{level}(\d+)_(\d+).*")
...
# check if match
m = expr.match(name)
if not m:
    continue

priority, sub_priority = m.groups()

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@kokas-a I have a PR in flight to rework this logic to not use the object files at all, so it would not rely on the format of the object sections anymore and just needs the boundary symbols in the final elf file, which should be a more robust solution (turns out the current one also breaks with GCC with LTO enabled). It deletes this code entirely, working on some kinks around armclang support but as long as the symbols are there it should work with this as well, could you give it a try? #62459

I'm also open to the idea of only running this for GCC and clang, there's only that much we can do about chasing quirks with non generally available compilers.

Copy link
Author

Choose a reason for hiding this comment

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

@fabiobaltieri , I have finally try your changes with WMDT compiler. They work fine!

Copy link
Member

Choose a reason for hiding this comment

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

brilliant, thanks a log, I'll nudge that one to get it unblocked then

zephyrproject-rtos#58388
breaks ARCMWDT building.

ARCMWDT linker generates extended section names
(ex.: .rela.z_init_POST_KERNEL40_0_.__init_k_sys_work_q_init).

Default script "build/check_init_priorities.py" is unable to parse
such format. This fixup adds limitation for reading only 2 parameters from
priority string.

Signed-off-by: Nikolay Agishev <[email protected]>
@kokas-a kokas-a dismissed stale reviews from tejlmand, abrodkin, and evgeniy-paltsev via dbec70d September 18, 2023 17:05
@kokas-a kokas-a force-pushed the fix_py_script_for_mwdt2 branch from 15fec74 to dbec70d Compare September 18, 2023 17:05
@fabiobaltieri
Copy link
Member

@kokas-a could you try if #62459 works for ARCMWDT? I'm fine with fixing this in the meantime but if that works it'd save me a merge conflict and you testing it again.

Comment on lines +83 to +86
# ARCMWDT linker generates extended section names
# (ex.: .rela.z_init_POST_KERNEL40_0_.__init_k_sys_work_q_init).
# limitation is required for reading only 2 parameters from
# priority string.
Copy link
Member

Choose a reason for hiding this comment

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

this comment is now redundant, also please check @fabiobaltieri PR, because these changes may not be needed at all

Copy link
Author

@kokas-a kokas-a Sep 19, 2023

Choose a reason for hiding this comment

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

@fabiobaltieri, @gmarull , Suggested changes #62459 works fine for WMDT compiler. So if pointed PR is applied in nearest future there is no need in this one (#62459).

Copy link
Member

Choose a reason for hiding this comment

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

That's merged, hopefully we should be good to go

Sorry about the breakage again, thanks for you patience.

@gmarull gmarull closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants