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

Unify linkscript for all targets #398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Unify linkscript for all targets #398

wants to merge 1 commit into from

Conversation

yhql
Copy link
Contributor

@yhql yhql commented Aug 2, 2023

Description

We currently use one linker script per target that embeds the memory definitions and the placement of code sections in memory. The placement part is pretty similar across devices and has no reason to be kept duplicated.
This PR splits things into per-target memory definitions and a single script.ld that places code sections, and can be updated for all devices at once when we need it to.
Perhaps even the memory definitions could be merged together also (in another PR).

Changes include

  • Other (for changes that might not fit in any category)

(cleanup/refactor)

@yhql yhql force-pushed the single_linkscript branch 2 times, most recently from 092d171 to abc172e Compare August 3, 2023 07:02
@xchapron-ledger
Copy link
Contributor

This is breaking app-bitcoin-new build on nanos, so probably something doesn't works as expected.
We probably should check that this doesn't change app binaries.

@yhql yhql force-pushed the single_linkscript branch from abc172e to f1f6170 Compare August 3, 2023 07:35
@bigspider
Copy link
Contributor

bigspider commented Aug 3, 2023

I suppose it's because the G_cx symbol is no longer allocated in the CXRAM section?
Note that I use the CXRAM section explicitly on NanoS, when I need memory but I'm not using any crypto functions that might touch CXRAM... 😇

@yhql yhql force-pushed the single_linkscript branch from f1f6170 to a22130b Compare August 3, 2023 08:09
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d31f67c) 69.09% compared to head (f1b8243) 69.09%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #398   +/-   ##
=======================================
  Coverage   69.09%   69.09%           
=======================================
  Files          11       11           
  Lines         864      864           
=======================================
  Hits          597      597           
  Misses        267      267           
Flag Coverage Δ
unittests 69.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yhql yhql force-pushed the single_linkscript branch from a22130b to f1b8243 Compare August 3, 2023 08:11
@yhql
Copy link
Contributor Author

yhql commented Aug 3, 2023

Yes, I forgot the placement of G_cx. Moved it to the Nano S's layout only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it is really used but the "debug" part for each device has been dropped

Copy link
Contributor

Choose a reason for hiding this comment

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

why you removed some comments? (some of them seem to be useful)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found those to be either invalid (or not valid anymore) or confusing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants