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.

Sign up for GitHub

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

Jump to bottom

nrf_compress: Add ARM thumb decompression code #17035

Merged
merged 3 commits into from
Sep 12, 2024
Merged

nrf_compress: Add ARM thumb decompression code #17035

merged 3 commits into from
Sep 12, 2024

Conversation

nordicjm
Copy link
Contributor 

Allows usage of the ARM thumb filter

@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Aug 27, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 27, 2024 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
nrfxlib nrfconnect/sdk-nrfxlib@eded44c (main) nrfconnect/sdk-nrfxlib#1448 nrfconnect/sdk-nrfxlib#1448/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 27, 2024 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 11

Inputs:

Sources:

sdk-nrf: PR head: fabe6fe9100ed2d9222d064736ada7c430e03844

more details

sdk-nrf:

PR head: fabe6fe9100ed2d9222d064736ada7c430e03844
merge base: ab2f82ad5ced6fc744acaa9b8dafa7ead4f12fbd
target head (main): ab2f82ad5ced6fc744acaa9b8dafa7ead4f12fbd
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (10) 
include
│  ├── nrf_compress
│  │  │ implementation.h
subsys
│  ├── nrf_compress
│  │  ├── CMakeLists.txt
│  │  ├── Kconfig
│  │  ├── lzma
│  │  │  ├── armthumb.c
│  │  │  │ armthumb.h
│  │  ├── src
│  │  │  │ arm_thumb.c
tests
│  ├── subsys
│  │  ├── nrf_compress
│  │  │  ├── decompression
│  │  │  │  ├── arm_thumb
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── prj.conf
│  │  │  │  │  ├── src
│  │  │  │  │  │  │ main.c
│  │  │  │  │  │ testcase.yaml

Outputs:

Toolchain

Version: 2aae60c2f9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:2aae60c2f9_81ed5a52d6

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 49
  • ✅ Integration tests
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_cloud
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi

Note: This message is automatically posted and updated by the CI

@nordicjm nordicjm force-pushed the compressionsubsysarmthumb branch 2 times, most recently from 5028ab9 to 4d83fad Compare August 27, 2024 10:57
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@nvlsianpu
Copy link
Contributor

Can you explain how this backend will be used. In chain with LZM2?

@nordicjm
Copy link
Contributor Author

Can you explain how this backend will be used. In chain with LZM2?

Yes, what this code does it change relative jumps to absolute jumps, so if you imagine 7 functions all calling 1 function in the code, you can't compress those jumps because they're all unique, but if you replace the relative address with the absolute address, all of those can be replace. So you run this filter before compressing with lzma, you compress with only absolute addresses, then on the other end you decompress with lzma and change the absolute addresses back to relatives. On one of the matter sample applications the addition of the ARM thumb filter saved ~22KiB for the update

@nordicjm nordicjm force-pushed the compressionsubsysarmthumb branch 3 times, most recently from 4522d46 to 7423400 Compare September 3, 2024 14:44
@github-actions github-actions bot removed the manifest label Sep 3, 2024
de-nordic
de-nordic reviewed Sep 10, 2024
View reviewed changes
subsys/nrf_compress/lzma/armthumb.c
uint32_t i = 0;

while ((i + 4) <= buf_size) {
if ((buf[i + 1] & 0xF8) == 0xF0 && (buf[i + 3] & 0xF8) == 0xF8) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this is same as in https://github.com/kobolabs/liblzma/blob/87b7682ce4b1c849504e2b3641cebaad62aaef87/src/liblzma/simple/armthumb.c, but is it possible that this has not been updated for Thumb 2 instruction set?
According to the armv7-m Architecture manual, the opcode, that is here 0xF8, has format:
1 1 J1 1 J2 so, yes it will have value 0xF8, but also 0xD0, 0xD8, 0xF0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a quick look through a disassembly of hello world and all the branch instructions are f0. On the test with the matter sample application this saved 20 or so KiB so I think the ones that are used by GCC seem to be covered well

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that is true that compilers tend to favor some opcodes.

@nordicjm
compress: Add armthumb filter
e774d8e 
Adds armthumb filter files taken from xz-5.6.2.tar.gz, the license
for these files is as follows:

LZMA SDK is written and placed in the public domain by Igor Pavlov.

Some code in LZMA SDK is based on public domain code from another
developers:
  1) PPMd var.H (2001): Dmitry Shkarin
  2) SHA-256: Wei Dai (Crypto++ library)

Anyone is free to copy, modify, publish, use, compile, sell, or
distribute the original LZMA SDK code, either in source code form
or as a compiled binary, for any purpose, commercial or
non-commercial, and by any means.

LZMA SDK code is compatible with open source licenses, for example,
you can include it to GNU GPL or GNU LGPL code.

Signed-off-by: Jamie McCrae <[email protected]>
@nordicjm
nrf_compress: Add ARM thumb decompression code
5c23e90 
Allows usage of the ARM thumb filter

Signed-off-by: Jamie McCrae <[email protected]>
@nordicjm
tests: nrf_compress: Add ARM thumb decompression test
fabe6fe 
Adds a test to ensure the library is working

Signed-off-by: Jamie McCrae <[email protected]>
@nordicjm nordicjm removed DNM manifest-nrfxlib changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 11, 2024
@carlescufi carlescufi merged commit c88a486 into nrfconnect:main Sep 12, 2024
13 of 16 checks passed
@nordicjm nordicjm deleted the compressionsubsysarmthumb branch November 6, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@de-nordic de-nordic de-nordic approved these changes

@nvlsianpu nvlsianpu nvlsianpu approved these changes

@carlescufi carlescufi carlescufi approved these changes

@michalek-no michalek-no Awaiting requested review from michalek-no

@tejlmand tejlmand Awaiting requested review from tejlmand

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nordicjm @NordicBuilder @nvlsianpu @carlescufi @de-nordic