-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: introduce crossfile arrays (xfa) #7523
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of interest: what is the rationale for making the xfa entries volatile on ARM platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simply, __attribute__((used, section(...)))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this dummy required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be adapted to current definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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 = &func BUT 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
onboard
to the linker section .xfa.mydrivers_params.Entries in those sections are sorted by priority (
0
in this case) and name (onboard
in 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: