search

esp-rs / rust

Rust for the xtensa architecture. Built in targets for the ESP32 and ESP8266
https://www.rust-lang.org
Other
744 stars 39 forks source link

Increase esp STD targets `max_atomic_width` to 64? #107

Closed MabezDev closed 2 years ago

MabezDev commented 2 years ago

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 commented 2 years ago

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

MabezDev commented 2 years ago

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 commented 2 years ago

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

ivmarkov commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

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.

ivmarkov commented 2 years ago

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! https://github.com/espressif/esp-idf/compare/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.