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: check_init_priorities: use the zephyr executable file [WAS: scripts: add dump_init_order.py] #62459

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

fabiobaltieri
Copy link
Member

@fabiobaltieri fabiobaltieri commented Sep 8, 2023

Hi, this is a rework of device discovery code of check_init_priorities. Instead of finding all the individual object files and find the level+piority from the section name, parse back the initlevel section from the output elf file. This means that the script is not impacted by stale files in the build directory, but more importantly it makes it work with LTO builds, where somehow the intermediate object fiels sections are completely mangled up.

While doing this, add a feature to print a human readable format of the initialization sections. This is pretty much just a friendlier version of arm-zephyr-eabi-objdump -j initlevel -j device_area -d build/zephyr/zephyr.elf. I'm using it to troubleshoot SYS_INIT order breakages, though I'm thinking one may use it in CI to compare the known sequence order of a build and catch unintended changes.

Looks something like this:

$ west build -t initlevels -p -b nrf52840dk_nrf52840 samples/drivers/led_pwm
...
EARLY
PRE_KERNEL_1
  __init_nordicsemi_nrf52_init: nordicsemi_nrf52_init(NULL)
  __init___device_dts_ord_74: clk_init(__device_dts_ord_74)
  __init_rtt_init: rtt_init(NULL)
  __init___device_dts_ord_11: gpio_nrfx_init(__device_dts_ord_11)
  __init___device_dts_ord_104: gpio_nrfx_init(__device_dts_ord_104)
  __init___device_dts_ord_112: uarte_0_init(__device_dts_ord_112)
  __init_uart_console_init: uart_console_init(NULL)
PRE_KERNEL_2
  __init_sys_clock_driver_init: sys_clock_driver_init(NULL)
POST_KERNEL
  __init_enable_logger: enable_logger(NULL)
  __init_malloc_prepare: malloc_prepare(NULL)
  __init___device_dts_ord_69: pwm_nrfx_init(__device_dts_ord_69)
  __init___device_dts_ord_19: led_gpio_init(__device_dts_ord_19)
  __init___device_dts_ord_68: led_pwm_init(__device_dts_ord_68)
APPLICATION
SMP

Very much inspired by a comment from @bjarki-trackunit on the original proposal, should be fairly easy to adapt to #61986.

Also pushed on https://github.com/zephyrproject-rtos/zephyr-testing/tree/vfabio-testing-branch

Since this now needs to work on a linked library I had to tweak the CMakeFiles a bit, and also it turns out this does not quite work with the elf file when using NATIVE_LIBRARY, although it does on the final exe. I could hack around it but I'd rather turn it off in that case right now and look for a proper solution down the road.

cfriedt
cfriedt previously approved these changes Sep 9, 2023
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Love it :)

@kartben
Copy link
Collaborator

kartben commented Sep 9, 2023

Very cool! Should this be added as a west target for convenience/ better discoverability?

@fabiobaltieri
Copy link
Member Author

Thanks for the feedback folks, I thought more about this and realized that the information here works for check_init_priorities.py, and does not require the hack for detecting sub builds plus it works with LTO files (which turns out the corrent code does not).

So I'm taking a 180 on this one, going to rip the file scanning code off check_init_priorities.py, replace it with this one and I'll integrate the functionality of this check_init_priorities.py itself.

@kartben yes will look into adding a target

@fabiobaltieri fabiobaltieri marked this pull request as draft September 11, 2023 09:17
@fabiobaltieri fabiobaltieri force-pushed the dump-init-order branch 2 times, most recently from 0d03b81 to b2f81d2 Compare September 11, 2023 17:42
@fabiobaltieri fabiobaltieri changed the title scripts: add dump_init_order.py scripts: check_init_priorities: use the zephry executable file [WAS: scripts: add dump_init_order.py] Sep 11, 2023
@fabiobaltieri fabiobaltieri force-pushed the dump-init-order branch 3 times, most recently from 703e7b5 to c78a7e7 Compare September 11, 2023 21:35
@fabiobaltieri fabiobaltieri marked this pull request as ready for review September 12, 2023 08:06
@fabiobaltieri fabiobaltieri force-pushed the dump-init-order branch 2 times, most recently from 9367106 to fdbea03 Compare September 14, 2023 15:41
@fabiobaltieri
Copy link
Member Author

