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: Cleanup access to ".intList" section #60838

Closed
wants to merge 1 commit into from

Conversation

rakons
Copy link
Collaborator

@rakons rakons commented Jul 26, 2023

This is currently proof of concept and idea for discussion.
I was trying to remove the need for dummy IDT_LIST memory definition that provides some limits to the number of supported interrupts. As I am working on solution for interrupt system that would require more fields in _isr_list structure this requirement for dummy memory is even more limiting.

The idea here is to add (INFO) type for the ".intList" section. Such a section is not present in the output and I could not access it via objcopy. But the section is still in elf file - and it can be accessed via pyelftools - and this is how I had used it.

The additional "gain" here is one temporary file less in the build directory.

I have tested it on hello_world sample on nrf52840dk_nrf52840 target. For the full solution it would probably require more cleanups for rest of the linker files around the declaration of the ".intList" section. I wish to get some feedback before I start doing it. If you see any drawbacks for the proposed solution or would you welcome it in zephyr?

@carlocaione
Copy link
Collaborator

carlocaione commented Jul 28, 2023

The only concern I have (so far) is that the INFO output section type is there only for backward compatibility and my impression is that it could be removed or simply neglected soon.

Can you expand on:

provides some limits to the number of supported interrupts

?

@carlescufi
Copy link
Member

@dcpleung could you please take a look?

@rakons
Copy link
Collaborator Author

rakons commented Jul 31, 2023

@carlocaione - the ".intList" section is put into IDT_LIST memory section that has to be provided by chip vendor linker files. I can see that it used to be 2KB. Sure - we can increase it, but still would be some gimmick just to put a section here.
Every _isr_list occupies 16B on 32b architecture and 24B on 64bit architecture. At the beginning of the section is a header that takes 16B. It means that on 32b arch we can put here 127 entries, and on 64b - 83. Am I missing something? What would happen if we overflow this IDT_LIST memory section?

@rakons
Copy link
Collaborator Author

rakons commented Jul 31, 2023

@carlocaione - Regarding to INFO section - I also saw it, but I also saw it is doing what I am expecting. This is also something we should discuss - how to reach the same result with the usage of INFO section. I will be probably able to look into it deeper in the next week.

@dcpleung
Copy link
Member

Looks reasonable.

@carlocaione
Copy link
Collaborator

@carlocaione - the ".intList" section is put into IDT_LIST memory section that has to be provided by chip vendor linker files. I can see that it used to be 2KB. Sure - we can increase it, but still would be some gimmick just to put a section here.

sorry, I'm still not fully clear here. Increasing the 2KB limit would solve all your issues? Because I understand this is a bit unusual (having a fake section just to temporarily keep interrupt vectors) but if we can solve all the problem by simply increasing its size I would gladly take that instead of reworking the whole mechanism just at the end of the day to save one temporary file in the build directory.

@rakons
Copy link
Collaborator Author

rakons commented Aug 1, 2023

Increasing the 2KB limit would solve all your issues? Because I understand this is a bit unusual (having a fake section just to temporarily keep interrupt vectors)

It still forces the vendors to choose some dummy address for that section that is not in use by anything else, just to temporary keep interrupt vectors. Why are we doing that? Why do we need to generate another file from input ELF file to just provide it to the script? Why cannot we just use elftools module that we are using anyway? This PR offers:

  • Build script simplification (removing of the file that is first generated from ELF and then provided to the script with the same ELF).
  • Getting rid of the needs of looking into device and providing some dummy address and space that has to be in the real address space but has to be free from any really existing peripherals and memories.
  • And probably - I did not tested in yet - we would be able to simplify the build process even more because the INFO section does not have to be stripped away in the final build.

@carlocaione
Copy link
Collaborator

It still forces the vendors to choose some dummy address for that section that is not in use by anything else, just to temporary keep interrupt vectors. Why are we doing that? Why do we need to generate another file from input ELF file to just provide it to the script? Why cannot we just use elftools module that we are using anyway? This PR offers:

ok, just trying to understand the rationale here. Can you entirely remove the IDT_LIST section from all the linker scripts in the Zephyr repo and see if the CI is still green?

@rakons
Copy link
Collaborator Author

rakons commented Aug 1, 2023

What about the usage of:

PHDRS
{
  intList PT_NOTE ;
}

...

SECTION_PROLOGUE(.intList,,)
{
	KEEP(*(.irq_info*))
	KEEP(*(.intList*))
} :intList

Did not tested it yet, just looking for some not depreciated way to get the same result. Anybody more familiar with linker scripts?


Just checked that it would not work.

@rakons
Copy link
Collaborator Author

rakons commented Aug 1, 2023

Interesting fact: The hex files before and after the change differs at the end. The lst files are identical.


I found the difference source - the difference is at very end of the file where the boot string banner is placed. As the part of it is the zephyr commit hash - this is quite obvious the it would differ.

@rakons
Copy link
Collaborator Author

rakons commented Aug 3, 2023

I had tried to create and use memory region:

IDT_LIST (!airwx) : ORIGIN = 0, LENGTH = 64K

It is still not the effect we are looking for.

@tejlmand
Copy link
Collaborator

Did some investigation on INFO in binutils.
It was introduced with binutils 2.7, Jun 24, 1996, and made backward compatible (note, this is not the same as deprecated) with binutils 2.10, May 2000.

This means that it has been around for >20 years, and afaict there is not a good alternative in the binutils releases <= 2.38.
2.38 is the version shipped with Zephyr-sdk.
With binutils >=2.39, there seems to be introduced an alternative with:
TYPE = type, where type can be an integer, or: SHT_PROGBITS, SHT_STRTAB, SHT_NOTE, SHT_NOBITS, SHT_INIT_ARRAY, SHT_FINI_ARRAY, and SHT_PREINIT_ARRAY, ref https://sourceware.org/binutils/docs/ld/Output-Section-Type.html.
but I haven't had a chance yet to test out if TYPE = <integer> can achieve the desired behavior.

