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

Increase esp STD targets max_atomic_width to 64? #107

Closed
MabezDev opened this issue Feb 8, 2022 · 7 comments · Fixed by rust-lang/rust#94292
Closed

Increase esp STD targets max_atomic_width to 64? #107

MabezDev opened this issue Feb 8, 2022 · 7 comments · Fixed by rust-lang/rust#94292
Assignees
Labels

Comments

@MabezDev
Copy link
Member

MabezDev commented Feb 8, 2022

Atomic load, store & CAS routines are emulated in the newlib component of esp-idf all the way up to 64 bits. Even for targets with native atomic support, 64bit atomics are emulated.

Therefore I think it should be safe to increase max_atomic_width to 64.

@ivmarkov
Copy link

We probably want to upstream this for riscv32imc-esp-espidf?

@MabezDev
Copy link
Member Author

I've updated the targets in https://github.com/esp-rs/rust/commits/esp-1.59.0.0, however this causes an issue for pio builds. They are fixed to v4.3.2, instead of the 4.3 release branch which is still getting backports. Perhaps its acceptable to leave pio users on 1.58, and hope that pio defaults to v4.4 soon?

@ivmarkov
Copy link

Hmm, can you paste the output from the build? I can't imagine how the pio build could be affected by that.

@ivmarkov
Copy link

Oh you mean 4.3.2 does not have the 64 bit atomics? Don't worry, I can patch those in, for the pio build.

@MabezDev MabezDev self-assigned this Feb 23, 2022
@MabezDev MabezDev moved this to Todo in esp-rs Feb 23, 2022
@MabezDev MabezDev moved this from Todo to In Progress in esp-rs Feb 23, 2022
@MabezDev
Copy link
Member Author

By the way, maybe we shouldn't apply patches on native build? Or atleast provide a way to override the patching. Building using native, with ESP_IDF_VERSION = { value = "release/v4.3" } inside .cargo/config.toml doesn't work because the patches fail to apply.

@ivmarkov
Copy link

By the way, maybe we shouldn't apply patches on native build? Or atleast provide a way to override the patching. Building using native, with ESP_IDF_VERSION = { value = "release/v4.3" } inside .cargo/config.toml doesn't work because the patches fail to apply.

4.3 (native or not) does need patching, as there is still a lot of stuff missing there:

  • A lot of the atomics for riscv32imc
  • Some of the atomics for esp32s2
  • A bug in the handling of thread local destructors (without this fixed, every join on a thread crashes)

If latest 4.3 fails to patch, we need to see why and branch the patching (as in one for 4.3.2, another for (?) 4.3.3 and so on)

Another - perhaps cleaner - approach I'm thinking about is to forget about patching-during-build completely, and just maintain a patched fork of the ESP-IDF main repo. Building both pio and native off from a forked ESP-IDF repo is supported.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 24, 2022
…atomics, r=petrochenkov

riscv32imc_esp_espidf: set max_atomic_width to 64

For espidf targets without native atomics, there is atomic emulation inside [the newlib component of espidf](https://github.com/espressif/esp-idf/blob/master/components/newlib/stdatomic.c), this has been extended to support emulation up to 64bits therefore we are safe to increase the atomic width for the `riscv32imc_esp_espidf` target.

Closes esp-rs#107

cc: `@ivmarkov`
Repository owner moved this from In Progress to Done in esp-rs Feb 24, 2022
@ivmarkov
Copy link

4.3 (native or not) does need patching, as there is still a lot of stuff missing there:

  • A lot of the atomics for riscv32imc
  • Some of the atomics for esp32s2
  • A bug in the handling of thread local destructors (without this fixed, every join on a thread crashes)

If latest 4.3 fails to patch, we need to see why and branch the patching (as in one for 4.3.2, another for (?) 4.3.3 and so on)

Another - perhaps cleaner - approach I'm thinking about is to forget about patching-during-build completely, and just maintain a patched fork of the ESP-IDF main repo. Building both pio and native off from a forked ESP-IDF repo is supported.

It seems none of the above patches are necessary anymore for release/4.3 because everything that needed patching is now merged there!
espressif/esp-idf@v4.3.2...release/v4.3

By the way, maybe we shouldn't apply patches on native build? Or atleast provide a way to override the patching. Building using native, with ESP_IDF_VERSION = { value = "release/v4.3" } inside .cargo/config.toml doesn't work because the patches fail to apply.

Why the native build applies patches to release/v4.3 is a mystery to me - will check. They should only be applied for tag v4.3.2 and nothing else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants