armcm: Add exception index and dummy heap for newlib 4.3+ #6331
Conversation
|
Thank you for submitting a PR, please could you refer to point 3 in the "what to expect" section in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and add a signed-off-by line. Thanks |
Laikulo
force-pushed
the
feature/armcm_exidx
branch
from
aa0d199
to
ecf4266
Compare
|
Trailer added |
|
Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html There are some steps that you can take now:
Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
|
Thanks. We can certainly change the linker scripts for compatibility, but at first glance this seems like a newlib bug. Do you know that newlib is committing to requiring these sections going forward (it doesn't require any particular linker sections today). -Kevin |
AFAICT, this is related to newlib enableing exception support on some more arm platforms, and since klipper uses a custom linker script, the exception index had nowhere to go in that config. The heap seems to be related, I'm not 100% sure the cause, but I believe the default exception handler likely does some malloc or similar, resulting in setbrk being linked, since it is only used where dynamic memory is. Malloc doesn't actually get linked into the resulting binary, I assume that its optimized away. That being said, I can't make any guarantees for the newlib team, and this change is old enough, that I can't find much discussion about it. This came out of my attempts to build klipper on opensuse, which lead me to realize that ubuntu was shipping a ~3y old version of libnewlib. It was actually quite some time before I opened this PR. I've been using this (and a few other patches) in my personal setup since January. |
|
I am becoming more skeptical of my prior assessment that newlib itself is implicated. As I dig more into this it appears that it may be more likely tied to libgcc. I'm currently testing across different versions to see exactly what is happening here. |
|
Digging a bit more, it appears to be more related to whether newlib was compiled with or without exceptions/unwinding enabled. |
|
Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
klipper-firmware currently does not define sections for exception index. This causes build errors with `gcc-arm-embedded-12` which is the default now. Until Klipper3d/klipper#6331 is added we pin it to `gcc-arm-embedded-11`.
|
Got the same issue on Armbian on a RPi5 Applying this commit as a patch on master (v0.12.0-207-g6cd17420 as of this writing) fixes the issue and linker complete succesfully. |
|
Just to report in, this is also a problem on Ubuntu 24.04 which comes with libnewlib-dev (4.4.0.20231231-2) by default
Applying this commit as a path on master (Version: v0.12.0-207-g6cd17420) allowed me to finish the compile of klipper on this platform |
|
I initially marked this as a draft, as I wasn't confident in my diagnosis why exactly the behaviors differed across versions. |
Signed-off-by: Aaron B. Haun <[email protected]>
Laikulo
force-pushed
the
feature/armcm_exidx
branch
from
ecf4266
to
aca4d17
Compare
|
I think PR #6615 may be a better fix for this issue. -Kevin |
|
It does indeed! Thanks for the fix! |
|
FYI, #6615 was merged. Hopefully that addresses the issue here. -Kevin |
|
Can confirm, compiled v0.12.0-242 successfully on Ubuntu 24.04 (which uses newlib > 4.2) |
|
I'll confirm on the same system that originated this ticket later today. I've got a github actions compile that fails after that was merged, but it may just be a poor interaction between the patchsets. |
|
I can confirm that stock upstream now compiles for the targets that were giving me grief. I'll run through the test battery later today. https://github.com/Laikulo/Klipper-firmware/actions/runs/9587485332/job/26437625740 |
|
All of the test configs succeeded, except ar100, which I am going to say is probably a different problem |
This commit adds entries to the armcm linker script to define a location for the exception index and a minimal heap.
This is to provide compat with newer versions of newlib, which will fail to compile if these sections are not defined.
This has been tested with newlib 4.3.0.20230120-2.3, gcc 13.2.1 20230803.
Example of failure before this commit:
Example with this patch
Config here: https://github.com/Laikulo/Klipper-firmware/blob/main/factory-configs/btt-octopus-stm32f429-canbridge-1m.config
I also ran a test build in an ubuntu (jammy) container with the -ci scripts, but one of the avr configurations failes both with and without this patch due to code space limitations. However, I confirmed that the config I used above also compiles successfully on ubuntu with newlib 3.3.0-1.3