llext: convert "modules" tests to sample #74060

pillo79 · 2024-06-11T08:53:52Z

The current llext test suite contains a number of tests that are called modules_enabled and were originally added in #67105 as a way to showcase the Kconfig tristate element type; those have since been expanded to 8 tests covering all current llext option combinations.

This abundance of tests, however, is not useful: since the different Kconfig options have no practical effect on the llext code, all those tests perform exactly like the non-modules-enabled configuration and just double the testing effort and number of configurations in testcase.yaml.

This PR replaces those redundant tests with a sample that shows how to actually create a "module" in the Linux kernel sense - a piece of code that can be either linked in or loaded at runtime.

teburd

Thank you for doing this, and yes this makes great sense to not repeat the same work over and over for showing how the tristate Kconfig works.

@lyakh should run this with qemu_xtensa at the very least and approve before merging, marking DNM (with my +1 here) until @lyakh does just that. Feel free to remove DNM once reviewed and approved by @lyakh.

kartben

this is awesome stuff - thanks! Some minor nits/typos in case this goes through another round of updates.

kartben · 2024-06-12T14:02:05Z

samples/subsys/llext/modules/README.rst

+their symbols can be accessed and functions called.
+
+Specifically, this shows how to call a simple "hello world" function,
+implemented in ``src/hello_world_ext.c``. This is achieved in two different ways,


