zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.99k stars 6.69k forks source link

Build system should allow some resizing between linker passes 1 and 2 #38836

Closed mbolivar-nordic closed 3 years ago

mbolivar-nordic commented 3 years ago

Is your enhancement proposal related to a problem? Please describe.

This is meant to track the 'real fix' to follow along the band-aid in #38834.

The userspace implementation builds a hash table using kernel object addresses as keys. These addresses must therefore remain valid between linker passes. Up until now, the solution to meeting this requirement has been that we cannot resize any values between passes 1 and 2.

However, this is causing pain and bugs. The pain is that unnecessary ROM is wasted by gen_handles.py (see #38834 and associated commit logs for more details) -- even when CONFIG_USERSPACE=n. An example bug is that merging #37836 led to #38181 because we ran out of room in an array that logically should have been resized between passes. In general, this is a restriction that is really starting to chafe where device handles are concerned.

Describe the solution you'd like

After discussing a bit with @tejlmand, it seems potentially possible to meet the userspace requirements and be able to resize our handles arrays if the linker scripts only ever allocate device handle arrays after all of the statically allocated kobjects. Such kobjects could be placed by their allocation macros into locations that come earlier in the image's ROM than the memory for device handle arrays. In this way, even if the handle arrays are resized, the addresses of the kernel objects should remain the same after the second pass link.

Describe alternatives you've considered

There is a wider-ranging RFC in https://github.com/zephyrproject-rtos/zephyr/issues/32129. That seems like potential overkill given that nothing else currently really "needs" to be resized between linker passes, assuming there's no showstopper associated with this issue.

nashif commented 3 years ago

@dcpleung FYI

JordanYates commented 3 years ago

I think the solution involving manually placing objects in ROM is unnecessarily fragile, but I also see the point that #32129 is overkill.

I am thinking of one additional linker step/elf file which is explicitly for scripts that require and produce size invariance.

zephyr_prebuilt.elf -> zephyr_fixed_layout.elf -> zephyr.elf

Where scripts such as gen_handles.py run on zephyr_prebuilt.elf, while gen_kobject_list.py and friends run on zephyr_fixed_layout.elf.

carlescufi commented 3 years ago

@JordanYates

I am thinking of one additional linker step/elf file which is explicitly for scripts that require and produce size invariance.

I really like this idea even though there's a risk that it might not be enough for the future. Even then we could fall back to a generic implementation like the one described in #32129.

mbolivar-nordic commented 3 years ago

I tend to agree with @JordanYates also

dcpleung commented 3 years ago

I think the current zephyr_prebuilt.elf is serving this purpose?

ceolin commented 3 years ago

I think the current zephyr_prebuilt.elf is serving this purpose?

the problem is that both gen_kobject_list.py and gen_handles.py operate in this stage and the latter should be able to resizing some things, which consequently breaks the former :/ And additional step is needed, or properly put the kobjects before the objects that need to be resized, but I also think that this is fragile.

dcpleung commented 3 years ago

The changing size is due to gperf generated hash table. This can be avoided if we can find a way around that, or a way of not using gperf while maintaining performance.

marc-hb commented 3 years ago

Whatever happens here, please never change any zephyr_something intermediate file "in-place". Mutable artefacts make troubleshooting build issues much more difficult and incremental builds impossible. Other potential issues: https://github.com/mesonbuild/meson/issues/6070

My 2 cents.

JordanYates commented 3 years ago

Whatever happens here, please never change any zephyr_something intermediate file "in-place".

Totally agree with this. It doesn't currently happen and I can't see any need for it to solve this issue.

JordanYates commented 3 years ago

I had a bit of a discussion with @carlescufi and worked out that Zephyr actually already uses 3 linker stages when CONFIG_USERSPACE is enabled (app_smem_unaligned_prebuilt.elf). So another way of going about this (instead of an additional stage between zephyr_prebuilt.elf and zephyr.elf) is to generate app_smem_unaligned_prebuilt.elf under additional conditions (CONFIG_HAS_DTS). gen_handles.py can then run on app_smem_unaligned_prebuilt.elf, generating the correct arrays in time for zephyr_prebuilt.elf.

ceolin commented 3 years ago

I had a bit of a discussion with @carlescufi and worked out that Zephyr actually already uses 3 linker stages when CONFIG_USERSPACE is enabled (app_smem_unaligned_prebuilt.elf). So another way of going about this (instead of an additional stage between zephyr_prebuilt.elf and zephyr.elf) is to generate app_smem_unaligned_prebuilt.elf under additional conditions (CONFIG_HAS_DTS). gen_handles.py can then run on app_smem_unaligned_prebuilt.elf, generating the correct arrays in time for zephyr_prebuilt.elf.

I may be wrong, but I don't think that is enough. gen_kobjects runs twice, first on app_smem_unaligned_prebuilt.elf to allocate a space for the hash table and and later on zephyr-prebuilt.elf with the final objects in the proper address. So, we may need to have to use gen_handles on app_smem_unaligned_prebuilt.elf then generate a new image that will be use by the first gen_kbojects run.