Another interesting observation is this commit:
https://gnu.googlesource.com/binutils-gdb/+/6752dd75f76457902729a5f03d09fa28ec5d68c3
image

which is fairly recent, Mar 16, 2023, and is using the INFO type.
If the binutils project itself is using this type, then that indicates to me the following

  • There is not a good alternative (or the project itself would have used it)
  • It's not being removed anytime soon, unless replaced with an alternative, meaning we can adjust to that alternative at the same time.

My proposal is to accept the use of (INFO) and follow binutils releases closely, so that as soon as there is a good alternative introduced, then we switch to the alternative.

Note, when switching, for example to TYPE = type, we must carefully take into consideration non-Zephyr-sdk toolchains, as they can still be based on older ld releases.
In such a case we may adjust the linker script template to support both old and new, by testing which binutil / ld release currently in use.

tejlmand
tejlmand previously approved these changes Aug 21, 2023
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.

Approved, based on following comment: #60838 (comment)

@tejlmand
Copy link
Collaborator

tejlmand commented Aug 21, 2023

@rakons maybe take some of the information given here #60838 (comment) and add to the commit message.

In future, it will be easier to do a git blame / git log and understanding the choices made wrt. to (INFO) without having to find the PR / github message itself.

@rakons
Copy link
Collaborator Author

rakons commented Aug 21, 2023

@tejlmand Thank you for the information and the job done. I had spend two days searching for alternative but did not manage to find so deep understanding. What I need now it to provide this proof of concept to the rest of the ld templates to make it fully ready PR.
During this work I will add additional info from your comment.

@rakons
Copy link
Collaborator Author

rakons commented Aug 22, 2023

Just removed missed #ifndef LINKER_ZEPHYR_FINAL from qemu_x86_tiny.ld.

@rakons
Copy link
Collaborator Author

rakons commented Aug 22, 2023

Rebase on main.

@rakons
Copy link
Collaborator Author

rakons commented Aug 22, 2023

Another rebase to include uart_sam0 shadowed variable fix.

@dcpleung
Copy link
Member

Have you tried with LLVM's lld and see if it still works?

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.

we need to extend the linker script generator to work with the new INFO type.

@@ -32,7 +32,7 @@ zephyr_linker_sources_ifdef(CONFIG_GEN_IRQ_VECTOR_TABLE
)

if(CONFIG_GEN_ISR_TABLES)
zephyr_linker_section(NAME .intList VMA IDT_LIST LMA IDT_LIST NOINPUT PASS NOT LINKER_ZEPHYR_FINAL)
zephyr_linker_section(NAME .intList NOINPUT TYPE (INFO) PASS NOT LINKER_ZEPHYR_FINAL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will not work.
The type specified here is not a GNU ld type, but a generic type which then maps to the linker script depending on whether that is ld or a scatter file.

That is also why you see a CMake warning with this PR:

CMake Warning at /projects/github/ncs/zephyr/cmake/modules/extensions.cmake:4247 (message):
  zephyr_linker_section(NAME ...) given unknown arguments: INFO;)                    

ld:

set(SECTION_TYPE_NOLOAD NOLOAD)
set(SECTION_TYPE_BSS NOLOAD)

scatter:

if("${type}" STREQUAL BSS)

https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/linker/armlink/scatter_script.cmake#L269

so for this to work with the linker generator then we need a new type there.

INFO would be a good name 😉

See also: #61846

@tejlmand
Copy link
Collaborator

Have you tried with LLVM's lld and see if it still works?

I have tested with llvm (clang / lld) and compiler-rt.

INFO type is fully understood by lld.

@rakons
Copy link
Collaborator Author

rakons commented Aug 31, 2023

In last force-push:
Updated section description from "TYPE (INFO)" to "TYPE INFO" after the discussion with @tejlmand as this would be the right way to use it after the INFO section would be added into linker script generator.

@rakons
Copy link
Collaborator Author

rakons commented Sep 28, 2023

As it seems we cannot support anything similar to INFO section type on Keil - we have to pass this solution and return to the old one. Not happy about this. The only think that may still be used from this PR is probably the way the script is updated to take the IRQ information directly from the elf file instead of using temporary binary file.

Change the way ".intList" section is handled by the build system.
Now it is marked as INFO section - it would not be available as an output
and does not need to be placed in any memory segment.
It cannot by accessed by objcopy, but can be accessed by elftools instead.

The used (INFO) section type background:

  INFO section was introduced with binutils 2.7 (Jun 1996).
  It was made backward compatible with binutils 2.10 (May 2000),
  and no alternative was provided since then.
  INFO section type was used by binutils-gdb itself in
  6752dd75f76457902729a5f03d09fa28ec5d68c3 commit dated at March 2023.

  With binutils >=2.39, there seems to be introduced an alternative with:
  ``TYPE = type``:
  https://sourceware.org/binutils/docs/ld/Output-Section-Type.html

  We use (INFO) section solution and follow binutils releases
  to migrate when there is a good alternative.

Signed-off-by: Radosław Koppel <radoslaw.koppel@nordicsemi.no>
Signed-off-by: Torsten Rasmussen <torsten.rasmussen@nordicsemi.no>
@rakons
Copy link
Collaborator Author

rakons commented Sep 29, 2023

As the decision has been made not to include this solution because we cannot support it on Keil compiler scatter files. New PR would be created with simplified version that only removes the need of the temporary binary file for interrupt table generation script.

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.

6 participants