LLEXT: dependency test #77738

lyakh · 2024-08-29T10:56:03Z

Add a twister test to the new LLEXT dependency infrastructure. It contains #77473 but the goal is to merge them separately

pillo79

Thank you for this! 🙇‍♂️ 🚀

One small issue I see with this setup is that it does not reliably test the actual relocation; there is no failure in the dependent if calling the function has no effect.

Instead of reusing the hello_world extension, what do you think about two tiny dep_parent_ext/dep_child_ext so that some easy calculation in child depends on the result of a function exported from parent? This way you can use z_assert in child to ensure the path was taken correctly.

pillo79 · 2024-08-29T14:30:49Z

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

+	void *dependency_buf = k_heap_alloc(&tllext_heap, sizeof(hello_world_ext), K_NO_WAIT);
+
+	zassert_not_null(dependency_buf);
+
+	void *dependent_buf = k_heap_alloc(&tllext_heap, sizeof(export_dependent_ext), K_NO_WAIT);
+
+	zassert_not_null(dependent_buf);
+
+	memcpy(dependency_buf, hello_world_ext, sizeof(hello_world_ext));
+	memcpy(dependent_buf, export_dependent_ext, sizeof(export_dependent_ext));


Why do you need to copy the _ext buffers?

there's a comment in the respective commit - because I reuse "hello world" and I thought it could be done by others from time to time too. And if we relocate extensions in place, that breaks

@pillo79 47f0f1c

Sorry, it was a quick review and I did not check the commits. 🤦‍♂️
However there are two things I am not fond of in that commit:

it is being applied even when STORAGE_WRITABLE is not enabled. This makes the LLEXT code always access ELF data in RAM, and thus will no longer catch bugs that cause LLEXT to write something there.

even if applied only on STORAGE_WRITABLE, this means that ELF buffers are never written to - they should be made fully const (not LLEXT_CONST).

pillo79 · 2024-08-29T14:31:47Z

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

+
+	void (*test_entry_fn)() = llext_find_sym(&ext_dependent->exp_tab, "test_dependent");
+
+	zassert_not_null(test_entry_fn, "test_entry should be an exported symbol");


test_entry in the message is really test_dependent

yeah, there's one common entry point, so I thought it shouldn't matter

lyakh · 2024-08-29T14:59:10Z

One small issue I see with this setup is that it does not reliably test the actual relocation; there is no failure in the dependent if calling the function has no effect.

@pillo79 thanks for reviewing! I first have done two extensions, but then I decided that I can reuse "hello world" for the dependency. As for no effect - I thought the effect would be an exception - if we somehow linked wrongly? So, yes, if there's a strong opinion, that we need a more explicit test, we can do that too

pillo79 · 2024-08-29T15:11:52Z

One small issue I see with this setup is that it does not reliably test the actual relocation; there is no failure in the dependent if calling the function has no effect.

[...] As for no effect - I thought the effect would be an exception - if we somehow linked wrongly?

Yes, most of the time a bad byte sequence will cause a CPU exception. However, I am sure I have seen jumps being silently ignored in some early LLEXT debugging. Maybe they were relative and "jumped 0 bytes" without a valid relocation?
Luckily we are past that time now 🥳 but I would still err on the safe side... 😇

lyakh · 2024-08-29T15:22:44Z

One small issue I see with this setup is that it does not reliably test the actual relocation; there is no failure in the dependent if calling the function has no effect.

[...] As for no effect - I thought the effect would be an exception - if we somehow linked wrongly?

Yes, most of the time a bad byte sequence will cause a CPU exception. However, I am sure I have seen jumps being silently ignored in some early LLEXT debugging. Maybe they were relative and "jumped 0 bytes" without a valid relocation? Luckily we are past that time now 🥳 but I would still err on the safe side... 😇

yeah, we could make the extension cryptographically sign our (pseudo-) random challenge... or we can just make it return 42 :-)

pillo79

I disagree with modifying the test_entry() signature in all extensions, when the return value is only ever useful for the new custom test.

Instead of the last 2 commits, I suggest to add 2 extensions:

one exporting an int get_value() { return 42; },
the other a standard void test_entry() { zassert_equal(get_value(), 42); }.

pillo79 · 2024-09-04T08:38:32Z

tests/subsys/llext/simple/CMakeLists.txt

