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

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

Closed
mbolivar-nordic opened this issue Sep 24, 2021 · 11 comments
Closed
Labels
area: Build System area: Userspace Userspace Enhancement Changes/Updates/Additions to existing features

Comments

@mbolivar-nordic
Copy link
Contributor

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 #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
Copy link
Member

nashif commented Sep 29, 2021

@dcpleung FYI

@JordanYates
Copy link
Collaborator

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
Copy link
Member

@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
Copy link
Contributor Author

I tend to agree with @JordanYates also

@dcpleung
Copy link
Member

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

@ceolin
Copy link
Member

ceolin commented Oct 15, 2021

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
Copy link
Member

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
Copy link
Collaborator

marc-hb commented Oct 18, 2021

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: mesonbuild/meson#6070

My 2 cents.

@JordanYates
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member

ceolin commented Oct 21, 2021

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.

tejlmand pushed a commit to tejlmand/zephyr that referenced this issue Oct 29, 2021
Introduce a new global property for source files that are generated from
the initial Zephyr link (app_smem_unaligned_prebuilt). This source list
is used for files which will introduce address shifts into the final
binary, which need to be present in `zephyr_prebuilt.elf` for
`CONFIG_USERSPACE` scripts to correctly generate `zephyr.elf`.

This resolves zephyrproject-rtos#38836.

Signed-off-by: Jordan Yates <[email protected]>
tejlmand added a commit to tejlmand/zephyr that referenced this issue Oct 29, 2021
cmake: rework linker script generation and linker prebuilt stages

This commit reworks the linker script generation and linking stages in
order to better support fixed section location as required by zephyrproject-rtos#38836.

Today we have the following generated linker scripts and the elf output
depending on the system configuration:
- linker_app_smem_unaligned.cmd --> app_smem_unaligned_prebuilt.elf
- linker_zephyr_prebuilt.cmd    --> zephyr_prebuilt.elf
- linker.cmd                    --> zephyr.elf

as not all linker scripts may be created and as there is a need for the
possibility to move gen handles earlier then those stages has been
renamed into more generic names so that with this commit we have:
- linker_zephyr_pre0.cmd --> zephyr_pre0.elf
- linker_zephyr_pre1.cmd --> zephyr_pre1.elf
- linker.cmd             --> zephyr.elf

This also means that is the stage zephyr_pre1 is not needed, then build
can go from `zephyr_pre0i.elf` to `zephyr.elf`.

To keep the current behavior of generating the isr table of what was
`zephyr_prebuilt` stage the code block contolling isr generation has
been relocated to be located after app_smem and device handle
generation.

Signed-off-by: Torsten Rasmussen <[email protected]>
tejlmand pushed a commit to tejlmand/zephyr that referenced this issue Oct 29, 2021
This commit reworks the linker script generation and linking stages in
order to better support fixed section location as required by zephyrproject-rtos#38836.

Today we have the following generated linker scripts and the elf output
depending on the system configuration:
- linker_app_smem_unaligned.cmd --> app_smem_unaligned_prebuilt.elf
- linker_zephyr_prebuilt.cmd    --> zephyr_prebuilt.elf
- linker.cmd                    --> zephyr.elf

as not all linker scripts may be created and as there is a need for the
possibility to move gen handles earlier then those stages has been
renamed into more generic names so that with this commit we have:
- linker_zephyr_pre0.cmd --> zephyr_pre0.elf
- linker_zephyr_pre1.cmd --> zephyr_pre1.elf
- linker.cmd             --> zephyr.elf

This also means that is the stage zephyr_pre1 is not needed, then build
can go from `zephyr_pre0.elf` to `zephyr.elf`.

The gen_handles.py has been changed so it now uses `zephyr_pre0.elf` as input.
This ensures that the handles array are final when invoking the next build and
linking stages.

To keep the current behavior of generating the isr table of what was
`zephyr_prebuilt` stage the code block contolling isr generation has
been relocated to be located after app_smem and device handle
generation.

Signed-off-by: Jordan Yates <[email protected]>
Signed-off-by: Torsten Rasmussen <[email protected]>
tejlmand pushed a commit to tejlmand/zephyr that referenced this issue Nov 8, 2021
Introduce a new global property for source files that are generated from
the initial Zephyr link (app_smem_unaligned_prebuilt). This source list
is used for files which will introduce address shifts into the final
binary, which need to be present in `zephyr_prebuilt.elf` for
`CONFIG_USERSPACE` scripts to correctly generate `zephyr.elf`.

This resolves zephyrproject-rtos#38836.

Signed-off-by: Jordan Yates <[email protected]>
tejlmand pushed a commit to tejlmand/zephyr that referenced this issue Nov 9, 2021
This commit reworks the linker script generation and linking stages in
order to better support fixed section location as required by zephyrproject-rtos#38836.

Today we have the following generated linker scripts and the elf output
depending on the system configuration:
- linker_app_smem_unaligned.cmd --> app_smem_unaligned_prebuilt.elf
- linker_zephyr_prebuilt.cmd    --> zephyr_prebuilt.elf
- linker.cmd                    --> zephyr.elf

as not all linker scripts may be created and as there is a need for the
possibility to move gen handles earlier then those stages has been
renamed into more generic names so that with this commit we have:
- linker_zephyr_pre0.cmd --> zephyr_pre0.elf
- linker_zephyr_pre1.cmd --> zephyr_pre1.elf
- linker.cmd             --> zephyr.elf

This also means that is the stage zephyr_pre1 is not needed, then build
can go from `zephyr_pre0.elf` to `zephyr.elf`.

The gen_handles.py has been changed so it now uses `zephyr_pre0.elf` as
input. This ensures that the handles array are final when invoking the
next build and linking stages.

To keep the current behavior of generating the isr table and kobj hash
of what was `zephyr_prebuilt` stage the code blocks contolling isr
generation and kobj hash has been relocated to be located after
app_smem and device handle generation.

Signed-off-by: Jordan Yates <[email protected]>
Signed-off-by: Torsten Rasmussen <[email protected]>
carlescufi pushed a commit that referenced this issue Nov 16, 2021
This commit reworks the linker script generation and linking stages in
order to better support fixed section location as required by #38836.

Today we have the following generated linker scripts and the elf output
depending on the system configuration:
- linker_app_smem_unaligned.cmd --> app_smem_unaligned_prebuilt.elf
- linker_zephyr_prebuilt.cmd    --> zephyr_prebuilt.elf
- linker.cmd                    --> zephyr.elf

as not all linker scripts may be created and as there is a need for the
possibility to move gen handles earlier then those stages has been
renamed into more generic names so that with this commit we have:
- linker_zephyr_pre0.cmd --> zephyr_pre0.elf
- linker_zephyr_pre1.cmd --> zephyr_pre1.elf
- linker.cmd             --> zephyr.elf

This also means that is the stage zephyr_pre1 is not needed, then build
can go from `zephyr_pre0.elf` to `zephyr.elf`.

The gen_handles.py has been changed so it now uses `zephyr_pre0.elf` as
input. This ensures that the handles array are final when invoking the
next build and linking stages.

To keep the current behavior of generating the isr table and kobj hash
of what was `zephyr_prebuilt` stage the code blocks contolling isr
generation and kobj hash has been relocated to be located after
app_smem and device handle generation.

Signed-off-by: Jordan Yates <[email protected]>
Signed-off-by: Torsten Rasmussen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Userspace Userspace Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

7 participants