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), refactored #9105

Closed
wants to merge 27 commits into from

Conversation

jnohlgard
Copy link
Member

Contribution description

A rebase of #7523. The alternate approach used here is to avoid introducing platform specific ldscripts and instead use the same approach as was used for the native platform in #7523. The only exception here is the AVR arch which needs a specific version of the patch ldscript to ensure that the xfa sections are placed where the code can use them. The AVR is a Harvard architecture which means that we can't simply place data in the code memory segment without adding extra code for accessing it.

TODO: Static check of linking xfa sections as suggested in #7523 (comment)

Issues/PRs references

rebase/refresh of #7523
based on #9104 #9089

@jnohlgard jnohlgard added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Platform: AVR Platform: This PR/issue effects AVR-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 9, 2018
@jnohlgard jnohlgard added this to the Release 2018.07 milestone May 9, 2018
@kaspar030
Copy link
Contributor

kaspar030 commented May 9, 2018

Thanks for taking over! I like using patch ldscripts, seems much cleaner. I think when I tried I didn't realize the order of "-T" matters.

Keep in mind, xfa in general still needs fixing the linking process (#5757 or probably better #8711).

@kaspar030
Copy link
Contributor

@gebart would you mind rebasing? The diff is quite large as is.

@jnohlgard
Copy link
Member Author

Will do soon ish, when I am back at the computer

@jnohlgard
Copy link
Member Author

@kaspar030 Feel free to rebase and force push to my branch if you like

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

I tried a static test:

diff --git a/tests/xfa/Makefile b/tests/xfa/Makefile
index abe5a68..d5b1763 100644
--- a/tests/xfa/Makefile
+++ b/tests/xfa/Makefile
@@ -3,3 +3,11 @@ APPLICATION = xfa
 include ../Makefile.tests_common
 
 include $(RIOTBASE)/Makefile.include
+
+all: static-test
+static-test: $(ELFFILE)
+       $(eval xfastart := $(shell $(PREFIX)readelf --symbols $< |\
+              grep ' xfatest_const$$' | awk '{printf "0x%s", $$2}'))
+       $(eval xfaend   := $(shell $(PREFIX)readelf --symbols $< |\
+              grep ' xfatest_const_end$$' | awk '{printf "0x%s", $$2}'))
+       bash -c "[[ $(xfastart) < $(xfaend) ]]"

Only tested that I get an error when changing the sign, not really tested on a non implemented board.

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

When commenting LINKFLAGS += -Txfa.ld the static-test fails, yeah.

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

For mips_pic32 it's the .data section that breaks. Putting it after .init as was done in native before compiles without error and pass the static-test for pic32-clicker and pic32-wifire.
I am not saying it is working, just it does not produce linker error message.

I copied xfa.ld to mips_pic32_common and did this diff:

diff --git a/cpu/mips_pic32_common/Makefile.include b/cpu/mips_pic32_common/Makefile.include
index e0582db..648adce 100644
--- a/cpu/mips_pic32_common/Makefile.include
+++ b/cpu/mips_pic32_common/Makefile.include
@@ -2,6 +2,8 @@ include $(RIOTCPU)/mips32r2_common/Makefile.include
 
 export INCLUDES += -I$(RIOTCPU)/mips_pic32_common/include
 
+LINKFLAGS += -L$(RIOTCPU)/mips_pic32_common/ldscripts
+
 USEMODULE += mips_pic32_common
 USEMODULE += mips_pic32_common_periph
 
diff --git a/cpu/mips_pic32_common/ldscripts/xfa.ld b/cpu/mips_pic32_common/ldscripts/xfa.ld
index 34b7d76..9816bc2 100644
--- a/cpu/mips_pic32_common/ldscripts/xfa.ld
+++ b/cpu/mips_pic32_common/ldscripts/xfa.ld
@@ -10,7 +10,7 @@ SECTIONS
     __data_load_end = (__data_load_start + SIZEOF(.data));
 }
 
-INSERT AFTER .text;
+INSERT AFTER .init;
 
 SECTIONS
 {

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

For msp430 based boards using msp430-gcc-4.6.3 I get the following error:

error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]

msp430-gcc --version
msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)

I disabled it with:

diff --git a/cpu/msp430_common/Makefile.include b/cpu/msp430_common/Makefile.include
index ab6e0cf..b34ce66 100644
--- a/cpu/msp430_common/Makefile.include
+++ b/cpu/msp430_common/Makefile.include
@@ -7,6 +7,9 @@ export CFLAGS += -DCPU_MODEL_$(MODEL)
 export UNDEF += $(BINDIR)/msp430_common/startup.o
 export USEMODULE += msp430_common msp430_common_periph periph_common
 
