LLEXT: more advanced detached section support #80626

lyakh · 2024-10-30T13:29:20Z

With this PR I have been able to move complex functions in extensions into detached sections with all the cross-referencing still working. I'll push a sample SOF PR, using this feature, later.

Move a variable calculation inside the block, where it is actually used. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Use the same logging level as in the rest of LLEXT. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Detached sections are used in situ without being copied to or referenced from ext->mem[] LLEXT region arrays. Their caches must be synchronised accordingly. Signed-off-by: Guennadi Liakhovetski <[email protected]>

subsys/llext/llext_link.c

pillo79

Thanks, mostly LGTM and I love the -fPIC option implementation. A few comments below.

pillo79 · 2024-10-31T14:50:43Z

subsys/llext/llext_link.c

+		 * In some cases
+		 * text + got_offset
+		 * is the same as
+		 * (uintptr_t)llext_loaded_sect_ptr(ldr, ext, shdr->sh_info) + rel->r_offset
+		 * but not in all. Eventually we could figure out how to generalize the latter to
+		 * cover all cases and to possibly simplify the above calculation


If this is the case, I think it deserves a dedicated LLEXT GH issue and some investigation. I believe it should absolutely be possible to only use the latter.

@pillo79 hm, I did see them being different during development, but I re-added a check now and it never triggered... Looks like they are indeed equal, maybe I had a bug in that version still.

If this is still a concern, I propose to replace this comment with a check, so we get useful debugging info for this issue.

@pillo79 how about "TODO: A should be the same as B, consider simplifying?"

pillo79 · 2024-10-31T15:16:42Z

cmake/compiler/xt-clang/target.cmake

 )
+
+if(CONFIG_LLEXT_BUILD_PIC)
+set(LLEXT_REMOVE_FLAGS ${LLEXT_REMOVE_FLAGS}
+  -fno-pic
+  -fno-pie
+)
+set(LLEXT_APPEND_FLAGS ${LLEXT_APPEND_FLAGS}
+  -fPIC
+)
+else()
+set(LLEXT_APPEND_FLAGS ${LLEXT_APPEND_FLAGS}
+  -ffreestanding


Interesting - never saw -ffreestanding before, so I have read about it... IIUC it makes the llext use the core's definition of the standard functions. Shouldn't this be the default for both PIC and non-PIC? (... and on other arches as well?)

Also, should we add -fno-pic here? Not sure what the Zephyr default is 🤷‍♂️

[whatever the decision, also update gcc/target_xtensa.cmake to match]

pillo79

I said "mostly"... 😇 but I disagree with the last commit in the series, and I really wish to find a better solution which works for everybody.

pillo79 · 2024-10-31T15:54:45Z

subsys/llext/llext_load.c

+		ldr->sect_hdrs = NULL;
 	}
-	ldr->sect_hdrs = NULL;



This tiny change leaks the contents of an internal and purposefully undocumented field, only when you use a buf_loader, for no apparent Zephyr reason. Do we have to support this peculiar field and functionality from now on and forevermore?

We went through this already with SOF taking "ownership" of the similarly private ldr->sects[].
Please do not access private fields and expect them to be stable. Instead, a properly documented API to access the information you need would improve LLEXT for everyone.

Only as examples, either of these would work for me - but feel free to propose your own:

the sect_hdrs pointer could be moved to a documented field of struct llext and disposed later with the extension, maybe gated with a load_param so it doesn't waste memory for those not interested in this feature;

a separate "cleanup" function could be added to dispose of all llext_loader fields (in this case samples and tests should be updated as well).

@pillo79 yep, agree, I don't like that either and I expected objections against it. In your first option - you mean llext_unload(), right? But currently it has no access to the loader. So we'd need to either modify the function's signature by adding it as the second argument, or to add a pointer to the loader to struct llext?

The loader struct can be dropped as soon as llext_load completes, so I would not create such a constraint. I was instead thinking about fully moving the field from struct llext_loader to struct llext. That way it is unambiguous that it's a permanent feature.

The pointer could be freed at the end of llext_load unless a load_param field like keep_section_headers (or whatever) was set, in which case it would be kept until llext_unload.
What do you think?

@pillo79 in principle that sounds good, would work fine for me, but with that header array you anyway then have to scan it, you probably don't have section index to jump directly to it. And we do already have an API function llext_find_section() that scans that array. Its usefulness at least for SOF is currently limited by it only returning an offset, not a complete header. Ideally I would modify that function to also return an entire section header in a user-supplied parameter, but it's an official API (LLEXT is still experimental). We could add a new function for that and potentially deprecate / migrate over time.
Also, if we do move the pointer to struct llext maybe just keep it until freeing without a parameter - it isn't that much memory

