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

Rework install_parameters generation #472

Merged
merged 4 commits into from
Dec 4, 2023
Merged

Conversation

xchapron-ledger
Copy link
Contributor

@xchapron-ledger xchapron-ledger commented Nov 24, 2023

Description

  • Update the way ELF metadata sections are added
  • Makefile: Improve app _install_parameters generation

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Tests
  • Documentation
  • Other (for changes that might not fit in any category)

Tests:

Tested on a list of 25 apps (but no specific app sha1) with no app.sha256 change:

app-bitcoin,
app-bitcoin-new,
app-boilerplate,
app-eos,
app-ethereum,
app-exchange,
app-flow,
app-misc,
app-nem,
app-plugin-boilerplate,
app-security-key,
app-symbol,
app-tron,
app-u2f,
app-waves,
app-zilliqa,
app-u2f,
app-acala,
app-avalanche,
app-khala,
app-polkadot,
app-cardano,
app-qrl,
app-qtum,
app-plugin-1inch

Tasks:

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3786673) 59.44% compared to head (117f71b) 59.44%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #472   +/-   ##
=======================================
  Coverage   59.44%   59.44%           
=======================================
  Files          12       12           
  Lines        1684     1684           
=======================================
  Hits         1001     1001           
  Misses        683      683           
Flag Coverage Δ
unittests 59.44% <ø> (ø)

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.

@xchapron-ledger xchapron-ledger force-pushed the xch/app-metadata branch 3 times, most recently from 9f0764d to 0d391f7 Compare November 27, 2023 15:31
@xchapron-ledger xchapron-ledger marked this pull request as ready for review November 28, 2023 08:37
@xchapron-ledger xchapron-ledger force-pushed the xch/app-metadata branch 2 times, most recently from 4c4a139 to 0209701 Compare November 28, 2023 08:59
xchapron-ledger pushed a commit to LedgerHQ/app-cardano that referenced this pull request Nov 30, 2023
This will become necessary once
LedgerHQ/ledger-secure-sdk#472 is merged and
cherry-picked.
Update metadata to keep the exact same values
src/app_metadata.c Show resolved Hide resolved
src/app_metadata.c Show resolved Hide resolved
Xavier Chapron added 3 commits December 1, 2023 18:28
This includes:
- Generation before build time and inclusion in the binary at build time
  whereas it was previously done only during sideloading.
  This allows:
  * to have a proper app size when using arm-none-eabi-size bin/app.elf,
    taking into account the size of the install_parameters
  * to have complete artifacts app.elf, app.hex, app.map, app.asm which
    can be used for debug
  * to have a proper Speculos emulation on install_parameters
    interactions.

- Parsing of APP_LOAD_PARAMS variable so that its differents fields can
  be used cleanly

- New symbols in the linker script to expose the start and the end of
  install_parameters, allowing to retrieve its size.

- A small change in src/pic.c to avoid application sha256 change in this
  commit. The change will be removed in the next commit.

This commit as been tested without modification on the application
sha256 (which includes install_parameters generated at build time or as
previously by ledgerblue.loadapp script) on numerous applications.
Following previous commit, this now included the install_parameters that
are added during the build process.
@xchapron-ledger xchapron-ledger merged commit 47af250 into master Dec 4, 2023
158 checks passed
@xchapron-ledger xchapron-ledger deleted the xch/app-metadata branch December 4, 2023 08:23
xchapron-ledger pushed a commit to LedgerHQ/app-cardano that referenced this pull request Dec 4, 2023
This will become necessary once
LedgerHQ/ledger-secure-sdk#472 is merged and
cherry-picked.

(cherry picked from commit 973b595)
@xchapron-ledger xchapron-ledger mentioned this pull request Dec 5, 2023
6 tasks
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.

4 participants