LTO-compatible ISRs

[rakons]

A big rebuild of the zephyr interrupt system.

Things that was done:

• Rewrite the script for isr generation to simplify the current and future implementation of new parsers. Now most of the script functionality is placed inside classes.
• Extract everything that relates to current parser (that is creating arrays in C files) into separate file.
• Add new parser, that relays on creating the array entry directly in the source code, and then placing it by linker.

How it works:

• basically the entry is created in the C code, exactly how it should appear as an entry in hardware vector table, software interrupt table or shared interrupt vectors.
  • Note that this was required to change the order of parameter and function pointer in shared interrupt array entry, to match exactly the entry in software interrupt vectors.
• Now generator creates C file where it places the empty implementation of every irq, isr ans shared isr entry that is not implemented by the user code. Also everytime we are having multiple entries in swi it is treated as shared interrupt and the required entries are created.
• The ld file is generated that places everything where it needs to be.

NOTE:
The implementation is mostly functional, but there are still a few things to do:

• Shared interrupts implementation still needs some adjustments! The main issue now seems to be to properly align the structures created by linker.
• Software and shared interrupt handlers should be placed in proper section (some implementations expect to have a possibility to write to it). Right now it cannot work together with dynamic ISR allocation.
• (not really sure about this one - it seems ok to include the generated files in final step) The moment when the isr_tables.ld is included should be starting from second parsing phase (now it is included only for final build and it makes the code to relocate.
• Only ARM architectures are covered right now (note: reflected in Kconfig, more architectures can be added later).
• The way the isr_tables.ld is included right now is not accurate - the _vector_end symbol generated by the linker does not show the real end of the vectors.
• The whole code requires polishing - some lines are too long.

But anyway - it is good enough to be used as long as you are not using shared interrupts. The code relocation for secure build may fail also.

For the time being please treat it as a proof of concept that I wish to finalize.

To compile the code using new parser please add -DCONFIG_ISR_TABLES_LOCAL_DECLARATION=y parameter to the build. For example, from samples/hello_world directory:

west build -p -b qemu_cortex_m0 -- -DCONFIG_ISR_TABLES_LOCAL_DECLARATION=y

Why do we need this change

Currently for the ISR handlers generation, before zephyr_final stage (in single or in both pre linking stages), the gen_isr_tables.py script is used (zephyr/scripts/build/gen_isr_tables.py) that is using the data from “.isrList” sections and creates required interrupt tables into [build]/zephyr/isr_tables.c that usually looks similar to the code below:

typedef void (* ISR)(const void *);
uintptr_t __irq_vector_table _irq_vector_table[48] = {
        ((uintptr_t)&_isr_wrapper),
        ((uintptr_t)&_isr_wrapper),
        ...
};
struct _isr_table_entry __sw_isr_table _sw_isr_table[48] = {
        {(const void *)0x2d81, (ISR)0x4e7d},
        {(const void *)0x0, (ISR)((uintptr_t)&z_irq_spurious)},
        {(const void *)0x535c, (ISR)0x4dcb},
        {(const void *)0x0, (ISR)((uintptr_t)&z_irq_spurious)},
        ...
};

Then the generated isr_tables.c file is compiled and linked together with the rest of the code to get zephyr_final binaries.

The main problem with the approach above is that the creates arrays is composed mainly from the direct addresses of the interrupt functions. Also the argument for software interrupt table is provided as a number that may be any type - it might be and address of IO register or may be pointer to the function or address of some object in the code. This makes it very hard to translate it into usable handler to let the linker the knowledge what object is really used.

As during linking with LTO enabled the addresses of the objects may change this approach is unusable in such configuration.

This does not mean that using LTO directly with current solution would not work. It is just risky. It means - it might work, but small change in the code may break it.

Solid solution does not lost information during compilation - in this case, such lost information is the connection between objects. Changing the object into a raw address makes linker not conciseness about the connection. The goal of the proposed solution is to preserve this information during the whole process of generation of the binary file.

[rakons]

Added alignments to generated linker script.

[rakons]

@dkalowsk - I hope it would make you interested. This PR is planed to solve (at least part) of the issues that appear during yours PR: #49553

[dkalowsk]

@dkalowsk - I hope it would make you interested. This PR is planed to solve (at least part) of the issues that appear during yours PR: #49553

thanks @rakons for the effort here. I'll be a bit slow to review just due to outside constraints. First brief look this appears to be going in the right direction, so thanks again for the effort here!

[rakons]

Currently I am including zephyr/isr_tables.ld when we are on final build step. This is not correct. I suppose I should include in any step beyond pre0.
And now how to do it? I did a simple test:

diff --git a/cmake/linker/ld/target.cmake b/cmake/linker/ld/target.cmake
index b57f890204..96dc98416b 100644
--- a/cmake/linker/ld/target.cmake
+++ b/cmake/linker/ld/target.cmake
@@ -70,6 +70,8 @@ macro(configure_linker_script linker_script_gen linker_pass_define)
       ${extra_dependencies}
       # NB: 'linker_script_dep' will use a keyword that ends 'DEPENDS'
       ${linker_script_dep}
+      COMMAND echo "+++++++++++++++++++++++++++"
+      COMMAND echo "${linker_script_gen}: \"${template_script_defines}\""
       COMMAND ${CMAKE_C_COMPILER}
       -x assembler-with-cpp
       ${NOSYSDEF_CFLAG}

And having 3 passes I get:

[52/123] Generating linker_zephyr_pre1.cmd
+++++++++++++++++++++++++++
linker_zephyr_pre1.cmd: "-DLINKER_ZEPHYR_PREBUILT"
[57/123] Generating linker_zephyr_pre0.cmd
+++++++++++++++++++++++++++
linker_zephyr_pre0.cmd: "-DLINKER_DEVICE_DEPS_PASS1"
[117/123] Generating linker.cmd
+++++++++++++++++++++++++++
linker.cmd: "-DLINKER_ZEPHYR_FINAL"

Then having 2 passes I get:

+++++++++++++++++++++++++++
linker_zephyr_pre0.cmd: "-DLINKER_ZEPHYR_PREBUILT"
[112/117] Generating linker.cmd
+++++++++++++++++++++++++++
linker.cmd: "-DLINKER_ZEPHYR_FINAL"

What surprises me is the LINKER_DEVICE_DEPS_PASS1 definition for pre0 file. Is that correct? How should I handle it to include the required file in compiler command script excluding pre0?

[rakons]

Last push - split the generated linker files to hardware vectors and swi (+shared) related parts. Include it in the right (?) places in the build system.
Remove the PROVIDE from z_shared_sw_isr_table in generated file - we do not want it to be overwritten.

[rakons]

Script errors fixes

[rakons]

Another fix of the script errors

[rakons]

Rebase (with a lot of manual work).

[rakons]

Fix code issues reported during Compliance checks

[carlescufi]

@rakons would you mind adding a section in the PR description explaining why this rework is required for LTO? Essentially a problem statement.

[rakons]

Fixed the issue of data alignment in intList section when 64bit architecture is selected without optimization.

[rakons]

Last commit makes it even worse - using file name and the line number would not work as expected for the interrupts that are connected by DT_INST_FOREACH_STATUS_OKAY - and we are doing it all around the code.

[rakons]

Now it seems that I am still experiencing an issue that there appear the properly placed interrupt vectors from sections that we required, but also - the vectors from arch/common/isr_tables.c - that is reported as an error on some compilations as the space for interrupt vectors is overflowed.

[rakons]

Rebase - west.yml no longer needs to be updated as now current hal_nxp contains the required commit.

[rakons]

@microbuilder

Can you address the compliance checks?

The compliance check, the one error here is false positive. Too much similar lines in two parsers.
I believe it was mentioned on Discord channel by @carlescufi

[andyross]

Still gives me the ick to be rushing on this, but I can't see a reason not to merge. Holding my breath and clicking +1.

[henrikbrixandersen]

Git bisect for the breakage on main points to this PR: #68508

[microbuilder]

@rakons This has already been merged, but please raise a separate PR clearly describing these changes in the release notes. It's an important change and needs highlighting.

[marc-hb]

This breaks old compilers:

xt-xcc ERROR parsing -fno-lto:  unknown flag

@tejlmand how can we test this flag first, any good example to follow?

[marc-hb]

Tentative fix submitted in #69058

[go2sh]

Hi @rakons,
the CONFIG_IRQ_VECTOR_TABLE_JUMP_BY_CODE config is only working if the compiler supports gnu extension. Its not allowed to define a function inside a function in plain. I use a complete asm based approach by changing the section before and after the jump code. That works and is still lto capable.
BR,
go2sh