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

Adds a paint-stack feature. #548

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

thejpster
Copy link
Contributor

Paints the stack with a fixed 0xcccccccc value, which makes it easier to see the stack high-water mark.

Also update the qemu.rs example to verify a .data value and a .bss value.

@thejpster thejpster requested a review from a team as a code owner August 7, 2024 11:13
@adamgreig
Copy link
Member

Nice idea and implementation, thanks! Could you fix the clippy errors?

I'm also not entirely sure it's worth having STACK_PAINT_VALUE especially if we can't even refer to it from the assembly. In other words, maybe we just document that the stack is painted with 0xCCCCCCCC and put that literal in the asm. On the other hand, if we can one day refer to the const in the asm block maybe that will be nicer.

I think either way we should write the actual paint value in the documentation too, because otherwise the first thing anyone has to do is resolve the value of that constant. It's not like we'll ever have to change it either (...right?).

@thejpster
Copy link
Contributor Author

thejpster commented Aug 8, 2024

The constant was for whoever writes a tide mark function in the future, and so it appears in the docs.

The clippy failures are due to new lints in stable Rust, not my changes. You might want another PR that fixes them all, to keep this one clean. Or I can try and fix them.

Edit: ok now I remember I did try and fix them but the workflow ran on an old version of the code. You'll see that error doesn't correspond to the latest commit I made. I have no idea why, and I'm not sure I can trigger a re-run.

@adamgreig
Copy link
Member

I've fixed the CI but you'll have to rebase the PR for it to pass, would you mind? You can also revert the spacing changes needed to appease clippy as they're already in master (but hopefully that should happen automatically when you rebase).

The constant was for whoever writes a tide mark function in the future, and so it appears in the docs.

OK, makes sense. Could you mention the value of the constant in the docs rather than only having it by name?

@thejpster
Copy link
Contributor Author

Dear GitHub. If I press “sync fork” why would you do a merge commit and not a rebase?
Yours, A phone user with bad signal.

@adamgreig
Copy link
Member

Thanks! I pushed a small change to mention the value in the docs and change the note about keeping it in sync from the docstring to a regular comment, if you think that looks OK I'll go ahead and merge.

@thejpster
Copy link
Contributor Author

Looks good to me. Might be worth a rebase first though.

thejpster and others added 3 commits August 15, 2024 01:27
Paints the stack with a fixed 0xcccccccc value, which makes it easier to see the stack high-water mark.

Also update the qemu.rs example to verify a .data value and a .bss value.
Now passes the new, stricter, clippy lints.
@adamgreig adamgreig added this pull request to the merge queue Aug 15, 2024
Merged via the queue into rust-embedded:master with commit 5473462 Aug 15, 2024
13 checks passed
@thejpster thejpster deleted the paint-stack branch August 15, 2024 07:37
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.

2 participants