+# Ignore unknown pragmas (for xfa with gcc 4.6.3)
+CFLAGS += -Wno-pragmas
+
 DEFAULT_MODULE += oneway_malloc
 
 # include the msp430 common Makefile

With the static-test, the pic32 patch and the warning disabled I can run buildtest with success.

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

I successfully run the xfa test with iotlab-m3, iotlab-a8-m3, and wsn430-v1_4.
I cannot test with samr21-xpro and arduino-zero it looks like saclay site is not working properly.

I could re-run all the automated tests these ones when it works again.

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

Hmm, in fact, the real problem is that samr21-xpro and arduino-zero do not show any output with this branch. I tested xfa, hello-world, and default.
They both use a samd21 cpu.

@jnohlgard
Copy link
Member Author

@cladmi could you test on those boards with only the cortexm_common ldscript refactoring applied?

@cladmi
Copy link
Contributor

cladmi commented May 9, 2018

Bisecting tells me it is the first bad commit:

cortexm_common: Update ldscript to be more similar to binutils default

@jnohlgard
Copy link
Member Author

We need to examine the linker map for that build (hello-world.map)

@jnohlgard
Copy link
Member Author

jnohlgard commented May 14, 2018

Updated:

  • Fixed CM0 alignment issues (cortexm: ldscript refactor #9104)
  • Added unit test for verifying XFA linking at runtime
  • Added static check in tests/xfa Makefile
  • Corrected typos/mistakes in XFA_USE macros

@jnohlgard
Copy link
Member Author

The PIC32 ldscripts are messy and difficult to work with... Trying to find a solution for now

@jnohlgard
Copy link
Member Author

I found a solution to get what I believe is a correct memory map for PIC32 XFA. I don't have any hardware to test on though. Could someone run unittests/tests-core or tests/xfa on a pic32-clicker or pic32-wifire please?

@cladmi
Copy link
Contributor

cladmi commented May 31, 2018

I will now be able to do on pic32-wifire using #9259

@cladmi
Copy link
Contributor

cladmi commented May 31, 2018

For pic32-wifire

unittests tests work for tests-core. The whole tests do not work but it is also the case even without this.

There is currently no test command in xfa, but here is the output

Type '/exit' to exit.
2018-05-31 18:07:56,063 - INFO # �main(): This is RIOT! (Version: 2018.07-devel-479-g0bc32-inetm02-pr/9105/introduce_crossfile_arrays__xfa__refactored)
2018-05-31 18:07:56,064 - INFO # Cross file array test
2018-05-31 18:07:56,065 - INFO # xfatest[2]:
2018-05-31 18:07:56,066 - INFO # [0] = 1, "xfatest1"
2018-05-31 18:07:56,067 - INFO # [1] = 2, "xfatest2"
2018-05-31 18:07:56,067 - INFO # xfatest_const[2]:
2018-05-31 18:07:56,068 - INFO # [0] = 123, "xfatest_const1"
2018-05-31 18:07:56,069 - INFO # [1] = 45, "xfatest_const2"
2018-05-31 18:08:13,671 - INFO # Exiting Pyterm

@jia200x
Copy link
Member

jia200x commented Oct 10, 2018

@jia200x we should focus on #8711 first, it will simplify the implementation of this PR and no/fewer linker hacks necessary to pull in the right symbols in the linking stage.

Ok, thanks for the information!

@cladmi cladmi added State: waiting for other PR State: The PR requires another PR to be merged first State: waiting for other PR labels Oct 11, 2018
@cladmi
Copy link
Contributor

cladmi commented Oct 11, 2018

It was missing the Waiting for other PR label, it is now fixed.

@stale
Copy link

stale bot commented Apr 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 10, 2020
@stale stale bot closed this May 11, 2020
@kaspar030 kaspar030 added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels May 11, 2020
@kaspar030 kaspar030 reopened this May 11, 2020
@aabadie
Copy link
Contributor

aabadie commented May 30, 2021

XFA support was merged in #15002, closing.

@aabadie aabadie closed this May 30, 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 Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Platform: AVR Platform: This PR/issue effects AVR-based platforms Process: needs >1 ACK Integration Process: This PR requires more than one ACK State: don't stale State: Tell state-bot to ignore this issue State: waiting for other PR State: The PR requires another PR to be merged first Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants