-
Notifications
You must be signed in to change notification settings - Fork 318
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
LLEXT: automatic gcc / xt-clang switching #9151
Conversation
scripts/xtensa-build-zephyr.py
Outdated
# libraries for Xtensa. Therefore when it's used we switch to | ||
# building relocatable ELF objects. | ||
if ("ZEPHYR_TOOLCHAIN_VARIANT" in platf_build_environ.keys() and | ||
platf_build_environ["ZEPHYR_TOOLCHAIN_VARIANT"] == 'xt-clang'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python can already do that for you:
platf_build_environ["ZEPHYR_TOOLCHAIN_VARIANT"] == 'xt-clang'): | |
if platf_build_environ.get("ZEPHYR_TOOLCHAIN_VARIANT") == 'xt-clang' |
scripts/llext_link_helper.py
Outdated
last_type = TYPE_NONE | ||
|
||
SHF_ALLOC = 2 | ||
SHF_EXECINSTR = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple git grep
showed that these are already defined, no need to duplicate
ipython3
In [1]: from elftools.elf.constants import SH_FLAGS
In [2]: SH_FLAGS.SHF_EXECINSTR
Out[2]: 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In [1]: from elftools.elf.constants import SH_FLAGS
that's the line I couldn't find, thanks
scripts/llext_link_helper.py
Outdated
TYPE_NONE = 0 | ||
TYPE_DATA = 1 | ||
TYPE_BSS = 2 | ||
last_type = TYPE_NONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3/library/enum.html
from enum import Enum
SECTION_TYPE = Enum('SECTION_TYPE', ['DATA', 'BSS'])
Don't define TYPE_NONE (the "billion dollar mistake"), just use Python's None
.
# A shared object type image, it contains segments | ||
sections = ['.text', '.rodata', '.data', '.bss'] | ||
alignment = [0x1000, 0x1000, 0x10, 0x1000] | ||
# File names differ when building shared or relocatable objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really belong to this script? As opposed to its caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do deal with both cases internally, and passing a module name additionally to the file name would be redundant
scripts/llext_link_helper.py
Outdated
data_found = False | ||
bss_found = False | ||
|
||
# The layout seems to be: .text, followed by other .data, .rodata and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"seems to be"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-hb I haven't found anywhere a statement, that that should be the case, but so far that's what I've seen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried to understand the logic so I wonder why it matters. Can't you just perform two passes? (1) collect all required information, no side-effect (2) do what must be done.
scripts/llext_link_helper.py
Outdated
|
||
if bss_found or data_found: | ||
print("ERROR: So far .text is expected first. Fix the script!") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERROR == success?
scripts/llext_link_helper.py
Outdated
if last_type == TYPE_BSS and data_found: | ||
# Trouble: intermixed data and bss | ||
print('ERROR: data again after data and .bss! Aborting') | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aborting with a success code?
Maybe:
assert last_type != TYPE_BSS or not data_found, "data again after data and .bss! Aborting'
When using a clang Cadence toolchain to build SOF and LLEXT modules we need to select a different LLEXT type than when using a Zephyr gcc toolchain. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
It was clear that hard-coded section names aren't reliable enough but they broke down way earlier than has been expected. This patch replaces hard-coded sections with a loop, scanning all sections and selecting them based on their flags and types. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When using xt-clang we cannot build shared objects, so we use '-r' to link relocatable objects. But the decision shouldn't be made based on the name of the compiler, but based on the selected LLEXT_BINARY_TYPE option. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style looks good to me.
Someone must review the actual ELF logic.
@lyakh Ready to go, pending mandatory CI to pass |
@wszypelt is quickbuild hanging here, can we restart, please? |
Even with https://sof-ci.01.org/sofpr/PR9403/build7332/build/firmware_community/mtl.log
|
We build different LLEXT object types with gcc and with xt-cland and switching between them should be automatic