Skip to content

cortexm: ldscript refactor#9104

Closed
jnohlgard wants to merge 9 commits intoRIOT-OS:masterfrom
jnohlgard:pr/cortexm-ldscript
Closed

cortexm: ldscript refactor#9104
jnohlgard wants to merge 9 commits intoRIOT-OS:masterfrom
jnohlgard:pr/cortexm-ldscript

Conversation

@jnohlgard
Copy link
Member

Contribution description

This is a rewrite of the cortexm_base ldscript to make it more similar to the default ldscripts for native and other platforms. Sections which do not need a runtime relocation (e.g. .text, .bss), except for some special cases (.preinit_array, .init_array, .fini_array etc.) do not need to have a output segment specified, as long as the attribute flags are set properly on the memory segments (#9089).

Issues/PRs references

based on #9089

@jnohlgard jnohlgard added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-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
@jnohlgard jnohlgard requested a review from kaspar030 May 9, 2018 06:11
@jnohlgard jnohlgard force-pushed the pr/cortexm-ldscript branch from 9f6a934 to 278bc15 Compare May 14, 2018 10:46
@jnohlgard
Copy link
Member Author

Fixed an alignment issue with the initialization code on Cortex-M0

@jnohlgard jnohlgard force-pushed the pr/cortexm-ldscript branch from 2d6ac65 to d40d9d2 Compare May 15, 2018 22:43
@cladmi cladmi added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Jun 4, 2018
@cladmi
Copy link
Contributor

cladmi commented Jun 4, 2018

To simplify reviewing, can you put in the commit message a link to the file you used a reference?

@jnohlgard
Copy link
Member Author

I don't have a link unfortunately, I used arm-none-eabi-ld --verbose to show the default ldscript

@cladmi
Copy link
Contributor

cladmi commented Jun 29, 2018

I started reviewing by getting the default one and modifying it to look like our own to help me reviewing, I will try something next week in this direction. My goal is to really show what is needed for RIOT which is not in the default one. So adding commits before that adds the default ldscript and adapts it before replacing our own.

Question, I first changed the indentation of the default ldscript to our own, I could do the opposite to really see the what are our required modifications from the default. Which way would you prefer ? Keeping our style or adopting the original one ?

To simplify comparing versions with mine, could you squash the changes that are not ". = ALIGN(4)" related ?

@jnohlgard
Copy link
Member Author

@cladmi I inserted a commit with the default ldscript before the RIOT specific changes to make this easier to review.

@jnohlgard jnohlgard requested a review from cladmi August 11, 2018 05:50
@cladmi cladmi mentioned this pull request Aug 21, 2018
18 tasks
@jnohlgard jnohlgard force-pushed the pr/cortexm-ldscript branch from 704b56a to 0688e88 Compare August 25, 2018 12:49
@jnohlgard
Copy link
Member Author

rebased and addressed build failures

@jnohlgard
Copy link
Member Author

ping @cladmi
This is useful to get #8711 further along

@cladmi
Copy link
Contributor

cladmi commented Sep 20, 2018

@gebart I am not forgetting it, no worry :)

But it is I think "only" for xfa #9105 that also needs ld -r and I tried to focus first on the ld -r dependency as it is a major change that I would like ready so it can have enough review time.

So my current pipeline for this was more ld -r -> cortexm ldscript -> xfa.

If you would like to have it before ld -r, for also other reasons, I can try to dedicate some time now.

@cladmi
Copy link
Contributor

cladmi commented Sep 20, 2018

I should have given updates here though, I am bad at this.

@jnohlgard
Copy link
Member Author

@cladmi sorry, I think I must have been mixed up the dependencies as well.
Let's focus on getting ld-r working first, then this and finally XFA.

@stale
Copy link

stale bot commented Jan 17, 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 Jan 17, 2020
@stale stale bot closed this Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms State: stale State: The issue / PR has no activity for >185 days 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.

6 participants