Great idea! Something along the lines of:

int llext_get_section_info(void **start, size_t *length, struct elf_shdr **shdr)

where each parameter is optional?
Or even better, I guess we can have a struct llext_section_info with all those fields. It will make it easier to extend in the future without breaking compatibility.
WDYT?

Sorry, I had a different use case in mind - for loaded sections. LGTM as well!

I like it

@teburd hm, but I've just realised, that it cannot work (without large-scale changes to LLEXT). All access to section header tables in LLEXT is currently performed via the loader. IIUC originally the loader was assumed to be used only for initial linking / relocation, only while llext_load() is running. Then we've relaxed that limitation by allowing the user to call llext_seek(), llext_peek(), llext_read() and potentially llext_find_section() - all with the loader as the first argument. But still we free all the dynamically allocated loader assets when exiting llext_load(). Maybe we indeed should extend the loader life-cycle and make an explicit loader_free()?

This gets a bit awkward at some point as we're allocating in llext_load() but then I guess we're needing information past the load call. What lifetime are you looking for in the loader? Why?

we've relaxed that limitation by allowing the user to call llext_seek(), llext_peek(), llext_read()

@lyakh These are used by LLEXT for ELF loading and they are explicitly undocumented. TBH I don't think an end user should use them 🙂

we free all the dynamically allocated loader assets when exiting llext_load()

That should be fine, if we move sect_hdrs to struct llext as discussed. This is definitely a big commit, but should be automated with a sed command 🙂 What field that is needed after load completes are you referring to?

teburd

Makes sense to me

pillo79 · 2024-11-06T14:10:55Z

subsys/llext/llext.c

@@ -23,12 +23,32 @@ static sys_slist_t _llext_list = SYS_SLIST_STATIC_INIT(&_llext_list);

 static struct k_mutex llext_lock = Z_MUTEX_INITIALIZER(llext_lock);

-ssize_t llext_find_section(struct llext_loader *ldr, const char *search_name)
+ssize_t llext_section_offset(struct llext_loader *ldr, struct llext *ext, const char *search_name)


The find_section API was IMO terribly broken, since it does not tell you the size associated with the section; it is unclear to me how to safely use it. 🤷‍♂️
Can we take this opportunity to provide a replacement where you get the full section header information instead? SOF can just refer to the sh_offset field in the returned struct.

Suggested change

ssize_t llext_section_offset(struct llext_loader *ldr, struct llext *ext, const char *search_name)

const elf_shdr_t *llext_get_section_header(struct llext_loader *ldr, struct llext *ext, const char *search_name)

@pillo79 yep, can do that, but I'd rather not return a pointer, but take a pointer as argument and copy the header into it. For a user, not familiar with LLEXT internals, returning a pointer to an object raises questions about object life-cycle, which might be a bit confusing?

pillo79 · 2024-11-06T14:13:18Z

subsys/llext/llext_link.c

+		 * In some cases
+		 * text + got_offset
+		 * is the same as
+		 * (uintptr_t)llext_loaded_sect_ptr(ldr, ext, shdr->sh_info) + rel->r_offset
+		 * but not in all. Eventually we could figure out how to generalize the latter to
+		 * cover all cases and to possibly simplify the above calculation


If this is still a concern, I propose to replace this comment with a check, so we get useful debugging info for this issue.

pillo79 · 2024-11-06T14:14:31Z

tests/subsys/llext/simple/src/test_llext_simple.c

@@ -415,7 +415,7 @@ ZTEST(llext, test_find_section)
 	res = llext_load(loader, "find_section", &ext, &ldr_parm);
 	zassert_ok(res, "load should succeed");

-	section_ofs = llext_find_section(loader, ".data");
+	section_ofs = llext_section_offset(loader, ext, ".data");


~~Since find_section is not deprecated, both should be tested here.~~
EDIT: Wow didn't notice it actually was deprecated! 👍 to that, though this deprecation broke a test in SOF. 🙁 Maybe the deprecation should be delayed after SOF switches to the new API...

we plan to deprecate it - once we've updated SOF to not call it to avoid twister failures

When detached sections are used, STB_GLOBAL relocations also have to be processed depending on the relocation type. This commit unified STB_GLOBAL and STB_LOCAL flows by making them use the same relocation type parser, adds support for R_XTENSA_GLOB_DAT and R_XTENSA_JMP_SLOT type relocations on Xtensa and fixes STT_SECTION address calculation for such sections. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Currently when building LLEXT for Xtensa we use the -fPIC compiler option, but this cannot be used when using detached sections in extensions. Add a Kconfig option to switch between the two compilation modes and switch -fPIC off when building relocatable (partially linked) ELF binaries. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Move cached section headers to struct llext from struct llext_loader to preserve them after llext_load() returns. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Now that section header cache is persistent, it can be used for finding sections. Add a new function, similar to llext_find_section() to use it and deprecate llext_find_section(). Signed-off-by: Guennadi Liakhovetski <[email protected]>

pillo79

LGTM!

lyakh added 3 commits October 30, 2024 14:23

LLEXT: (cosmetic) reduce the scope of a variable

1a9a727

Move a variable calculation inside the block, where it is actually used. Signed-off-by: Guennadi Liakhovetski <[email protected]>

LLEXT: Xtensa: fix logging level

55318b8

Use the same logging level as in the rest of LLEXT. Signed-off-by: Guennadi Liakhovetski <[email protected]>

LLEXT: fix detached section cache synchronisation

da9c7c4

Detached sections are used in situ without being copied to or referenced from ext->mem[] LLEXT region arrays. Their caches must be synchronised accordingly. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh requested review from teburd and pillo79 October 30, 2024 13:29

zephyrbot added area: Build System area: llext Linkable Loadable Extensions area: Toolchains Toolchains area: Xtensa Xtensa Architecture labels Oct 30, 2024

zephyrbot requested review from 57300, andyross, ceolin, dcpleung, jeremybettis, nashif, nordicjm, stephanosio and tejlmand October 30, 2024 13:30

zephyrbot assigned teburd Oct 30, 2024

teburd reviewed Oct 30, 2024

View reviewed changes

subsys/llext/llext_link.c Show resolved Hide resolved

pillo79 reviewed Oct 31, 2024

View reviewed changes

pillo79 requested changes Oct 31, 2024

View reviewed changes

lyakh force-pushed the llext branch 5 times, most recently from 2914e9c to 2bde66a Compare November 6, 2024 12:03

teburd previously approved these changes Nov 6, 2024

View reviewed changes

lyakh dismissed teburd’s stale review via dfc3690 November 6, 2024 13:13

lyakh force-pushed the llext branch from 2bde66a to dfc3690 Compare November 6, 2024 13:13

pillo79 reviewed Nov 6, 2024

View reviewed changes

lyakh added 4 commits November 6, 2024 17:59

LLEXT: preserve section headers

b11d6d8

Move cached section headers to struct llext from struct llext_loader to preserve them after llext_load() returns. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh force-pushed the llext branch from dfc3690 to 23c9838 Compare November 6, 2024 17:40

pillo79 approved these changes Nov 7, 2024

View reviewed changes

teburd approved these changes Nov 8, 2024

View reviewed changes

lyakh mentioned this pull request Nov 8, 2024

[TO BE REBASED] disable FLIX commands #81145

Draft

nashif merged commit 44d96a2 into zephyrproject-rtos:main Nov 16, 2024
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLEXT: more advanced detached section support #80626

LLEXT: more advanced detached section support #80626

lyakh commented Oct 30, 2024

pillo79 left a comment

pillo79 Oct 31, 2024

lyakh Nov 4, 2024

pillo79 Nov 6, 2024

lyakh Nov 6, 2024

pillo79 Oct 31, 2024

pillo79 left a comment

pillo79 Oct 31, 2024

lyakh Nov 4, 2024

pillo79 Nov 4, 2024 •

edited

Loading

lyakh Nov 4, 2024

pillo79 Nov 4, 2024

teburd Nov 4, 2024

pillo79 Nov 4, 2024

lyakh Nov 4, 2024

teburd Nov 4, 2024

pillo79 Nov 4, 2024 •

edited

Loading

teburd left a comment

pillo79 Nov 6, 2024

lyakh Nov 6, 2024

pillo79 Nov 6, 2024

pillo79 Nov 6, 2024 •

edited

Loading

lyakh Nov 6, 2024

pillo79 left a comment

	ssize_t llext_section_offset(struct llext_loader ldr, struct llext ext, const char *search_name)
	const elf_shdr_t llext_get_section_header(struct llext_loader ldr, struct llext ext, const char search_name)

LLEXT: more advanced detached section support #80626

LLEXT: more advanced detached section support #80626

Conversation

lyakh commented Oct 30, 2024

pillo79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 left a comment

Choose a reason for hiding this comment

pillo79 Nov 4, 2024 •

edited

Loading

pillo79 Nov 4, 2024 •

edited

Loading

pillo79 Nov 6, 2024 •

edited

Loading