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

cmake: removing manual linking of libgcc.a #653

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

tejlmand
Copy link
Contributor

@tejlmand tejlmand commented Dec 4, 2024

This commit removes the manual lookup and linking of libgcc.a.

Linking of C and runtime libraries are the responsibility of the higher level build system which handles the toolchain and building of the executable.

Current sidewalk implementation is broken in the sense that it fails to consider the CPU architecture and just looks up any libgcc.a file.

For example the code might discover arm-zephyr-eabi/12.2.0/libgcc.a when instead arm-zephyr-eabi/12.2.0/thumb/v8-m.main+fp/hard/libgcc.a should be used, and thus causiong link errors such as:

error: /.../12.2.0/libgcc.a(_arm_muldf3.o):
                                    conflicting CPU architectures 17/2

and:

error: zephyr/zephyr_pre0.elf uses VFP register arguments,
       /.../12.2.0/libgcc.a(_fixunsdfdi.o) does not

@github-actions github-actions bot added sid-lib PR changing Sidewalk libraries source PR changing src files labels Dec 4, 2024
@ktaborowski
Copy link
Contributor

Similar code for M 4 (nrf52) libraries: sidewalk/lib/cortex-m4/lora_fsk/CMakeLists.txt
Should it be also changes?

Copy link

github-actions bot commented Dec 4, 2024

Sample diff used total
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.demo.ble_only RAM -4.3 KB 101.53 KB 0 B
ROM -10.95 KB 317.52 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.dut RAM -4.57 KB 140.05 KB 0 B
ROM -11.94 KB 460.99 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.dut.ble_only RAM -129.51 KB 0 B 0 B
ROM -394.63 KB 0 B 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.dut.no_secure RAM -4.57 KB 140.05 KB 0 B
ROM -11.94 KB 458.28 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello RAM -4.57 KB 117.75 KB 0 B
ROM -11.33 KB 410.19 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello.ble_only RAM -4.3 KB 91.57 KB 0 B
ROM -10.91 KB 315.86 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello.ble_only.release RAM -4.3 KB 86.94 KB 0 B
ROM -8.79 KB 247.5 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello.release RAM -4.57 KB 101.72 KB 0 B
ROM -8.84 KB 323.32 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.demo RAM -4.57 KB 117.08 KB 0 B
ROM -11.05 KB 445.19 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.demo.ble_only RAM -4.3 KB 102.3 KB 0 B
ROM -10.93 KB 367.55 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.dut RAM -4.57 KB 140.83 KB 0 B
ROM -11.93 KB 510.77 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.dut.ble_only RAM -130.29 KB 0 B 0 B
ROM -444.41 KB 0 B 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.dut.no_secure RAM -4.57 KB 140.82 KB 0 B
ROM -11.92 KB 503.09 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello RAM -4.57 KB 118.53 KB 0 B
ROM -11.33 KB 460.2 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello.ble_only RAM -4.3 KB 92.35 KB 0 B
ROM -10.91 KB 365.87 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello.ble_only.release RAM -4.3 KB 87.71 KB 0 B
ROM -8.79 KB 296.57 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello.release RAM -4.57 KB 102.5 KB 0 B
ROM -8.82 KB 372.41 KB 0 B
thingy53/nrf5340/cpuapp:sample.sidewalk.demo.ble_only RAM -3.1 KB 106.34 KB 0 B
ROM -6.07 KB 351.69 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.demo RAM -5.74 KB 118 KB 0 B
ROM -28.27 KB 450.53 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.demo.ble_only RAM -5.47 KB 102.93 KB 0 B
ROM -28.14 KB 368.16 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.dut RAM -5.74 KB 141.65 KB 0 B
ROM -29.14 KB 515.62 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.dut.ble_only RAM -131.98 KB 0 B 0 B
ROM -461.65 KB 0 B 0 B
nrf52840dk/nrf52840:sample.sidewalk.dut.no_secure RAM -5.74 KB 141.62 KB 0 B
ROM -29.13 KB 507.08 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello RAM -5.74 KB 119.23 KB 0 B
ROM -28.54 KB 465.56 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello.ble_only RAM -5.47 KB 92.76 KB 0 B
ROM -28.14 KB 366.51 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello.ble_only.release RAM -5.47 KB 87.95 KB 0 B
ROM -26.03 KB 299.24 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello.release RAM -5.74 KB 103.03 KB 0 B
ROM -26.06 KB 378.18 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.demo RAM -3.37 KB 111.5 KB 0 B
ROM -6.18 KB 395.55 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.demo.ble_only RAM -3.1 KB 96.58 KB 0 B
ROM -6.08 KB 312.05 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.dut RAM -3.37 KB 135.27 KB 0 B
ROM -7.05 KB 461.38 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.dut.ble_only RAM -123.4 KB 0 B 0 B
ROM -384.36 KB 0 B 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.dut.no_secure RAM -3.37 KB 135.25 KB 0 B
ROM -7.07 KB 453.93 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello RAM -3.37 KB 113 KB 0 B
ROM -6.45 KB 410.85 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello.ble_only RAM -3.1 KB 86.66 KB 0 B
ROM -6.06 KB 310.63 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello.ble_only.release RAM -3.1 KB 82.02 KB 0 B
ROM -3.92 KB 241.08 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello.release RAM -3.37 KB 96.94 KB 0 B
ROM -3.96 KB 320.82 KB 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.demo RAM -121.65 KB 0 B 0 B
ROM -456.17 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.demo.ble_only RAM -106.6 KB 0 B 0 B
ROM -378.42 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.dut RAM -145.4 KB 0 B 0 B
ROM -522.63 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.dut.ble_only RAM -130.29 KB 0 B 0 B
ROM -444.34 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.dut.no_secure RAM -145.38 KB 0 B 0 B
ROM -514.94 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.hello RAM -123.09 KB 0 B 0 B
ROM -471.46 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.hello.ble_only RAM -96.64 KB 0 B 0 B
ROM -376.71 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.hello.ble_only.release RAM -92.01 KB 0 B 0 B
ROM -305.29 KB 0 B 0 B
nrf54l15dk/nrf54l10/cpuapp:sample.sidewalk.hello.release RAM -107.07 KB 0 B 0 B
ROM -381.16 KB 0 B 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.demo RAM -4.57 KB 116.3 KB 0 B
ROM -11.04 KB 395.18 KB 0 B

@tejlmand
Copy link
Contributor Author

tejlmand commented Dec 4, 2024

Similar code for M 4 (nrf52) libraries: sidewalk/lib/cortex-m4/lora_fsk/CMakeLists.txt Should it be also changes?

nice catch.
Sure it should, I didn't notice this was actually duplicate in both m33 and m4 when I was looking into nRF5340dk.

Copy link
Contributor

@ktaborowski ktaborowski left a comment

Choose a reason for hiding this comment

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

lgtm
unit test failed on native_posix deprected platfrom, not related with changes in the pr. Should pass on next commit (or amend)

This commit removes the manual lookup and linking of libgcc.a.

Linking of C and runtime libraries are the responsibility of the higher
level build system which handles the toolchain and building of the
executable.

Current sidewalk implementation is broken in the sense that it fails to
consider the CPU architecture and just looks up any libgcc.a file.

For example the code might discover `arm-zephyr-eabi/12.2.0/libgcc.a`
when instead `arm-zephyr-eabi/12.2.0/thumb/v8-m.main+fp/hard/libgcc.a`
should be used, and thus causiong link errors such as:
> error: /.../12.2.0/libgcc.a(_arm_muldf3.o):
>                                    conflicting CPU architectures 17/2

and:
> error: zephyr/zephyr_pre0.elf uses VFP register arguments,
>        /.../12.2.0/libgcc.a(_fixunsdfdi.o) does not

Signed-off-by: Torsten Rasmussen <[email protected]>
@tejlmand tejlmand force-pushed the issues/libgcc_linking branch from b713b97 to e420702 Compare December 4, 2024 11:35
@tejlmand
Copy link
Contributor Author

tejlmand commented Dec 4, 2024

Similar code for M 4 (nrf52) libraries: sidewalk/lib/cortex-m4/lora_fsk/CMakeLists.txt Should it be also changes?

done

@carlescufi
Copy link

@tejlmand @ktaborowski please apply this change to the main branch as well.

@ktaborowski ktaborowski merged commit ba96090 into nrfconnect:v2.8-branch Dec 4, 2024
53 of 55 checks passed
@tejlmand
Copy link
Contributor Author

tejlmand commented Dec 5, 2024

@tejlmand @ktaborowski please apply this change to the main branch as well.

there is already a PR for that here: #654

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sid-lib PR changing Sidewalk libraries source PR changing src files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants