core: introduce crossfile arrays (xfa)#7523
core: introduce crossfile arrays (xfa)#7523kaspar030 wants to merge 14 commits intoRIOT-OS:masterfrom
Conversation
|
I like the general idea. One word of warning though: the msp430 platforms seem to have a unique ldscript per CPU, all included in the toolchain. The maintenance cost for those boards will be higher than for the arm boards. |
core/include/xfa.h
Outdated
| #define _XFA(type, name, prio) __attribute__((used)) \ | ||
| __attribute__((section(".xfa." #name "." #prio))) \ | ||
| XFA_EXTRA \ | ||
| __attribute__((aligned(__alignof__(type)))) |
There was a problem hiding this comment.
I still wonder if the alignment is necessary or if the compiler will do this anyway. But seems to me it is not quite trivial to write a valid test app for this?!
There was a problem hiding this comment.
Honestly, I don't remember. I did have oddly aligned data at some point while testing. I'll remove that for now.
core/include/xfa.h
Outdated
| #define XFA_H | ||
|
|
||
| #if defined(__arm__) | ||
| #define XFA_EXTRA volatile |
There was a problem hiding this comment.
out of interest: what is the rationale for making the xfa entries volatile on ARM platforms?
There was a problem hiding this comment.
Again, I don't remember, and I cannot see any difference without anymore. Removed.
For the atmega, I extracted the used linker script using |
core/include/xfa.h
Outdated
| * | ||
| * @internal | ||
| */ | ||
| #define _XFA(type, name, prio) __attribute__((used)) \ |
There was a problem hiding this comment.
nice, now we can remove type from this internal funciton, making the XFA var declaration more intuitive, right?
--> XFA(name_me, 23) int foo = 123;
|
We could maybe drop the priority parameter, as entries get sorted by name anyways. Priority can be expressed by prepending the symbol name with the priotity, an a "_" prefix as digits are not allowed as first character in a symbol name. |
core/include/xfa.h
Outdated
| * @internal | ||
| */ | ||
| #define _XFA(name, prio) __attribute__((used)) \ | ||
| __attribute__((section(".xfa." #name "." #prio))) |
There was a problem hiding this comment.
why not simply, __attribute__((used, section(...)))?
| const _XFA(name, 0_) type name [0] = {}; \ | ||
| const _XFA(name, 9_) type name ## _end [0] = {}; \ | ||
| _Pragma("GCC diagnostic pop") \ | ||
| extern unsigned __xfa_dummy |
There was a problem hiding this comment.
Without the dummy, gcc complains about an extra ;:
XFA_INIT(xfatest_t, xfatest);```
core/include/xfa.h
Outdated
| * | ||
| * Use instead of type in variable definition, e.g.: | ||
| * | ||
| * XFA(driver_params, 0, driver_params_t) _onboard = { .pin=42 }; |
There was a problem hiding this comment.
this needs to be adapted to current definition
|
I like the idea. Do you see any use case for the priority field? |
I added for auto init, where some things must be initialized after others. But I think I prefer using the sorted symbol name instead of the extra parameter. Will change... |
Unfortunately this doesn't work, the linker only sorts section names. That's wher the "prio" parameter goes to allow some sorting. |
|
I was thinking about some use cases for I am however not sure yet, what the best way for integrating this into the interface would be. I guess it comes down to (i) separate function (macro) calls or (ii) an additional parameter - I don't have a strong opinion (or better: none at all)... |
|
I also just played with simplifying // auto_init_netdev_tap.c
AUTO_INIT(auto_init_netdev_tap, 1);using // auto_init.h
typedef void(*auto_init_t)(void);
#define AUTO_INIT(func, prio) XFA(auto_init_list, prio) auto_init_t func ## _foo = &funcBUT this does not quite work with the known problem on the linker dropping the |
|
Does it work if you put the auto init in the same file as the driver? |
You mean other than #5757? |
Yes, I thought let's get in flash XFAs and then go from there...
I think it'll have to be different function macros. In order to end up in RAM, we'd need another linker section which ends up in RAM, which would need a different name. |
BTW, one way to do that is to only insert the pointer to a device descriptor to a (flash-based) xfa. |
Brrrrr... |
|
To be more verbose: I could live with the linkerscript files, but the macros really hurt me. |
|
For the record: I don't want to be just destructive here and will try to think about a different solution. |
That is totally valid. But please, let's put a timebox on that. I've spent quite some time finding a solution with minimal uglyness. Unfortunately, C being compilation unit based is not very helpful. I experimented with code generation (#4598), but IMO the macro based approach of this PR is preferable. |
core/include/xfa.h
Outdated
| extern const type name [0]; \ | ||
| extern const type name ## _end [0]; \ | ||
| _Pragma("GCC diagnostic pop") \ | ||
| extern unsigned __xfa_dummy |
There was a problem hiding this comment.
Here, disabling '-Wpedantic' is not necessary as it is just a symbol definition.
fixed. |
f8dd2c0 to
eb33172
Compare
|
|
@OlegHahm any news? |
|
Question, could it be possible to use the same trick as From This way, we would not need to store all boards linker scripts in the repository, just have a 'ld give me the linker script' command. |
| *(.gnu.linkonce.d*) | ||
|
|
||
| . = ALIGN(2); | ||
| KEEP (*(SORT(.xfa.*))) |
There was a problem hiding this comment.
You will need to define that in every cpu's ldscript. Maybe you could put it in a "linked" single ldscript. The main problem of what I propose is that every cpu's ldscript need to have the same interface (define the same memory areas, output sections...), which is currently not the case.
There was a problem hiding this comment.
In your branch, you have 2 different scripts, and I think it could be more than ok to have different ways on doing it for cpus familys.
There was a problem hiding this comment.
I have 2 different scripts because the linker scripts interfaces are not the same. But the features are the same. While it is ok to have differents ways to do something, it may be nice to not copy/past the same way everywhere.
|
Does it have to be in a section |
|
After some experiments I found that the default linker scripts do not sort the .rodata sections. Even when working around the garbage collection stripping everything, the array will be in disarray 😝, so it's not usable like that. |
|
Another idea, so far untested: |
You mean to have a two-stage linking process, with the first only collecting all xfa pieces? |
|
Yes, but it doesn't look like it will work without KEEP() in the ldscripts. However, a different solution using an augmentation ldscript with -Txfa.ld and using INSERT BEFORE in xfa.ld looks like it will work. |
INSERT BEFORE instead of INSERT AFTER (like I did for native)? |
|
Nevermind, I am writing from my phone and I didn't look at the code. |
|
This needs a rebase. |
|
@cladmi @kaspar030 The patch ldscript approach does seem to work for other platforms than native, though if the main ldscript is also specified with -T on the command line, the patch -T ldscript will have to be specified before the main -T ldscript on the ld command line. Otherwise we get the cryptic error message ".data not found for insert". Edit: This is basically what @astralien3000 wrote in the comment above regarding the shell ldscripts |
|
We talked about it on Friday with @jia200x, he may have some time to work on this. I thought, one required thing, which would help, is a test that fails at compile time if it is not working. This way all new boards MUST make it work to let murdock run and we could get a list of TODOs boards. |
|
For static testing, we could write a shell script which extracts the addresses of the xfa marker symbols from the elf file and check that they are not equal. |
|
I have created a rebase for this PR and also found a solution to the patch ldscript that seem to work for all platforms, except for pic32 which are giving me linker errors right now. It needs to be tested though to ensure that the linking is correct, in particular with regards to the data relocation during boot. I'll post later tonight or tomorrow. |
|
Closed in favour of #9105. |
At many places, we're hitting a problem with C: arrays have to be defined within one compilation unit. The language itself has no way of doing otherwise, unless the linker helps a little.
This PR adds some macros and the added linker script entries in order to allow adding members to a static array from different compilation units. See the included tests/xfa for an example.
The main idea is that from anywhere, code can do sth like this:
XFA(mydriver_params_t, 0, mydriver_params) onboard = { .spi=blah, .foo=bar}XFA(mydriver_params, 0) mydriver_params_t onboard = { .spi=blah, .foo=bar}This would add
onboardto the linker section .xfa.mydrivers_params.Entries in those sections are sorted by priority (
0in this case) and name (onboardin this case).Something like that would be very convenient for:
We've always tried to avoid "linker script black magic", but after years of dodging (and coming up with schemes like in #7519) and having unified the ldscripts per platform, I think we can afford adding those two lines.
I see this PR as a proof-of-concept and I'm happy to hear your opinion!
Currently only native, atmega2560 and cortexm have the necessary linker script support.
Waiting for #5757 (needs ugly hacks without, look for hack[12] in the example).
Solves #4290, #7519 and some others.
Todo: