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) v3 #15002

Merged
merged 27 commits into from
Feb 18, 2021
Merged

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Sep 10, 2020

Contribution description

This PR is the third iteration of Cross File Arrays (xfa), which allow typed and sorted arrays to be spread over multiple .c files.

This is basically a rebased version of #9105.

What can this be used for?

This could be useful to organize auto_init functions.

Currently, they're centrally managed in a single auto_init. file (well, multiple if auto_init_saul, _netif etc. should be counted).

With XFA's it is possible to add an initialization function to a global "init_functions" array, within the module's .c file:

 XFA_ADD_PTR(init_functions, 1234, ztimer, &ztimer_init)

auto_init.c could be reduced to this:

XFA_CONST_USE(init_function_t, init_functions);

void auto_init() {
    unsigned n = XFA_LEN(init_function_t, init_functions);
    for (unsigned i = 0; i < n; i++) {
		init_functions[i]();
    }
}

Compared to libc_init_array, XFA's support priority values. All entries get sorted by priority value, at link time.
In the above example, the ztimer init function was added with priority 100. Anything requiring ztimer would need to use a priority value that is higher than that.
The priority space is basically unlimited, the numbers get turned into symbol names, which the linker sorts. (note to self: see if that sort is numerical, or maybe 10 sorts before 2).

This is also not limited to functions. Other candidates for benefiting from non-central arrays are:

  • shell commands
  • saul drivers
  • netif drivers
  • threads that should started at boot time

Downsides

Apart from being a bit macro hacky, this currently requires disabling -Warray-bounds for files using XFA's (that include xfa.h).
Also, this needs linker script support. A generic xfa.ld is used unless overridden. Some architectures need slightly different handling due to different section names or linker symbols.

Testing procedure

As is, XFA's are not used anywhere other than in the test.
Ensure that test works, on all platforms.

Issues/PRs references

Taken over #9105.
Initially introduced in #7523.
Not needing ugly hacks since #14754.

@kaspar030 kaspar030 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Sep 10, 2020
@maribu
Copy link
Member

maribu commented Sep 10, 2020

Apart from being a bit macro hacky, this currently requires disabling -Warray-bounds for files using XFA's

Would it be possible to add a static inline helper function for accessing the arrays? If so, maybe some #pragma GCC diagnostics dance around that helper function would do?

@kaspar030
Copy link
Contributor Author

Would it be possible to add a static inline helper function for accessing the arrays? If so, maybe some #pragma GCC diagnostics dance around that helper function would do?

it probably would. also, this could make it possible to not force avr to do this in completely RAM. It would not make it more ergonomic, I think. will need to try.

@kaspar030
Copy link
Contributor Author

I don't manage to adapt the xfa.ld to the internal msp430 linker script. :( some help would be appreciated.

I see some options:

  • someone with more ld-script-fu figures out how to do it
  • we extract the msp430 ld scripts and add them to our repo, then add the xfa support there (there are like 5 different ones)
  • we make xfa a feature for now, excluding msp430, making it much less usable

@bergzand bergzand added this to the Release 2021.01 milestone Sep 23, 2020
@maribu
Copy link
Member

maribu commented Sep 23, 2020

The ld-script-force is strong with @benpicco. Maybe he can help?

@benpicco
Copy link
Contributor

I don't have the hardware to test, but when I drop d50d0b6 and apply this patch

diff --git a/makefiles/arch/msp430.inc.mk b/makefiles/arch/msp430.inc.mk
index 011e2778ea..2210919568 100644
--- a/makefiles/arch/msp430.inc.mk
+++ b/makefiles/arch/msp430.inc.mk
@@ -15,6 +15,7 @@ ASFLAGS += $(CFLAGS_CPU) --defsym $(CPU_MODEL)=1 $(CFLAGS_DBG)
 
 LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT)
 LINKFLAGS += -Wl,--gc-sections -Wl,-L$(MSP430_SUPPORT_FILES)/include
+LINKFLAGS += -T $(MSP430_SUPPORT_FILES)/include/$(CPU_MODEL).ld
 
 OPTIONAL_CFLAGS_BLACKLIST += -fdiagnostics-color
 OPTIONAL_CFLAGS_BLACKLIST += -Wformat-overflow

it will at least compile for msp430

@chrysn chrysn mentioned this pull request Oct 26, 2020
@jia200x
Copy link
Member

jia200x commented Oct 26, 2020

I don't have the hardware to test, but when I drop d50d0b6 and apply this patch

I have a z1 lying around. I will try it

@kaspar030
Copy link
Contributor Author

Seems like the riotdocker llvm is too old for the static test:

llvm-objdump: Unknown command line argument '--syms'.  Try: '/usr/bin/llvm-objdump -help'
llvm-objdump: Did you mean '-dsym'?
llvm-objdump: Unknown command line argument '--syms'.  Try: '/usr/bin/llvm-objdump -help'
llvm-objdump: Did you mean '-dsym'?
Error: Static check of XFA linked const array failed, verify linker flags and try again

The test succeeds with llvm 11, but not with the one in our build container.

@kaspar030
Copy link
Contributor Author

@kaspar030 Murdock is idle, good time to address those last couple of nitpicks ;)

done & squashed

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK!

@kaspar030 kaspar030 merged commit 15124e4 into RIOT-OS:master Feb 18, 2021
@kaspar030 kaspar030 deleted the pr/xfa_v3 branch February 18, 2021 13:49
@kaspar030
Copy link
Contributor Author

Thanks a lot everyone involved and especially @fjmolinas for reviewing, and for keeping me looking at this!

@miri64
Copy link
Member

miri64 commented Feb 18, 2021

🎉

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! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Release notes: added Set on PRs that have been processed into the release notes for the current release. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants