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

New cargo target detection regress *-esp-idf target builds. #1262

Open
Vollbrecht opened this issue Nov 2, 2024 · 11 comments
Open

New cargo target detection regress *-esp-idf target builds. #1262

Vollbrecht opened this issue Nov 2, 2024 · 11 comments

Comments

@Vollbrecht
Copy link

Regression introduced in: cc-rs 1.1.32.

Problem: The target is misidentified as riscv32imc_zicsr_zifencei-esp-espidf. While this would be a "correctly" formed target for a riscv target with respect to riscv ISA version 2.1, this is not a correct target description for any esp-idf target introduced on ISA 2.0.

The esp-idf targets where added while the underlying compiler still supported the riscv ISA 2.0 that did not include any zicsr_zifencei description. E.g the official names are listed https://doc.rust-lang.org/rustc/platform-support/esp-idf.html#requirements there.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 3, 2024

Hello, what cargo/rustc version are you using?

cc try to use cargo env var passed to the build-script first before falling back to using pre-generated targets.

I checked our pre-generated targets and they are fine, so I think it's due to the rustc/cargo version you use.

@Vollbrecht
Copy link
Author

This is on current rustc nightly

rustc 1.84.0-nightly (705cfe0e9 2024-11-01)
binary: rustc
commit-hash: 705cfe0e966399e061d64dd3661bfbc57553ed87
commit-date: 2024-11-01
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.1

The same setup is working with setting cc-rs to 1.1.31. The build is invoked within the following env vars:

CARGO=/home/.../.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo
CARGO_CFG_ESPIDF_TIME64=''
CARGO_CFG_FMT_DEBUG=full
CARGO_CFG_OVERFLOW_CHECKS=''
CARGO_CFG_PANIC=abort
CARGO_CFG_RELOCATION_MODEL=static
CARGO_CFG_TARGET_ABI=''
CARGO_CFG_TARGET_ARCH=riscv32
CARGO_CFG_TARGET_ENDIAN=little
CARGO_CFG_TARGET_ENV=newlib
CARGO_CFG_TARGET_FAMILY=unix
CARGO_CFG_TARGET_FEATURE=c,m
CARGO_CFG_TARGET_HAS_ATOMIC=16,32,8,ptr
CARGO_CFG_TARGET_HAS_ATOMIC_EQUAL_ALIGNMENT=16,32,8,ptr
CARGO_CFG_TARGET_HAS_ATOMIC_LOAD_STORE=16,32,8,ptr
CARGO_CFG_TARGET_OS=espidf
CARGO_CFG_TARGET_POINTER_WIDTH=32
CARGO_CFG_TARGET_VENDOR=espressif
CARGO_CFG_UB_CHECKS=''
CARGO_CFG_UNIX=''
CARGO_ENCODED_RUSTFLAGS='--cfgespidf_time64'
CARGO_FEATURE_BINSTART=1
CARGO_FEATURE_NATIVE=1
CARGO_FEATURE_STD=1
CARGO_MANIFEST_DIR=/home/.../.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-idf-sys-0.35.0
CARGO_MANIFEST_LINKS=esp_idf
CARGO_MANIFEST_PATH=/home/.../.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-idf-sys-0.35.0/Cargo.toml
CARGO_PKG_NAME=esp-idf-sys
CARGO_PKG_README=README.md
CARGO_PKG_REPOSITORY='https://github.com/esp-rs/esp-idf-sys'
CARGO_PKG_RUST_VERSION=1.66
CARGO_PKG_VERSION=0.35.0
CARGO_PKG_VERSION_MAJOR=0
CARGO_PKG_VERSION_MINOR=35
CARGO_PKG_VERSION_PATCH=0
CARGO_PKG_VERSION_PRE=''
CRATE_CC_NO_DEFAULTS=1
DEBUG=true
DEP_COMPILER_RT_COMPILER_RT=/home/.../.cargo/registry/src/index.crates.io-6f17d22bba15001f/compiler_builtins-0.1.136/compiler-rt
ESP_IDF_VERSION=v5.2.2
HOST=x86_64-unknown-linux-gnu
LD_LIBRARY_PATH='/home/.../rust/cc-regression-test/target/debug/deps:/home/.../rust/cc-regression-test/target/debug:/home/.../.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib'
MCU=esp32c3
NUM_JOBS=12
OPT_LEVEL=z
OUT_DIR=/home/.../rust/cc-regression-test/target/riscv32imc-esp-espidf/debug/build/esp-idf-sys-122dd241fb831de6/out
PROFILE=debug
RUSTC=/home/.../.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustc
RUSTC_LINKER=ldproxy
RUSTDOC=/home/.../.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rustdoc
TARGET=riscv32imc-esp-espidf

The two notable env vars here are CRATE_CC_NO_DEFAULTS=1 that we always set and well the TARGET=riscv32imc-esp-espidf one. Still it will go into morphing that into riscv32imc_zicsr_zifencei-esp-espidf

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 3, 2024

That's strange I can't find anything in target/generated.rs

Can you show me where the riscv32imc_zicsr_zifencei-esp-espidf is used? Is it in the args passed to the compiler?

Also cc @madsmtm

@Vollbrecht
Copy link
Author

In the past it was the case that rust / llvm where following the ISA spec 2.0, In there words the i2p0 were equivalent to i2p1_zicsr2p0_zifencei2p0. In the idf toolchain based on gcc12+ ISA spec 2.1 is used, making a breaking change out of it.

We then needed to somehow not break the ecosystem and bridge the gap between the rust/llvm world and gcc toolchain version that were introduced into newer esp-idf targets.

This lead us to manually injecting a "new target" into cc-rs via cmake config in code like this:

    455     let mut cmake_config = cmake::Config::new(&out_dir);

    473         if target == "riscv32imc-esp-espidf" {
    474             cmake_config.target("riscv32imc_zicsr_zifencei-esp-espidf");
    475         } else if target == "riscv32imac-esp-espidf" {
    476             cmake_config.target("riscv32imac_zicsr_zifencei-esp-espidf");
    477         } else if target == "riscv32imafc-esp-espidf" {
    478             cmake_config.target("riscv32imafc_zicsr_zifencei-esp-espidf");

This would end up creating the right invocation for the newer gcc version's. I think this is what is now broken. Have to look closer tomorrow at this.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 3, 2024

Thanks, so gcc start using newer schema and we need some mapping here?

@madsmtm
Copy link
Contributor

madsmtm commented Nov 3, 2024

I'm not against adding custom code for handling these targets, preferably in the gen-target-info crate.

@Vollbrecht
Copy link
Author

In a perfect world cc-rs would have info about the gcc version in the Tool struct or something. Either knowing its gcc12+ or a older version. And then if it sees riscv32imc it would emit -march=rv32imc for the older compiler versions and --march=rv32imc_zicsr_zifencei, but i guess we don't have this luxury. At least in the default case.

That is one of the points why we currently still always emitting the CC_CRATES_NO_DEFAULT env var so that code path is not triggered for builds that are using the newer toolchain.

Still have to test into how cmake crate handels the stuff when we would change it directly.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 4, 2024

Either knowing its gcc12+ or a older version.

I think that might be doable?

Our compiler_detection.c already uses bunches of macros, and IIRC there're also macros for gcc major/minor version

@ivmarkov
Copy link

ivmarkov commented Nov 4, 2024

I'm not against adding custom code for handling these targets, preferably in the gen-target-info crate.

IMO it is not "just" about the ESP IDF targets (rsicv32im(af)c-esp-espidf).

I think currently, every single RISCV target is affected, so this future custom code that emits this extra _zicsr_zifencei string appended to the GCC target for GCC 12+ - I think - needs to be triggered for all RISCV targets, as soon as GCC 12+ is detected.

It is a RISCV ISA 2.0 vs 2.1 confusion, in that Rust+LLVM still follow ISA 2.0 (right?), while GCC >= 12 follows ISA 2.1.

@ivmarkov
Copy link

ivmarkov commented Nov 4, 2024

Related: rust-lang/cmake-rs#225

@ivmarkov
Copy link

ivmarkov commented Nov 4, 2024

Related: rust-lang/cmake-rs#225

Since it might not be clear how this ^^^ is related:

  • In our own cross-compiling scenario, I think it is best to completely turn off the cc-rs build flags' derivation logic, as we have all the flags we need "apriori" and without turning off the cc-rs flags generation, we end up with duplicated flags. Including the -march flag duplicated wrongly, as per above.

The reason why we can't just switch off the flags' generation logic of cc-rs is simply because cmake-rs (which we actually use) does not expose the no_default_flags builder-setter of cc-rs, hence the above PR to cmake-rs.

But I think regardless, the "zicsr" and "zifencei" problem needs to be solved anyway, if not for us, then for those folks that might want cc-rs to generate the -march flag for them. I'm a bit surprised no-one had complained so far?

Perhaps the reason is the combination of the facts that at least RISCV32 is still new-ish, and then these targets - when used from Rust - tend to be used baremetal-only (i.e. no C code around to compile with GCC as these are usually MCU targets). With the notable exception of the -espidf targets of course.

@Vollbrecht Vollbrecht changed the title New cargo taraget detection regress *-esp-idf target builds. New cargo target detection regress *-esp-idf target builds. Nov 4, 2024
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

No branches or pull requests

4 participants