Skip to content

avr: Provide abort() #830

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

Merged
merged 1 commit into from
Apr 21, 2025
Merged

avr: Provide abort() #830

merged 1 commit into from
Apr 21, 2025

Conversation

Patryk27
Copy link
Contributor

@Patryk27 Patryk27 commented Apr 19, 2025

On AVRs, an architecture that doesn't support traps, unreachable code paths get lowered into calls to an intrinsic called abort:

https://github.com/llvm/llvm-project/blob/cbe8f3ad7621e402b050e768f400ff0d19c3aedd/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp#L4462

avr-gcc doesn't provide this function, presumably because they deal with traps in some other way, so when LLVM decides not to optimize those unreachable code paths out (spotted over Rahix/avr-hal#651 and rust-lang/rust#139737), currently the linker fails with undefined reference.

It doesn't have to, though, because we can provide a valid implementation of this intrinsic right here.

Note that while we could shove this responsibility to end users, same way we do with #[panic_handler], traps don't really allow to recover in a generic way - abort() is called when an unexpected code path gets exercised, so for all we know the environment (especially the Rust's stdlib part of it) is gone by then, there's no point in calling panic!() or anything else.

At the same time, some people might want to provide their own recovery mechanism (doing whatever magic they want), so let's mark this intrinsic as weak, so that it can be overridden if someone wants to (in particular, a safe-ish way to implement it would be to use inline assembly to manually manipulate registers to blink a LED, for instance).

cc @tgross35

@tgross35
Copy link
Contributor

Is it possible to use the intrinsics! macro here?

Providing this seems fine to me but @bjorn3 are there any concerns here?

@Patryk27
Copy link
Contributor Author

Sure, done!

@EvansJahja
Copy link

Thank you for taking the issue.

I'm wondering if it is the right thing to do to provide a default implementation that loop. My concern is that users that are unaware of this behavior might be running their code and suddenly it locks up without much explanation. With panic handlers it was made obvious that you have to provide a panic handler, but with this, now it just locks up silently.

Is it possible to emit a warning mentioning that users should either provide their own abort otherwise the default implementation of loop{} will be used?

@tgross35
Copy link
Contributor

tgross35 commented Apr 20, 2025

Sorry about the CI noise, noise from the recent libm merge. Should go away with a rebase (I think...)

@Patryk27
Copy link
Contributor Author

Patryk27 commented Apr 20, 2025

My concern is that users that are unaware of this behavior might be running their code and suddenly it locks up without much explanation.

I see, I just think there's not much of other behavior that can happen here - you can't reliably restart the chip (if it's connected to some other chips via SPI etc., those wouldn't be restarted) etc., so while a lockup is unfortunate, it's also not really expected to happen in practice (after all, those are supposed to be unreachable code paths).

Is it possible to emit a warning mentioning that users should either provide their own abort otherwise the default implementation of loop{} will be used?

We could certainly document it, but I'm not sure if there's a way to provide a warning - the thing is that you can pull abort() from outside of Rust code (by side-linking it from a library), so throwing a warning from rustc would be checking it too early, this warning would have to be done by the linker.

Once we drop avr-gcc and switch to LLVM's linker we could experiment there, but for the time being I'm not sure if there's something better we can do - I'm open to suggestions, of course!

Just note that abort(), except for when called explicitly through code::intrinsics::abort(), is opportunistic - instead of calling it, LLVM might simply not generate any instruction whatsoever (i.e. on unreachable paths the control will flow to the next block / next function, with totally broken assumptions) etc., so users mustn't rely on it being a protection layer.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

LGTM, but I think you will need to rebase

@tgross35 tgross35 enabled auto-merge (rebase) April 21, 2025 06:30
@tgross35 tgross35 merged commit e823bb7 into rust-lang:master Apr 21, 2025
35 checks passed
@tgross35
Copy link
Contributor

Or I guess I can :) thanks!

tgross35 added a commit to tgross35/rust that referenced this pull request Apr 22, 2025
Includes the following changes:

* Provide `abort` on AVR [1]

[1]: rust-lang/compiler-builtins#830
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 22, 2025
Update `compiler_builtins` to 0.1.156

Includes the following changes:

* Provide `abort` on AVR [1]

[1]: rust-lang/compiler-builtins#830
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 23, 2025
Update `compiler_builtins` to 0.1.156

Includes the following changes:

* Provide `abort` on AVR [1]

[1]: rust-lang/compiler-builtins#830
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2025
Rollup merge of rust-lang#140146 - tgross35:update-builtins, r=tgross35

Update `compiler_builtins` to 0.1.156

Includes the following changes:

* Provide `abort` on AVR [1]

[1]: rust-lang/compiler-builtins#830
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.

4 participants