Skip to content
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

Closed
wants to merge 14 commits into from

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Aug 29, 2017

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:

  • auto-init functions (declare them with the code, not in sys/auto_init/...)
  • shell commands (same, no more sys/shell/commands.c)
  • driver parameters (board adds onboard params to params array, user can simply add more)
  • saul drivers

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:

  • merge make: fix linking #5757
  • add linkerscripts for msp430*
  • add linkerscripts for remaining AVRs
  • add linkerscripts for MIPS

@kaspar030 kaspar030 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: waiting for other PR State: The PR requires another PR to be merged first State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Aug 29, 2017
@jnohlgard
Copy link
Member

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.

#define _XFA(type, name, prio) __attribute__((used)) \
__attribute__((section(".xfa." #name "." #prio))) \
XFA_EXTRA \
__attribute__((aligned(__alignof__(type))))
Copy link
Contributor

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

Copy link
Contributor Author

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.

#define XFA_H

#if defined(__arm__)
#define XFA_EXTRA volatile
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@kaspar030
Copy link
Contributor Author

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.

For the atmega, I extracted the used linker script using gcc -Wl,-verbose. We'd have to do that (once) for all avr and msp430 MCUs...

*
* @internal
*/
#define _XFA(type, name, prio) __attribute__((used)) \
Copy link
Contributor

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@kaspar030
Copy link
Contributor Author

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.

* @internal
*/
#define _XFA(name, prio) __attribute__((used)) \
__attribute__((section(".xfa." #name "." #prio)))
Copy link
Member

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(...)))?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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);```

*
* Use instead of type in variable definition, e.g.:
*
* XFA(driver_params, 0, driver_params_t) _onboard = { .pin=42 };
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lebrush
Copy link
Member

lebrush commented Sep 1, 2017

I like the idea. Do you see any use case for the priority field?

@kaspar030
Copy link
Contributor Author

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

@kaspar030
Copy link
Contributor Author

We could maybe drop the priority parameter, as entries get sorted by name anyways.

Unfortunately this doesn't work, the linker only sorts section names. That's wher the "prio" parameter goes to allow some sorting.

@haukepetersen
Copy link
Contributor

I was thinking about some use cases for xfa and have an additional feature request: I would like to be also be able to allocate cross file arrays in RAM. This would help to generalize e.g. the device driver initialization by being able to pre-allocate device descriptors during compile time.

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

@haukepetersen
Copy link
Contributor

I also just played with simplifying auto_init using xfa. In theory, this is very nice and boils down to e.g.

// 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 auto_init_netdev_tap.c compilation unit as there is nothing else referenced in this file. Do we have any ideas on how we could address this issue in a clean and scalable way?

@jnohlgard
Copy link
Member

Does it work if you put the auto init in the same file as the driver?

@kaspar030
Copy link
Contributor Author

Do we have any ideas on how we could address this issue in a clean and scalable way?

You mean other than #5757?

@kaspar030
Copy link
Contributor Author

I would like to be also be able to allocate cross file arrays in RAM. This would help to generalize e.g. the device driver initialization by being able to pre-allocate device descriptors during compile time.

Yes, I thought let's get in flash XFAs and then go from there...

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

@kaspar030
Copy link
Contributor Author

I would like to be also be able to allocate cross file arrays in RAM.

BTW, one way to do that is to only insert the pointer to a device descriptor to a (flash-based) xfa.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 3, 2017

This PR adds some macros and the added linker script entries

Brrrrr...

@OlegHahm
Copy link
Member

OlegHahm commented Oct 3, 2017

To be more verbose: I could live with the linkerscript files, but the macros really hurt me.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 4, 2017

For the record: I don't want to be just destructive here and will try to think about a different solution.

@kaspar030
Copy link
Contributor Author

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.

extern const type name [0]; \
extern const type name ## _end [0]; \
_Pragma("GCC diagnostic pop") \
extern unsigned __xfa_dummy
Copy link
Contributor

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.

@kaspar030
Copy link
Contributor Author

Here, disabling '-Wpedantic' is not necessary as it is just a symbol definition.

fixed.

@kaspar030 kaspar030 force-pushed the introduce_crossfile_arrays branch from f8dd2c0 to eb33172 Compare November 6, 2017 22:05
@kaspar030
Copy link
Contributor Author

  • rebased

@kaspar030
Copy link
Contributor Author

@OlegHahm any news?

@cladmi
Copy link
Contributor

cladmi commented Feb 8, 2018

Question, could it be possible to use the same trick as native with a "patch ldscript" for all boards ?

From ld man page, I understand that we would need to do something like -T/path/to/default_script -Txfa_ldscript.

       -T scriptfile
       --script=scriptfile
           Use scriptfile as the linker script.  This script replaces ld's
           default linker script (rather than adding to it), so commandfile must
           specify everything necessary to describe the output file.    If
           scriptfile does not exist in the current directory, "ld" looks for it
           in the directories specified by any preceding -L options.  Multiple
           -T options accumulate.

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.*)))
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

@jnohlgard
Copy link
Member

jnohlgard commented Feb 20, 2018

Does it have to be in a section .xfa.*? Why not placing them in a section .rodata.xfa*? That way it might work with the standard ldscripts for each platform.

@jnohlgard
Copy link
Member

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.

@jnohlgard
Copy link
Member

Another idea, so far untested:
Use the xfa_ldscript to link only all xfa sections and properly sort them into a single .rodata section, then use that resulting file in the final link to place all xfa pieces together.

@kaspar030
Copy link
Contributor Author

Use the xfa_ldscript to link only all xfa sections and properly sort them into a single .rodata section, then use that resulting file in the final link to place all xfa pieces together.

You mean to have a two-stage linking process, with the first only collecting all xfa pieces?

@jnohlgard
Copy link
Member

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.

@kaspar030
Copy link
Contributor Author

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

@jnohlgard
Copy link
Member

Nevermind, I am writing from my phone and I didn't look at the code.

@jnohlgard
Copy link
Member

This needs a rebase.
How do we proceed to get this ready for merging?
I think we need to have this feature on all archs, or else it will be useless for many use cases (e.g. auto_init) because of the workaround which will have to be written for every general case.

@jnohlgard
Copy link
Member

jnohlgard commented May 5, 2018

@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".
To get the XFA array placed in the correct segment it may be necessary to specify the segment in the patch ldscript, which means that we need a separate patch ldscript for each arch. This shouldn't be a big issue though since the XFA script is fairly uncomplicated and should remain quite static for future maintenance.

Edit: This is basically what @astralien3000 wrote in the comment above regarding the shell ldscripts

@cladmi
Copy link
Contributor

cladmi commented May 7, 2018

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.
A like time assert verifying that the array has not a zero size would be perfect for this.

@jnohlgard
Copy link
Member

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.

@jnohlgard
Copy link
Member

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.

@kaspar030
Copy link
Contributor Author

Closed in favour of #9105.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully State: waiting for other PR State: The PR requires another PR to be merged first State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants