llext: add support for init arrays - revised #77641

pillo79 · 2024-08-27T13:25:54Z

This replaces #76724, and includes all feedback received there.

Add support for the .preinit_array, .init_array and .fini_array sections in ELF files. These sections are arrays of function pointers that are filled by the compiler with the addresses of functions, such as C++ constructors, that need to be called at app startup or cleanup by the loader.

This is now achieved separately from llext_load, in whatever execution context is deemed secure by the application writer, by either calling directly llext_bringup/llext_teardown or starting a new user or kernel thread with the llext_bootstrap function. The list of function pointers is obtained in all cases via a syscall. (link to related doc)

~~The PR also adds a init_fini test that creates an ELF file with the required sections and verifies the execution order is correct.~~ EDIT: Moved to #77941.

~~Supporting the .fini_array section requires more changes and is not currently implemented.~~

mathieuchopstm

Looks good overall.

A few thoughts and some minor change suggestions/requests:

doc/services/llext/load.rst

include/zephyr/llext/llext.h

subsys/llext/llext.c

subsys/llext/llext_handlers.c

subsys/llext/llext_load.c

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

subsys/llext/llext_load.c

teburd · 2024-08-28T17:04:04Z

subsys/llext/llext.c

+	}
+
+	/* Start extension main function, if provided */
+	if (entry_fn) {


This seems like it could be done by the caller, unclear what benefit passing entry_fn in and calling it here does.

My reasoning was to allow llext_bootstrap to be called directly by k_thread_create, to minimize boilerplate code. See the changes in test_llext_simple.c and the above linked doc.

Ahh that makes sense, it’s user mode friendly then I take it?

Absolutely. The LLEXT test suite in this PR already runs llext_bootstrap both in user and kernel mode 🙂

pillo79 · 2024-08-29T10:26:43Z

v2:

addressed all change suggestions
added sanity check for returned pointers in llext_get_init_array

include/zephyr/llext/llext.h

subsys/llext/llext_load.c

subsys/llext/llext.c

lyakh · 2024-08-30T10:30:32Z

subsys/llext/llext.c

+		typedef void (*elf_init_fn_t)(void);
+
+		int init_fn_count = ret / sizeof(elf_init_fn_t);
+		elf_init_fn_t init_fns[init_fn_count];


should we check ret for a maximum value before allocating an arbitrary size array on stack?

The syscall returns sizes of arrays already in memory, so this number is "sane", but of course the size of the stack is unknown. Not sure how I could add overflow checks - suggestions?

lyakh · 2024-08-30T10:39:12Z

subsys/llext/llext_handlers.c

+
+	if (buf) {
+		char *byte_ptr = buf;
+		const char **fn_ptrs = buf;


const void **? I had to think why char, thought there would be some particular purpose in it. Same for text_start and test_end below. In fact personally I prefer uint8_t * over char * in such cases for the 2 reasons: (1) these aren't characters or strings, and (2) uint8_t has a well defined signedness unlike char, but I do realise, that char is used a lot in such cases...

I am only comparing the pointer values in a sanity check here, so the different types have no actual effect; choosing a byte-sized type allows to easily calculate text_end and compare pointers without ugly extra casts, and char * is used by memcpy() a few lines above.

subsys/llext/llext_handlers.c

lyakh · 2024-08-30T10:46:16Z

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

+};
+static const void *const init_fn_ptrs[] __used Z_GENERIC_SECTION(".init_array") = {
+	init_fn
+};


can we actually use C++ tests? Might be even nicer to actually have a couple of constructors as a test.

... that would require proper C++ support for LLEXT (cmake, toolchains, etc). That's a different league - definitely not going that far in one PR. This is portable and has the same effect.

C++ tests will come in due time! We as Arduino kinda depend on that 🙂

subsys/llext/llext_handlers.c

pillo79

Thanks for the review! Will post an update soon.

pillo79 · 2024-08-30T12:11:44Z

subsys/llext/llext.c

+		typedef void (*elf_init_fn_t)(void);
+
+		int init_fn_count = ret / sizeof(elf_init_fn_t);
+		elf_init_fn_t init_fns[init_fn_count];


The syscall returns sizes of arrays already in memory, so this number is "sane", but of course the size of the stack is unknown. Not sure how I could add overflow checks - suggestions?

pillo79 · 2024-08-30T12:21:46Z

subsys/llext/llext_handlers.c

+
+	if (buf) {
+		char *byte_ptr = buf;
+		const char **fn_ptrs = buf;


I am only comparing the pointer values in a sanity check here, so the different types have no actual effect; choosing a byte-sized type allows to easily calculate text_end and compare pointers without ugly extra casts, and char * is used by memcpy() a few lines above.

pillo79 · 2024-08-30T12:50:45Z

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

+};
+static const void *const init_fn_ptrs[] __used Z_GENERIC_SECTION(".init_array") = {
+	init_fn
+};


... that would require proper C++ support for LLEXT (cmake, toolchains, etc). That's a different league - definitely not going that far in one PR. This is portable and has the same effect.

C++ tests will come in due time! We as Arduino kinda depend on that 🙂

subsys/llext/llext_load.c

Load the .preinit_array, .init_array and .fini_array sections in ELF files. These sections are arrays of function pointers that are filled by the compiler with the addresses of functions that need to be called at startup or termination by the loader, such as C++ constructors and destructors. Signed-off-by: Luca Burelli <[email protected]>

llext_bringup() and llext_teardown() are intended to be used to call the extension's own initialization and cleanup functions, respectively. They are meant to be called by the developer after loading an extension and before unloading it. The list of function pointers to be called is obtained via the new llext_get_fn_table() syscall, so that they are compatible with user mode. llext_bootstrap() is intended to be used as the entry point for a thread created to run an extension, in either user or kernel contexts. It will call the extension's own initialization functions and then an additional entry point in the same context (if desired). The same function can also be called directly in the main thread, if only initialization is required. Signed-off-by: Luca Burelli <[email protected]>

Add documentation for the new LLEXT APIs that allow to call the initialization and cleanup functions of an extension. Signed-off-by: Luca Burelli <[email protected]>

pillo79 · 2024-09-03T18:45:19Z

Rebased onto current main to include #77473, then pushed v3:

moved section identification to llext_map_sections
added support for .fini_array as well
added llext_bringup and llext_teardown APIs to manually call setup / cleanup functions
moved tests to separate PR (llext: add tests for init arrays #77941) to simplify review.

lyakh · 2024-09-04T07:45:18Z

subsys/llext/llext_load.c

+				 * referenced at most once in the ELF file.
+				 */
+				LOG_ERR("Region %d redefined", mem_idx);
+				return -ENOEXEC;


wouldn't this be caught in line 211?

No, line 211 checks that the ELF section (ldr->sect_map[shndx]) was assigned a region before entering llext_map_sections - I added that log so the whole function prints a full ELF section table, including reserved sections found by llext_find_tables.
This is instead checking if the region has been already defined (ie. ldr->sects[mem_idx] - the array that really should be renamed ldr->regions...).

lyakh · 2024-09-04T07:50:33Z

subsys/llext/llext.c

+
+	/* Start extension main function */
+	LOG_DBG("calling entry function %p(%p)", entry_fn, user_data);
+	entry_fn(user_data);


you wrote "if desired" in the commit description. Should you check for entry_fn == NULL then?

Sorry, commit message is a leftover from the previous iteration, when llext_bootstrap was the only way to call init functions. I have since changed code, comments and documentation to make it clear it's not optional anymore; if you only want to init an extension, you now have llext_bringup.
Will fix if I have to do any code change 👍

pillo79 force-pushed the pr-init-arrays-v3 branch 2 times, most recently from bd0f582 to d3d9141 Compare August 28, 2024 07:26

pillo79 requested review from teburd, mathieuchopstm and lyakh August 28, 2024 09:33

pillo79 marked this pull request as ready for review August 28, 2024 09:33

zephyrbot added the area: llext Linkable Loadable Extensions label Aug 28, 2024

zephyrbot assigned teburd Aug 28, 2024

pillo79 mentioned this pull request Aug 28, 2024

llext: add support for init arrays #76724

Closed

pillo79 requested a review from ceolin August 28, 2024 09:36

mathieuchopstm reviewed Aug 28, 2024

View reviewed changes

teburd reviewed Aug 28, 2024

View reviewed changes

pillo79 force-pushed the pr-init-arrays-v3 branch from d3d9141 to 9623405 Compare August 29, 2024 10:11

mathieuchopstm previously approved these changes Aug 29, 2024

View reviewed changes

lyakh reviewed Aug 30, 2024

View reviewed changes

pillo79 commented Aug 30, 2024

View reviewed changes

pillo79 added 3 commits September 3, 2024 17:49

doc: llext: add extension bringup and teardown documentation

035b4ed

Add documentation for the new LLEXT APIs that allow to call the initialization and cleanup functions of an extension. Signed-off-by: Luca Burelli <[email protected]>

pillo79 force-pushed the pr-init-arrays-v3 branch from 9623405 to 1fb4040 Compare September 3, 2024 18:33

pillo79 dismissed mathieuchopstm’s stale review via 035b4ed September 3, 2024 18:33

pillo79 force-pushed the pr-init-arrays-v3 branch from 1fb4040 to 035b4ed Compare September 3, 2024 18:33

pillo79 mentioned this pull request Sep 3, 2024

llext: add tests for init arrays #77941

Merged

pillo79 requested review from teburd, mathieuchopstm and lyakh September 3, 2024 18:45

lyakh reviewed Sep 4, 2024

View reviewed changes

lyakh approved these changes Sep 6, 2024

View reviewed changes

teburd approved these changes Sep 6, 2024

View reviewed changes

nashif merged commit fca654c into zephyrproject-rtos:main Sep 6, 2024
24 checks passed

pillo79 deleted the pr-init-arrays-v3 branch October 2, 2024 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llext: add support for init arrays - revised #77641

llext: add support for init arrays - revised #77641

pillo79 commented Aug 27, 2024 •

edited

Loading

mathieuchopstm left a comment

teburd Aug 28, 2024

pillo79 Aug 29, 2024

teburd Aug 29, 2024 •

edited

Loading

pillo79 Aug 29, 2024

pillo79 commented Aug 29, 2024

lyakh Aug 30, 2024

pillo79 Aug 30, 2024

lyakh Aug 30, 2024

pillo79 Aug 30, 2024 •

edited

Loading

lyakh Aug 30, 2024

pillo79 Aug 30, 2024

pillo79 left a comment

pillo79 Aug 30, 2024

pillo79 Aug 30, 2024 •

edited

Loading

pillo79 Aug 30, 2024

pillo79 commented Sep 3, 2024

lyakh Sep 4, 2024

pillo79 Sep 4, 2024

lyakh Sep 4, 2024

pillo79 Sep 4, 2024

llext: add support for init arrays - revised #77641

llext: add support for init arrays - revised #77641

Conversation

pillo79 commented Aug 27, 2024 • edited Loading

mathieuchopstm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 Aug 30, 2024 • edited Loading

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

pillo79 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 commented Sep 3, 2024

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 commented Aug 27, 2024 •

edited

Loading

teburd Aug 29, 2024 •

edited

Loading

pillo79 Aug 30, 2024 •

edited

Loading

pillo79 Aug 30, 2024 •

edited

Loading