Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

cargo: Add rustflag to force frame pointers #23

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Aug 17, 2022

as required by the
esp-backtrace crate.

@har7an
Copy link
Contributor Author

har7an commented Aug 17, 2022

I found this after inspecting the esp-backtrace crate, which was included as dependency in a new project I generated from the no_std template today. Although I must say that it also works without this flag, and the output is the same regardless of whether it is present or not (for Panics, that is. Haven't tested exceptions yet).

@MabezDev
Copy link
Member

Frame pointers make no difference on Xtensa, the ABI guarantees access to the previous frame. This PR should be adding the flag in the RISC-V section. On top of that we should probably add a comment next to it mentioning that although you get better debuggability, the code produced is most likely sub-optimal.

@har7an
Copy link
Contributor Author

har7an commented Aug 17, 2022

I see, thanks for clarifying!
Should I still add it by default then, or maybe commented out with the explaining comment next to it? I'll also try to file a PR against esp-backtrace to add that bit of information to the README.

@MabezDev
Copy link
Member

imo we should add it by default, I personally value debuggability over performance when developing applications. As long as we mention that for production builds you probably want to turn this off (possibly even then not, if you want to send backtraces to the cloud) it should be okay.

@har7an har7an force-pushed the fix/esp-backtrace-force-frame-pointers branch from 8ef874e to a67e12e Compare August 17, 2022 10:46
@har7an
Copy link
Contributor Author

har7an commented Aug 17, 2022

Done, what do you think?

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!

@MabezDev MabezDev merged commit c5bf012 into esp-rs:main Aug 17, 2022
@har7an har7an deleted the fix/esp-backtrace-force-frame-pointers branch August 17, 2022 12:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants