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

Figure out if portable-atomic and the trap handler can coexist #965

Closed
MabezDev opened this issue Nov 20, 2023 · 10 comments
Closed

Figure out if portable-atomic and the trap handler can coexist #965

MabezDev opened this issue Nov 20, 2023 · 10 comments

Comments

@MabezDev
Copy link
Member

Before we cut a release, and possibly break a bunch of code, we should figure out if these are actually incompatible. It would suck to break the world and then unbreak it in the next release.

@bugadani
Copy link
Contributor

bugadani commented Nov 20, 2023

A thing to try might be codegenning for -imac, but I believe that is also something the users will need to do when they update. (And I also think that would make portable-atomic fall back to the trap mechanism, too?)

@jessebraham
Copy link
Member

Has there been any progress here? This is definitely a blocker for the next round of releases, IMO, and would be great to resolve. I unfortunately an not too familiar with these bits.

@bugadani
Copy link
Contributor

bugadani commented Nov 30, 2023

This PR landed and will be in tomorrow's nightly, which may make some difference.

@MabezDev
Copy link
Member Author

I got tired of waiting for it in nightly and built the compiler myself. I don't think its possible for them to coexist nicely.

I think with rust-lang/rust#114499 merged, we should probably just remove the trap handler entirely (xtensa too). The whole reason for the trap handler was because we couldn't just have load_store atomics without enabled +a extension in LLVM, and in doing that we had to then handle CAS operations. This was 2 years ago, things have moved on and portable-atomic is a really nice drop in replacement (and sometimes improvement!) over core.

I think I will make a PR to tear out the trap handler, and enable portable-atomic by default. The only downside to this of course is that we will be stuck on nightly until 1.76. I'd like to hear some input on how we should handle this.

@bugadani
Copy link
Contributor

(xtensa too)

FYI that will break all heapless 0.7-based code as some stuff there uses compare-and-swap operations but does not polyfill them for Xtensa.

@MabezDev
Copy link
Member Author

I think the simple answer is update to 0.8 heapless :D, but good point, definitely worth noting that.

@jessebraham
Copy link
Member

jessebraham commented Nov 30, 2023

I think I will make a PR to tear out the trap handler, and enable portable-atomic by default. The only downside to this of course is that we will be stuck on nightly until 1.76. I'd like to hear some input on how we should handle this.

Oof, that's not ideal, but given the current situation with atomics it's probably justifiable. I'm not sure how many people, if any, are not using a recent nightly anyway, so perhaps the impact will be minimal.

I'd just say move forward with the PR, if you don't mind, and then we can make a final decision at that point. I'd like to sleep on it for now.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 30, 2023

I think the simple answer is update to 0.8 heapless :D, but good point, definitely worth noting that.

I think one of our dependencies (of esp-wifi) where we are currently on heapless-0.7 is embedded-svc. It's already updated but would need a release. Can't think of any other dependency but that doesn't mean there isn't one

@bugadani
Copy link
Contributor

Can't think of any other dependency

smoltcp 0.10 still uses it, along with embedded-nal-async

@MabezDev
Copy link
Member Author

MabezDev commented Dec 4, 2023

The answer is no, but thats okay. With rust-lang/rust#114499 merged, we can now just rely on portable-atomic and remove the trap handler. We did this in #985.

Closing.

@MabezDev MabezDev closed this as completed Dec 4, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants