Skip to content

Conversation

rettichschnidi
Copy link
Contributor

@rettichschnidi rettichschnidi commented Mar 15, 2024

This disables the LTO builds for platforms which are affected by build errors.

While not a solution for the underlying issue, those changes will allow unrelated PRs with changes in the kernel code to pass CI and get merged.

Hint: #70426 tries to resolve the same issue

@danieldegrasse
Copy link
Contributor

If we want to use this solution, we should disable builds based on if CONFIG_CODE_RELOCATION is selected- this is the actual underlying issue that is affecting these platforms, and could affect other ones as well

@rettichschnidi rettichschnidi force-pushed the gardena/rs/upstream/fix-kernel-tests-ci branch from a35d8e8 to bfdb7d9 Compare March 15, 2024 22:29
@rettichschnidi
Copy link
Contributor Author

If we want to use this solution, we should disable builds based on if CONFIG_CODE_RELOCATION is selected- this is the actual underlying issue that is affecting these platforms, and could affect other ones as well

Just updated the PR. I guess you meant CONFIG_CODE_DATA_RELOCATION.

@rettichschnidi rettichschnidi force-pushed the gardena/rs/upstream/fix-kernel-tests-ci branch from bfdb7d9 to e93fe82 Compare March 15, 2024 22:34
@rettichschnidi rettichschnidi marked this pull request as ready for review March 16, 2024 09:01
cfriedt
cfriedt previously approved these changes Mar 16, 2024
danieldegrasse
danieldegrasse previously approved these changes Mar 18, 2024
Copy link
Contributor

@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.

I think it's better to prevent LTO with Code relocation enabled.
At least until it has been ensured that those two features generally works together.

Thus instead of the changes proposed in this PR, then make an extra dependency here:

zephyr/Kconfig.zephyr

Lines 430 to 432 in 1408d1e

config LTO
bool "Link Time Optimization [EXPERIMENTAL]"
depends on (!(GEN_ISR_TABLES || GEN_IRQ_VECTOR_TABLE) || ISR_TABLES_LOCAL_DECLARATION) && !NATIVE_LIBRARY

like:

depends on (!(GEN_ISR_TABLES || GEN_IRQ_VECTOR_TABLE) || ISR_TABLES_LOCAL_DECLARATION) && !NATIVE_LIBRARY && !CODE_DATA_RELOCATION

@rakons have you done any verification related to LTO and the code relocation feature ?

@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Mar 19, 2024

I think it's better to prevent LTO with Code relocation enabled.
At least until it has been ensured that those two features generally works together.

I created a (draft) PR with this idea: #70426

While it also resolves (works around) my issue of no longer being able to pass CI with unrelated code changes, I am not a fan of it as it produce a new warning:

warning: LTO (defined at Kconfig.zephyr:430) was assigned the value 'y' but got the value 'n'. Check these unsatisfied dependencies: (!CODE_DATA_RELOCATION) (=n)

@rakons
Copy link
Contributor

rakons commented Mar 19, 2024

I had stated already in some issue here that I am not expecting LTO work as expected with code relocation.


Ok - found it: #69730 (comment)
Sorry - I did not have a time to discover it. I also do not think that we are using it at Nordic - so it would be hard to find any board to test the issue. What do I need?

@danieldegrasse
Copy link
Contributor

I think it's better to prevent LTO with Code relocation enabled.
At least until it has been ensured that those two features generally works together.

@tejlmand Do you think it would be possible to exclude the files relocated with code relocation using the prohibit_lto compiler property, like it is done here?: https://github.com/zephyrproject-rtos/zephyr/blob/main/CMakeLists.txt#L815

I am not sure if we can, since the code relocation feature uses generator expressions heavily. I tried to implement this to fix #69730 but did not get very far.

@tejlmand
Copy link
Contributor

I think it's better to prevent LTO with Code relocation enabled.
At least until it has been ensured that those two features generally works together.

@tejlmand Do you think it would be possible to exclude the files relocated with code relocation using the prohibit_lto compiler property, like it is done here?: https://github.com/zephyrproject-rtos/zephyr/blob/main/CMakeLists.txt#L815

I am not sure if we can, since the code relocation feature uses generator expressions heavily. I tried to implement this to fix #69730 but did not get very far.

I believe it should be possible.
I'm occupied with HWMv2 atm and won't have time to look into the details.

Implementing it should not take long but testing / verifying that things indeed work as expected is where I believe most time will need to be spend.

@tejlmand
Copy link
Contributor

@rettichschnidi I believe a combination of this PR (which filters out tests that enables data relocation) with the proposal in #70426 to avoid enabling LTO together with relocation will be the optimal solution to this problem.

andyross
andyross previously approved these changes Mar 21, 2024
@rettichschnidi rettichschnidi force-pushed the gardena/rs/upstream/fix-kernel-tests-ci branch from ddf153d to 2866eb4 Compare March 21, 2024 23:48
As per issue zephyrproject-rtos#69730, building arch.shared_interrupt.lto and
kernel.common.lto for mimxrt685_evk or mimxrt595_evk/mimxrt595s/cm33 is
currently broken due to their usage of CONFIG_CODE_DATA_RELOCATION.

This commit disables LTO when CODE_DATA_RELOCATION is enabled, allowing
PRs with changes in the kernel code to pass the CI check.

On the down side, builds affected by this change produce a new warning:

> warning: LTO (defined at Kconfig.zephyr:430) was assigned the value
> 'y' but got the value 'n'. Check these unsatisfied dependencies:
> (!CODE_DATA_RELOCATION) (=n)

Signed-off-by: Reto Schneider <[email protected]>
Trying to enable LTO when CODE_DATA_RELOCATION is already enabled
produces a warning.

This commit prevents twister from building such combinations for
kernel.common.lto.

Signed-off-by: Reto Schneider <[email protected]>
Trying to enable LTO when CODE_DATA_RELOCATION is already enabled
produces a warning.

This commit prevents twister from building such combinations for
arch.shared_interrupt.lto.

Signed-off-by: Reto Schneider <[email protected]>
@nashif nashif merged commit 397b846 into zephyrproject-rtos:main Mar 22, 2024
@rettichschnidi rettichschnidi deleted the gardena/rs/upstream/fix-kernel-tests-ci branch March 22, 2024 23:11
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.

8 participants