Ok small update now that I managed to get my hands on an armclang file, there's two problems:

1 - the init data is spread into multiple sections, looks something like:

        0x000021d8 - 0x00002200 is init_103
        0x00002200 - 0x00002208 is init_104
        0x00002208 - 0x00002208 is init_end

2 - the symbols for the init section boundaries are not there, looks like the linker resolved and ate them up

So [1] is easy to fix and I've actually decided to integrate the change regardless: I've just changed _initlevel_pointer to resolve the section for every pointer individually, which is actually nice regardless cause it means the script does not need to look for a specific section name to find the data.

[2] is a more of a problem though, if the armclang linker cannot be configured to omit those symbols (some debug option?) I think I'll have to either guess the symbols from those init_103 strings (is that dependable?!) or just disable this for armclang entirely.

Waiting for further feedback from @tejlmand.

@fabiobaltieri
Copy link
Member Author

        0x000021d8 - 0x00002200 is init_103
        0x00002200 - 0x00002208 is init_104
        0x00002208 - 0x00002208 is init_end

Wonder how the linker ended up with those now that I looked into it, from the content these seems to be 103: PRE_KERNEL_1 and 104: PRE_KERNEL_2.

Was trying to correlate those in the code but maybe it's better to just shut this off for armclang for the moment. Waiting to hear if it works for ARCMWDT at least.

Rework check_init_priorities to use the main executable file instead of
the individual object files for discovering the devices.

This should make the script more robust in case of stale files in the
build directory, and also makes it work with LTO object files.

Additionally, keep track of the detected init calls, and add a handy
"-i" option to produce a human readable print of the initcalls in the
call sequence, which can be useful for debugging initialization problems
due to odd SYS_INIT and DEVICE interactions.

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
Add a new "initlevels" target that can be used to print a human readable
version of the init sequence by running: west build -t initlevels.

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
@fabiobaltieri
Copy link
Member Author

Added a conditional to skip this for armclang binaries, no point blocking on that, it can be fixed down the road by someone that have access to that compiler.

gmarull
gmarull previously approved these changes Sep 18, 2023
Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev left a comment

Choose a reason for hiding this comment

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

Tested with MWDT toolchain - seems to be working fine.

@fabiobaltieri
Copy link
Member Author

@evgeniy-paltsev thanks for testing this

@tejlmand can we go ahead with this one excluded on armclang for now? Then you folks with access to that compiler can look into this whenever you have time

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

looks good, just a couple of minor nits to get cleaned out before final +1.

CMakeLists.txt Outdated Show resolved Hide resolved
Kconfig.zephyr Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Kconfig.zephyr Outdated Show resolved Hide resolved
The script does not play well with armclang binariest at the moment.
Disable the automatic invocation when running with armclang, at least
until this is investigated and fixed.

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
@fabiobaltieri
Copy link
Member Author

Reworked the "no armclang support" patch, it now works by:

  • not defining the initlevels target
  • adding a Kconfig dep instead

${fail_on_warning}
)
endif()

if(NOT CMAKE_C_COMPILER_ID STREQUAL "ARMClang")
Copy link
Collaborator

Choose a reason for hiding this comment

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

not fond, of it, but I see your reasoning here.
#62459 (comment)

and users could want to run the target even with the kconfig to n.
I don't think this specific feature is strong enough to have something along TOOLCHAIN_CREATES_GOOD_ELF or similar.

So accepted for now, and let's see if we can find a way with armclang to support the check init feature.

Copy link
Member Author

@fabiobaltieri fabiobaltieri Sep 20, 2023

Choose a reason for hiding this comment

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

Yeah, I'm sure it can be fixed, those stripping options in cmake/bintools/armclang/elfconvert_command.cmake looks suspicious to me. It'd be cool to figure a way of having a CI run with these compilers so we don't depend on individuals to test stuff though.

I'll file an issue about it, it's worth discussing.

@fabiobaltieri fabiobaltieri merged commit 4a556a9 into zephyrproject-rtos:main Sep 20, 2023
20 checks passed
@fabiobaltieri fabiobaltieri deleted the dump-init-order branch September 20, 2023 19:24
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.