LLEXT: Relocatable fixes

[lyakh]

two fixes for relocatable LLEXT objects. This should fix errors in #72383

[pillo79]

Thanks for the debug, LGTM, just one small refactor request.

[pillo79]

This code is duplicated, can you please refactor so the calculation and log is done once outside the if?
Also, note the commit that introduces it has a spelling issue in the commit title.

[lyakh]

@pillo79 @teburd they aren't duplicated. Both the sym_tab->syms[j].addr assignment and the log entry are different. I can unify the logging call by dropping the difference, but as of now they're different.

[pillo79]

The only difference in the address calculation (arguably the most important line in the whole function!) is just the name of one argument; in the log, there is a section name that could be calculated for the other case.
I really see no point in having 2 code paths that do the same logical thing in different ways.

[teburd]

Minor typo in commit s/stanrads/standard

changes look good to me

[teburd]

Sorry I maybe wasn't clear enough where I saw the typo, the commit message for 2235022 has
"llext: fix handling of non-stanrad sections in relocatable case"

github doesn't necessarily let me comment on a commit message, boooo

[lyakh]

Sorry I maybe wasn't clear enough where I saw the typo, the commit message for 2235022 has "llext: fix handling of non-stanrad sections in relocatable case"

github doesn't necessarily let me comment on a commit message, boooo

@teburd arrrgh, you were perfectly clear, it's just that I cannot type

[teburd]

Minor nits still, but the change looks ok to me otherwise

[pillo79]

I love the idea of being able to respect the ELF-provided addresses, but IMO we really need a test case for this new feature in Zephyr. I don't want to have to guess if any of the patches I submit are breaking SOF... (or even worse, hear screaming coming that way 😅)

[pillo79]

I think this should be:

Suggested change

	sym_tab->syms[j].addr = (uint8_t *)sym.st_value +
	(ldr->hdr.e_type == ET_REL ? section_addr : 0);
	sym_tab->syms[j].addr = (uint8_t *)section_addr
	+ sym.st_value;

because in the "pre-located" case the symbols'VMA should not be modified, so the calculation is the same regardless of dynamic or static linking.

[lyakh]

@pillo79 no, the difference between ET_REL (relocatable object) and ET_DYN (shared object) remains. In the relocatable case that calculation is correct:

[  113.934401] <inf> llext: llext_copy_symbols: type: 1 sym volume_init: 0x1e8c section 0xa068a000
[  113.934410] <inf> llext: llext_copy_symbols: type: 1 sym volume_func_count: 0x24 section 0xa068e000
[  113.934420] <inf> llext: llext_copy_symbols: type: 1 sym volume_reset_state: 0xf50 section 0xa068a000

but in the shared case it's this:

[  150.092295] <inf> llext: llext_copy_symbols: type: 3 sym volume_init: 0xa068ba5c section 0xa068a000
[  150.092306] <inf> llext: llext_copy_symbols: type: 3 sym volume_func_count: 0xa068d0b0 section 0xa068d000
[  150.092318] <inf> llext: llext_copy_symbols: type: 3 sym volume_reset_state: 0xa068b510 section 0xa068a000

[pillo79]

You're right, that is indeed why it's subtracted in the else case!

[pillo79]

I think something like "Use the virtual symbol addresses from the ELF instead of the run-time ones" might be clearer here.

[lyakh]

I love the idea of being able to respect the ELF-provided addresses, but IMO we really need a test case for this new feature in Zephyr.

@pillo79 testing is good... We could add a test where an LLEXT is built for a fixed address, but for which one? You'd probably need to modify a linker script to assign a fixed address range for that extension? And that would be testing / twister-specific, whereas now standard scripts, e.g. soc/cdns/dc233c/include/xtensa-dc233c.ld are used... Any ideas?

[pillo79]

We could add a test where an LLEXT is built for a fixed address, but for which one?

You could use the SOF strategy and add a custom re-link step that only fixes an integer variable to 0xdeadbeef, then just check its final address from the test. (EDIT: If partial placement is not feasible, limit the test to the qemu-xtensa arch only). WDYT?

[lyakh]

We could add a test where an LLEXT is built for a fixed address, but for which one?

You could use the SOF strategy and add a custom re-link step that only fixes an integer variable to 0xdeadbeef, then just check its final address from the test. (EDIT: If partial placement is not feasible, limit the test to the qemu-xtensa arch only). WDYT?

@pillo79 not sure I understand. We need to build an LLEXT with some predefined address, similar to how we do for SOF by using gcc -Wl,-Tdata=0x........ right? And then QEMU must be able to access that address. So you could do something like:

1. in test_llext_simple.c allocate space for .data from the llext object like uint8_t obj_data[4096];
2. build everything
3. check zephyr.elf and find the address of obj_data
4. re-link LLEXT with -Wl,-Tdata=...
5. load that LLEXT, copy .data to that array, maybe delete the original .data to make sure the correct one is used...

So, not quite trivial unless there's a simpler way

[pillo79]

LGTM, will merge the test from #73479 after this.