@@ -15,7 +15,9 @@ target_include_directories(app PRIVATE
  ${ZEPHYR_BASE}/arch/${ARCH}/include
 )

-set(ext_names hello_world logging relative_jump object syscalls threads_kernel_objects)
+set(ext_names
+  hello_world logging relative_jump object syscalls threads_kernel_objects export_dependent


Please convert this so the names are one per line, I think we are going to need this going forward 🙂

pillo79 · 2024-09-04T08:51:56Z

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

@@ -38,6 +38,10 @@ LOG_MODULE_REGISTER(test_llext_simple);
 #define LLEXT_FIND_BUILTIN_SYM(symbol_name) llext_find_sym(NULL, # symbol_name)
 #endif

+#ifdef CONFIG_LLEXT_STORAGE_WRITABLE
+K_HEAP_DEFINE(tllext_heap, 32 * 1024);


Suggested change

K_HEAP_DEFINE(tllext_heap, 32 * 1024);

K_HEAP_DEFINE(scratch_heap, CONFIG_LLEXT_HEAP_SIZE * 1024);

(name is a suggestion but please make it more descriptive, and size it automatically)

just to verify - CONFIG_LLEXT_HEAP_SIZE is used in llext_mem.c as the heap size for LLEXT itself, you want to use the same here for the test?

I know it's overkill, but it was the simplest solution I could think of - you can also add a CONFIG_MAX_ELF_SIZE in the test Kconfig, for example.
IMHO the hardcoded number will bite us often - AArch64 already needs a bigger buffer in the initial PR 🙂
But these comments are not blockers, sorry. Don't know how to explain what I needed to the Github UI, apparently 🤦‍♂️

lyakh · 2024-09-06T09:49:21Z

I disagree with modifying the test_entry() signature in all extensions, when the return value is only ever useful for the new custom test.

Instead of the last 2 commits, I suggest to add 2 extensions:
* one exporting an `int get_value() { return 42; }`,

* the other a standard `void test_entry() { zassert_equal(get_value(), 42); }`.

@pillo79 I actually think it's good for extensions to return a value, also demonstrates and tests the API better IMHO. @teburd what do you think?

pillo79 · 2024-09-06T10:34:19Z

I disagree with modifying the test_entry() signature in all extensions, when the return value is only ever useful for the new custom test.

@pillo79 I actually think it's good for extensions to return a value, also demonstrates and tests the API better IMHO. @teburd what do you think?

@lyakh I am totally in favor of doing that; I would also go the extra step of passing a pointer in, TBH.
But the way it was implemented gave no meaning or definition to the value in any test except the new special-case "dependency", where it was compared to a known one. That's what I want to avoid 🙂

So I would gladly approve either:

a standard way to define expected return values and verify them in llext_load_call_unload(), refactoring current tests to match, plus the new dependency one, or
the simpler solution of doing the check inside the specific _ext, as every other basic test does.

lyakh · 2024-09-06T10:58:23Z

a standard way to define expected return values and verify them in llext_load_call_unload(), refactoring current tests to match, plus the new dependency one, or

the simpler solution of doing the check inside the specific _ext, as every other basic test does.

@pillo79 ok, for option 1 - would it be enough if I extended LLEXT_LOAD_UNLOAD(_name, _userspace, _perm_setup) with an additional _ret parameter and pass it to threads as the third argument? I was also thinking about a void * argument, but can you pass such pointers to user-space threads?
For option 2 - you mean while making all test functions return an integer, or making a single new one to be called in this test?

pillo79

Looks much better to me, thanks! 👍 2 minor comments below.

pillo79 · 2024-09-10T13:17:53Z

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

+	short a = mask & (uintptr_t)test_dependent;
+	short b = mask & ((uintptr_t)test_dependent >> half_ptr_bits);


I don't think short is correct on 64 bit arches given the values involved (although the assert will be fine).
IMHO a simpler calculation with constant values would make the logic clearer and still test proper linking and argument passing.

@pillo79 yeah, I was thinking about it too - if addresses are 64-bit, then IIRC int can still be 32 bits. So I wanted to replace int with long and short with int, would that be enough?

pillo79 · 2024-09-10T13:21:32Z

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

+#include <zephyr/sys/util.h>
+extern int test_dependency(short a, short b);
+
+void test_dependent(void)


Nit: I think this should be test_entry, for consistency with all other test suites.

LLEXT loader parameters are input-only, make them "const." Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

When identifying the size of an extension image we simply need its size in bytes, not the number of array elements, that contain it. Both happen to return the same value, because we use arrays of bytes, but using sizeof() is clearer. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

This test uses two LLEXT objects: one of them exports a symbol and the other one calls the exported function. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

pillo79

LGTM!

lyakh force-pushed the depend-test branch 2 times, most recently from c04e284 to 21f1a03 Compare August 29, 2024 14:23

pillo79 reviewed Aug 29, 2024

View reviewed changes

lyakh force-pushed the depend-test branch from 21f1a03 to 23380d1 Compare August 30, 2024 15:10

lyakh mentioned this pull request Aug 30, 2024

LLEXT dependencies: add support for linking between LLEXT objects #77473

Merged

lyakh force-pushed the depend-test branch from 23380d1 to 239b75c Compare September 3, 2024 10:21

lyakh marked this pull request as ready for review September 3, 2024 10:21

zephyrbot added the area: llext Linkable Loadable Extensions label Sep 3, 2024

zephyrbot requested a review from teburd September 3, 2024 10:22

zephyrbot assigned teburd Sep 3, 2024

lyakh force-pushed the depend-test branch 2 times, most recently from 56c7e5c to 63b376b Compare September 3, 2024 12:09

lyakh requested a review from pillo79 September 4, 2024 09:32

pillo79 requested changes Sep 4, 2024

View reviewed changes

lyakh force-pushed the depend-test branch from 63b376b to 13a59f3 Compare September 10, 2024 10:48

lyakh requested a review from pillo79 September 10, 2024 10:48

pillo79 reviewed Sep 10, 2024

View reviewed changes

lyakh force-pushed the depend-test branch from 13a59f3 to 09aa62e Compare September 10, 2024 15:18

lyakh added 3 commits September 10, 2024 17:31

llext: make loader parameters "const"

70db7a6

LLEXT loader parameters are input-only, make them "const." Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: add a test for inter-extension linking

8029f37

This test uses two LLEXT objects: one of them exports a symbol and the other one calls the exported function. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the depend-test branch from 09aa62e to 8029f37 Compare September 10, 2024 15:33

pillo79 approved these changes Sep 11, 2024

View reviewed changes

teburd approved these changes Sep 12, 2024

View reviewed changes

nashif merged commit 96852bc into zephyrproject-rtos:main Sep 12, 2024
23 checks passed

lyakh deleted the depend-test branch September 13, 2024 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLEXT: dependency test #77738

LLEXT: dependency test #77738

lyakh commented Aug 29, 2024

pillo79 left a comment

pillo79 Aug 29, 2024

lyakh Aug 29, 2024

lyakh Aug 29, 2024

pillo79 Aug 29, 2024 •

edited

Loading

pillo79 Aug 29, 2024

lyakh Aug 29, 2024

lyakh commented Aug 29, 2024

pillo79 commented Aug 29, 2024

lyakh commented Aug 29, 2024

pillo79 left a comment

pillo79 Sep 4, 2024

pillo79 Sep 4, 2024

lyakh Sep 6, 2024

pillo79 Sep 6, 2024

lyakh commented Sep 6, 2024

pillo79 commented Sep 6, 2024

lyakh commented Sep 6, 2024

pillo79 left a comment

pillo79 Sep 10, 2024

lyakh Sep 10, 2024

pillo79 Sep 10, 2024

pillo79 left a comment


		void (*test_entry_fn)() = llext_find_sym(&ext_dependent->exp_tab, "test_dependent");

		zassert_not_null(test_entry_fn, "test_entry should be an exported symbol");

	K_HEAP_DEFINE(tllext_heap, 32 * 1024);
	K_HEAP_DEFINE(scratch_heap, CONFIG_LLEXT_HEAP_SIZE * 1024);

		short a = mask & (uintptr_t)test_dependent;
		short b = mask & ((uintptr_t)test_dependent >> half_ptr_bits);

LLEXT: dependency test #77738

LLEXT: dependency test #77738

Conversation

lyakh commented Aug 29, 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

pillo79 Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh commented Aug 29, 2024

pillo79 commented Aug 29, 2024

lyakh commented Aug 29, 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

lyakh commented Sep 6, 2024

pillo79 commented Sep 6, 2024

lyakh commented Sep 6, 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

pillo79 left a comment

Choose a reason for hiding this comment

pillo79 Aug 29, 2024 •

edited

Loading