nit (i.e. not worth updating unless there's another round of reviews needed): maybe make this a :zephyr_file: like you're doing for others

kartben · 2024-06-12T14:04:07Z

samples/subsys/llext/modules/README.rst

+   :relevant-api: llext
+
+    Call a function in a loadable extension module,
+    either built in or loaded at runtime.


Suggested change

either built in or loaded at runtime.

either built-in or loaded at runtime.

kartben · 2024-06-12T14:06:22Z

samples/subsys/llext/modules/Kconfig

+	default m
+	help
+	  This enables calling the hello_world function, implemented in
+	  hello_world_ext.c, either as an llext module or as a built in part of


Suggested change

hello_world_ext.c, either as an llext module or as a built in part of

hello_world_ext.c, either as an llext module or as a built-in part of

lyakh · 2024-06-12T14:09:16Z

samples/subsys/llext/modules/sample.yaml

+tests:
+  sample.llext.modules_enabled:
+    extra_configs:
+      - CONFIG_MODULES=y


just to make sure this is correct - it's also set in prj.conf and left unchanged below in the monolithic case - is that correct?

Thanks, you are right, it's an useless line. The point is to have MODULES always enabled during this test so I will remove this.

lyakh · 2024-06-12T14:18:22Z

tests/subsys/llext/simple/testcase.yaml

-    extra_configs:
-      - CONFIG_MODULES=y
-      - CONFIG_LLEXT_STORAGE_WRITABLE=y
-      - CONFIG_LLEXT_TYPE_ELF_RELOCATABLE=y


as an example, I looked for another CONFIG_LLEXT_STORAGE_WRITABLE=y && CONFIG_LLEXT_TYPE_ELF_RELOCATABLE=y test here since you're saying, that you're removing copies, and I haven't found such a test, am I missing something?

No, you're right - that was a proper mistake. 🙂
I will make sure to check all removed paths and convert them to non-MODULES in case they were not duplicates.

mathieuchopstm

Looks nice! A few points that I think need to be addressed/checked:

mathieuchopstm · 2024-06-12T14:02:37Z

tests/subsys/llext/simple/testcase.yaml

-      - simulation
-      - arch
-    platform_exclude:
-      - qemu_cortex_a9 # MMU


Cortex-A9 platform exclusion seems gone in sample.yaml.
Is this on purpose?

Yes, that exclusion was there before the runtime filter was added, so that platform should be caught by the above filter anyway. I will double-check this before submitting v2 though.

mathieuchopstm · 2024-06-12T14:07:11Z

samples/subsys/llext/modules/sample.yaml

+  filter: not CONFIG_MPU and not CONFIG_MMU and not CONFIG_SOC_SERIES_S32ZE
+  platform_exclude:
+    - apollo4p_evb
+    - apollo4p_blue_kxr_evb


Please add a comment referring to issues 72775 / 73443 for apollo platforms, to make the reason why they are excluded obvious (as done for the numaker right below).

mathieuchopstm · 2024-06-12T14:11:44Z

samples/subsys/llext/modules/src/hello_world_ext.c

+{
+	printk("Hello, world!\n");
+}
+LL_EXTENSION_SYMBOL(hello_world);


If CONFIG_LLEXT_HELLO_WORLD=y, this file will be built as part of the Zephyr application. However, there is nothing special in the LL_EXTENSION_SYMBOL macro that makes it not emit the export table entry.
Is there no risk to blow up CI with orphan section linker warnings due to something being emitted to the .exported_sym section which, AFAICT, is not handled in Zephyr linker script?

Interesting... it did not in my tests, but I can't be sure it won't everywhere. I will fix this as we did for the EXPORT_SYMBOL macro: make it a NOP if LLEXT is not enabled, and disable LLEXT for the =y case.

I'm not sure this would work in this case though; AFAICT, CONFIG_LLEXT is always y for this sample. Maybe switching according to the tristate's value would work though (but I don't know what CONFIG_LLEXT_HELLO_WORLD=m yields once converted to C macro...)

mathieuchopstm · 2024-06-12T14:15:36Z

samples/subsys/llext/modules/prj.conf

+CONFIG_LOG=y
+CONFIG_LOG_MODE_IMMEDIATE=y
+
+CONFIG_MODULES=y


Nit, and definitely out of this PR's scope: I find this Kconfig option's name quite confusing at first glance.

Indeed it is. We can't change it unless we modify kconfiglib internals, though, since that is the name that triggers the whole "tristate" stuff in Kconfig. 🙁

mathieuchopstm · 2024-06-12T14:16:06Z

samples/subsys/llext/modules/src/main_builtin.c

+LOG_MODULE_REGISTER(app);
+
+#include <zephyr/llext/llext.h>
+#include <zephyr/llext/buf_loader.h>


These includes seem unneeded.

pillo79

Thanks everyone for your comments, will try to address all of them 🙂

pillo79 · 2024-06-12T14:23:52Z

samples/subsys/llext/modules/prj.conf

+CONFIG_LOG=y
+CONFIG_LOG_MODE_IMMEDIATE=y
+
+CONFIG_MODULES=y


Indeed it is. We can't change it unless we modify kconfiglib internals, though, since that is the name that triggers the whole "tristate" stuff in Kconfig. 🙁

pillo79 · 2024-06-12T14:26:30Z

samples/subsys/llext/modules/sample.yaml

+tests:
+  sample.llext.modules_enabled:
+    extra_configs:
+      - CONFIG_MODULES=y


Thanks, you are right, it's an useless line. The point is to have MODULES always enabled during this test so I will remove this.

pillo79 · 2024-06-12T14:29:09Z

samples/subsys/llext/modules/src/hello_world_ext.c

+{
+	printk("Hello, world!\n");
+}
+LL_EXTENSION_SYMBOL(hello_world);


Interesting... it did not in my tests, but I can't be sure it won't everywhere. I will fix this as we did for the EXPORT_SYMBOL macro: make it a NOP if LLEXT is not enabled, and disable LLEXT for the =y case.

pillo79 · 2024-06-12T14:31:06Z

tests/subsys/llext/simple/testcase.yaml

-      - simulation
-      - arch
-    platform_exclude:
-      - qemu_cortex_a9 # MMU


Yes, that exclusion was there before the runtime filter was added, so that platform should be caught by the above filter anyway. I will double-check this before submitting v2 though.

pillo79 · 2024-06-12T14:33:31Z

tests/subsys/llext/simple/testcase.yaml

-    extra_configs:
-      - CONFIG_MODULES=y
-      - CONFIG_LLEXT_STORAGE_WRITABLE=y
-      - CONFIG_LLEXT_TYPE_ELF_RELOCATABLE=y


No, you're right - that was a proper mistake. 🙂
I will make sure to check all removed paths and convert them to non-MODULES in case they were not duplicates.

mathieuchopstm

LGTM.

mathieuchopstm · 2024-06-13T07:34:21Z

samples/subsys/llext/modules/README.rst

+   :zephyr-app: samples/subsys/llext/modules
+   :board: qemu_xtensa
+   :goals: build run
+   :gen-args: -DCONFIG_LLEXT_HELLO_WORLD=y


Shouldn't this be -T sample.llext.modules_disabled instead?

mathieuchopstm · 2024-06-13T07:34:23Z

samples/subsys/llext/modules/README.rst

+   :zephyr-app: samples/subsys/llext/modules
+   :board: qemu_xtensa
+   :goals: build run
+   :gen-args: -DCONFIG_LLEXT_HELLO_WORLD=m


Shouldn't this be -T sample.llext.modules_enabled instead?

The LL_EXTENSION_SYMBOL macro is used to export a symbol to the base image. When CONFIG_LLEXT is not defined, or the file is being compiled outside of an llext, the macro is not useful and would leave orphan sections in the final image instead. This patch adds the LL_EXTENSION_BUILD definition to the llext build process, and uses it to stub out the symbol-defining macro when not building an llext. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This adds a new sample to demonstrate the use of tristate symbols in Kconfig to build a function as an llext module or as a built-in part of Zephyr. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

These tests are simple copies of the existing tests with the addition of the CONFIG_MODULES=y option. This different Kconfig setting has no practical effect on the code, and the tests are therefore redundant. Remove them to halve the number of tests. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 · 2024-06-13T14:24:34Z

Thanks @mathieuchopstm for making me think a bit more about what we are trying to explain here.

v2:

applied all review comments;
added a way to detect that an llext build is in progress [1], and use it to stub out LL_EXTENSION_SYMBOL when the extension source file is included directly in the Zephyr build;
renamed CONFIG_LLEXT_HELLO_WORLD to CONFIG_HELLO_WORLD_MODE and improved descriptions;
made the test output different strings depending on the build type and cross-check them.

Note [1]: the first commit adds an LL_EXTENSION_BUILD symbol that is only defined during an llext compilation. This is currently not exported to the EDK; ~~I have found some issues there and would like to tackle them in a related PR.~~ EDIT: Issues already fixed on main via #73286, so I will open a separate PR for the missing symbol after this is merged.

Ready for review/test as soon as CI turns this way... 🙂

teburd · 2024-06-14T13:33:30Z

@mathieuchopstm did you accept the invite? should have a green checkbox

mathieuchopstm · 2024-06-14T13:35:17Z

@mathieuchopstm did you accept the invite? should have a green checkbox

Pretty sure I did, I'm confused as well 🤔

EDIT: and as a matter of fact, in my GitHub security log, I do have entries for repository_invitation.accept and org.add_outside_collaborator.

pillo79 force-pushed the llext-module-sample branch 4 times, most recently from 8e277c4 to b0a6452 Compare June 12, 2024 10:42

teburd previously approved these changes Jun 12, 2024

View reviewed changes

teburd added this to the v3.7.0 milestone Jun 12, 2024

teburd added the DNM This PR should not be merged (Do Not Merge) label Jun 12, 2024

teburd self-assigned this Jun 12, 2024

teburd requested review from lyakh and mathieuchopstm June 12, 2024 13:57

pillo79 marked this pull request as ready for review June 12, 2024 13:59

zephyrbot added area: Samples Samples area: Linkable Loadable Extensions labels Jun 12, 2024

zephyrbot requested review from kartben and nashif June 12, 2024 14:00

teburd requested a review from bjarki-andreasen June 12, 2024 14:00

kartben previously approved these changes Jun 12, 2024

View reviewed changes

lyakh reviewed Jun 12, 2024

View reviewed changes

mathieuchopstm requested changes Jun 12, 2024

View reviewed changes

pillo79 commented Jun 12, 2024

View reviewed changes

pillo79 dismissed stale reviews from kartben and teburd via c4d58b2 June 12, 2024 16:37

pillo79 force-pushed the llext-module-sample branch from b0a6452 to c4d58b2 Compare June 12, 2024 16:37

mathieuchopstm approved these changes Jun 13, 2024

View reviewed changes

pillo79 force-pushed the llext-module-sample branch from c4d58b2 to 4c850da Compare June 13, 2024 10:57

zephyrbot added the area: Build System label Jun 13, 2024

zephyrbot requested review from 57300, jeremybettis, nordicjm and tejlmand June 13, 2024 10:58

pillo79 force-pushed the llext-module-sample branch from 4c850da to 594d8aa Compare June 13, 2024 14:07

pillo79 added 3 commits June 13, 2024 16:12

llext: add a "modules" Kconfig tristate example

59931ec

This adds a new sample to demonstrate the use of tristate symbols in Kconfig to build a function as an llext module or as a built-in part of Zephyr. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 force-pushed the llext-module-sample branch from 594d8aa to 9eb02d1 Compare June 13, 2024 14:13

pillo79 mentioned this pull request Jun 13, 2024

check_compliance: allow _MODULE variant of Kconfig options #74258

Merged

pillo79 requested review from lyakh, kartben and teburd June 13, 2024 16:11

mathieuchopstm approved these changes Jun 14, 2024

View reviewed changes

bjarki-andreasen approved these changes Jun 14, 2024

View reviewed changes

teburd approved these changes Jun 14, 2024

View reviewed changes

teburd removed the DNM This PR should not be merged (Do Not Merge) label Jun 14, 2024

kartben approved these changes Jun 14, 2024

View reviewed changes

aescolar merged commit 6db4844 into zephyrproject-rtos:main Jun 14, 2024
29 of 30 checks passed

katgiadla mentioned this pull request Jun 19, 2024

samples: subsys: llext: modules: sample.llext.modules.module_build reports overflow #74536

Closed

henrikbrixandersen added the area: llext Linkable Loadable Extensions label Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llext: convert "modules" tests to sample #74060

llext: convert "modules" tests to sample #74060

pillo79 commented Jun 11, 2024

teburd left a comment

kartben left a comment •

edited

Loading

kartben Jun 12, 2024

kartben Jun 12, 2024

kartben Jun 12, 2024

lyakh Jun 12, 2024

pillo79 Jun 12, 2024

lyakh Jun 12, 2024

pillo79 Jun 12, 2024

mathieuchopstm left a comment

mathieuchopstm Jun 12, 2024

pillo79 Jun 12, 2024

mathieuchopstm Jun 12, 2024

mathieuchopstm Jun 12, 2024

pillo79 Jun 12, 2024

mathieuchopstm Jun 12, 2024

mathieuchopstm Jun 12, 2024

pillo79 Jun 12, 2024

mathieuchopstm Jun 12, 2024

pillo79 left a comment

pillo79 Jun 12, 2024

pillo79 Jun 12, 2024

pillo79 Jun 12, 2024

pillo79 Jun 12, 2024

pillo79 Jun 12, 2024

mathieuchopstm left a comment

mathieuchopstm Jun 13, 2024

mathieuchopstm Jun 13, 2024

pillo79 commented Jun 13, 2024 •

edited

Loading

teburd commented Jun 14, 2024

mathieuchopstm commented Jun 14, 2024 •

edited

Loading

	either built in or loaded at runtime.
	either built-in or loaded at runtime.

	hello_world_ext.c, either as an llext module or as a built in part of
	hello_world_ext.c, either as an llext module or as a built-in part of

llext: convert "modules" tests to sample #74060

llext: convert "modules" tests to sample #74060

Conversation

pillo79 commented Jun 11, 2024

teburd left a comment

Choose a reason for hiding this comment

kartben left a comment • 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

Choose a reason for hiding this comment

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

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 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

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

pillo79 commented Jun 13, 2024 • edited Loading

teburd commented Jun 14, 2024

mathieuchopstm commented Jun 14, 2024 • edited Loading

kartben left a comment •

edited

Loading

pillo79 commented Jun 13, 2024 •

edited

Loading

mathieuchopstm commented Jun 14, 2024 •

edited

Loading