Rework riscv -march and -mabi detection #796

dramforever · 2023-03-05T03:44:18Z

Currently all the riscv*gc targets use the 'd' double-float ABI, and
soft-float otherwise. There's no need to detect the operating system
type. Fixes #795.

I'm not sure how correct this is. I've checked all the targets on version
1.69.0-nightly (ef934d9b6 2023-02-08) with this script:

rustc --print target-list |
  grep riscv |
  while IFS= read target; do
    rustc --target "$target" --print target-spec-json -Z unstable-options |
      jq --arg name "$target" -r '"\(.["llvm-abiname"]) \($name)"'
  done

Which gives:

ilp32d riscv32gc-unknown-linux-gnu
ilp32d riscv32gc-unknown-linux-musl
null riscv32i-unknown-none-elf
null riscv32im-unknown-none-elf
null riscv32imac-unknown-none-elf
null riscv32imac-unknown-xous-elf
null riscv32imc-esp-espidf
null riscv32imc-unknown-none-elf
lp64d riscv64gc-unknown-freebsd
lp64d riscv64gc-unknown-linux-gnu
lp64d riscv64gc-unknown-linux-musl
lp64d riscv64gc-unknown-none-elf
lp64d riscv64gc-unknown-openbsd
null riscv64imac-unknown-none-elf

Also I'm not sure whether to add tests or not. Please advise on this.

JohnTitor · 2023-03-15T11:14:00Z

src/lib.rs

+
+                        // If riscv32gc-* or riscv64gc-* then double-float ABI,
+                        // otherwise use soft-float ABI.
+                        let float_abi = if arch.contains("g") { "d" } else { "" };


This looks fragile to me, maybe we could use arch.ends_with("gc") or contains("gc") at least.

My reasoning is that g means imafd_zifencei_zicsr, so it implies d for double, which is required for double float ABI. Currently it seems (see table in top post) that rustc does use lp64d/ilp32d for all targets with double. Maybe I can change the code to detect d or g and/or change the comment to reflect this reasoning better?

I guess I'll also change it so that it handles e.g. riscv64imac_zsomething with a multi-letter extension tha happens to contain g. Not sure if something other than an ISA/-march string appears here though tbh.

In the RISC-V Unprivileged Specification RISC-V ISA can be decided from ISA string, which indicates that if it has D inside, the target must support double length floating points; and G means that it has IMAFD and Zifencei (F indicates Zicsr). Using C here would not cover this case correctly, as that C doesn't imply G or D.

It is possible to have an RVD implementation without RVC, for example disabling C instruction set on RV64GC platform (which is sometimes common on embedded SoC or developing new SoCs); detecting GC for D will not be correct in those cases.

I've added a bunch of comments explaining the logic, and also updated the detection logic to handle the possible future existence of multi-letter extensions, so it won't get confused by e.g. riscv64imac_zsomething containing a g.

dramforever · 2023-04-08T04:59:55Z

Additional justification for inferring the ABI: https://github.com/riscv-non-isa/riscv-toolchain-conventions#specifying-the-target-isa-with--march

A target -march which includes floating point instructions implies a hardfloat calling convention, but can be overridden using the -mabi flag

Since the Rust target implies -march, it makes sense to also infer -mabi from it.

I can add a comment linking to it, if the maintainers are okay with this logic in general.

tea · 2024-06-24T09:07:37Z

Can this be revived? As it stands, cc-rs is sort-of useless for embedded RISCV*GC targets (at least without target-specific workarounds similar to mentioned in #795).

NobodyXu · 2024-06-24T09:24:20Z

Can this be revived? As it stands, cc-rs is sort-of useless for embedded RISCV*GC targets (at least without target-specific workarounds similar to mentioned in #795).

I think it needs to resolve merge conflicts and rebase against main, then I can do another review pass on it

dramforever · 2024-06-25T05:48:14Z

As original author of this PR I'm interested in reviving this

dramforever · 2024-06-25T06:52:23Z

Aaaand, rebased, hope this works.

I haven't added the toolchain convention reference, because it has been removed and it's now only a de facto standard.

Honestly, we should just get the information from rustc itself once it's available...

src/lib.rs

Instead of adding cases for all the operating systems, Use target architecture directly as ISA string, replacing "riscv" with "rv", and detect floating point ABI based on support for the D and F extensions. Fixes rust-lang#795

NobodyXu · 2024-06-26T10:11:24Z

src/lib.rs

                        }
+
+                        cmd.args.push("-mcmodel=medany".into());


This flag is pushed unconditionally now and it was conditional before.
Can you please add a comment explain why?

My understanding is that there was no reason to make this conditional in the first place and everything uses medium/medany (same thing). But I'll verify it later.

NobodyXu · 2024-06-26T10:16:50Z

src/lib.rs

+                            .split(['_', 'z', 's'])
+                            .next()
+                            // The arch string starts with 32 or 64
+                            .expect("arch string cannot be empty");


I'm bit confused on this, I checked the riscv targets rust supported and doesn't find any z, s or underscore in after the prefix "riscv" .

What is this trying to detect?
Is there any new target which contains z, s or underscore?

i don't know how else to address the fact that isa string parsing is "fragile" #796 (comment)

in case in the future we add extensions like zsomething the arch string could look like riscv64imaczsomething or riscv64imac_zsomething and we don't want to match on the g of zsomething

i don't think there's any easy way to say how the rust project will decide to name future riscv architectures, but if you're okay with this being "fragile" i can remove this part

just to chim in here, to give an idea about how for example the riscv-esp-elf-gcc sees target definitions, it looks something like the following:

*multilib_defaults: march=rv32imafdc_zicsr_zifencei mabi=ilp32 *multilib_extra: *multilib_matches: march=rv32i_zicsr_zifencei march=rv32i_zicsr_zifencei;march=rv32iac_zicsr_zifencei march=rv32iac_zicsr_zifencei;march=rv32ic_zicsr_zifencei march=rv32ic_zicsr_zifencei;march=rv32im_zicsr_zifencei march=rv32im_zicsr_zifencei;march=rv32imc_zicsr_zifencei march=rv32imc_zicsr_zifencei;march=rv32imac_zicsr_zifencei march=rv32imac_zicsr_zifencei;march=rv32imafc_zicsr_zifencei march=rv32imafc_zicsr_zifencei;march=rv32imafdc_zicsr_zifencei march=rv32imafdc_zicsr_zifencei;march=rv32imafdc_zicsr_zifencei_xesppie march=rv32imafdc_zicsr_zifencei_xesppie;mabi=ilp32 mabi=ilp32;mabi=ilp32f mabi=ilp32f;fno-rtti fno-rtti;

this above is part of the output of ./riscv32-esp-elf-gcc -dumpspecs and presumably it looks similar on other gcc based variants.

Oh so it's non-standard rust target used in riscv-esp-elf-gcc?

If it is non-standard then it would indeed makes more sense to pass them as environment variables.

I am not sure what you mean by non-standard rust target, but our target descriptions are similar on how the other riscv targets on the rust site are being expressed.

But as dramforever mentions downstream gcc & clang is more complicated matter. Mapping to thouse means that there are possible different ISA versions that can be associated with. Rust target names in its current form lag that expressiveness if i see it correctly.

I am not sure what you mean by non-standard rust target, but our target descriptions are similar on how the other riscv targets on the rust site are being expressed.

I consider any target not in rustup or maintainer by the rust officially to be a non-standard one.

I heard of cargo plugins for esp and I think it sets the cflags and etc to work?

Would it makes sense here?

I guess it might make sense to summarize the situation:

Rust targets lists the instruction extensions after riscv{32,64}. f means hardware f32, d means hardware f64, and g is abbreviation of imafd

Currently there are only single letters there so we can detect it based on just looking for g/d or f

Previously a maintainer complained that it was fragile... but with no real way to predict future target names IMO it will never be not fragile

There's no way to get the target ABI used by rustc here, IIUC. If there was this whole detection would be obsolete for everyone.

The de facto standard for -mabi is to use the "maximum" supported hard float ABI

There's no way to get the target ABI used by rustc here, IIUC. If there was this whole detection would be obsolete for everyone.

We do have a crate to extract some riscv target information from nightly rust.

I wonder if we could get this from it as well?

https://github.com/rust-lang/cc-rs/blob/main/dev-tools/gen-target-info/src/main.rs

dramforever · 2024-06-26T10:46:36Z

@NobodyXu I just thought of another solution, can we just delete this section of the altogether and rely on the user to have configured the toolchain correctly or add the options for themselves?

NobodyXu · 2024-06-26T11:16:31Z

can we just delete this section of the altogether and rely on the user to have configured the toolchain correctly or add the options for themselves?

If it is optional flags that's there to improve performance, then maybe we can do it.

If it is required for it to compile, then I think we should keep it

dramforever · 2024-06-26T13:41:11Z

If it is optional flags that's there to improve performance, then maybe we can do it.

If it is required for it to compile, then I think we should keep it

Unfortunately it is required for correctness :(

(Edit I mean, required for compilation to go through. The linker will catch (most) mistakes.)

tea · 2024-09-21T08:09:56Z

What is preventing this PR from being merged as is? Would it introduce any regressions?

It might not be perfect, it might be fragile for some obscure corner cases, but right now cc-rs can't even handle the bog-standard riscv64gc arch on non-standard OSes! In particular, it blocks introducing risc-v support on Redox, at least without forking this crate which is pretty much a non-starter.

Maybe the PR could be merged in (to move the needle in the right direction), and then you guys could continue perfecting it?

NobodyXu · 2024-09-21T08:46:40Z

What is preventing this PR from being merged as is? Would it introduce any regressions?

we were previously discussing about getting the flags from rustc codebase at build time.

It might not be perfect, it might be fragile for some obscure corner cases, but right now cc-rs can't even handle the bog-standard riscv64gc arch on non-standard OSes! In particular, it blocks introducing risc-v support on Redox, at least without forking this crate which is pretty much a non-starter.

I've posted this on zulip asking for a final review from someone understands riscv before merging in, I'm not very familiar with riscv so I don't know if it'd work/break something.

It would be great if it has CI running for riscv, with that I can merge in with confidence.

dramforever · 2024-09-21T16:21:37Z

I'm still interested in working on this, although I would like some input on what kind of detection is expected here before trying to make up some logic without evidence to back it

NobodyXu · 2024-09-21T23:33:27Z

I'm still interested in working on this, although I would like some input on what kind of detection is expected here before trying to make up some logic without evidence to back it

cc-rs has an infrastructure to extract information from rustc +nightly -Zunstable-options --print all-target-specs-json.

https://github.com/rust-lang/cc-rs/blob/main/dev-tools/gen-target-info/src/main.rs

Basically you need to modify the main.rs to parse the riscv related target information, and write them to src/target_info.rs as an (sorted) array.

Once you've done that, you'd need to run cargo run on that crate, which will generate an updated src/target_info.rs for you and you can use it in lib.rs, without hardcoding anything.

You'd also need to stage and commit src/target_info.rs as well @dramforever

JohnTitor reviewed Mar 15, 2023

View reviewed changes

dramforever force-pushed the riscv-abi-arch-fix branch 6 times, most recently from 23303fc to 6a1aff2 Compare March 19, 2023 06:40

nspin mentioned this pull request Jan 6, 2024

nix: Add riscv*gc targets seL4/rust-sel4#53

Merged

dramforever force-pushed the riscv-abi-arch-fix branch from 6a1aff2 to 0526b06 Compare June 25, 2024 06:51

NobodyXu requested changes Jun 25, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

dramforever force-pushed the riscv-abi-arch-fix branch from 0526b06 to 6019801 Compare June 25, 2024 10:56

NobodyXu requested changes Jun 25, 2024

View reviewed changes

src/lib.rs Show resolved Hide resolved

Rework riscv -march and -mabi detection

0fed19a

Instead of adding cases for all the operating systems, Use target architecture directly as ISA string, replacing "riscv" with "rv", and detect floating point ABI based on support for the D and F extensions. Fixes rust-lang#795

dramforever force-pushed the riscv-abi-arch-fix branch from 6019801 to 0fed19a Compare June 26, 2024 09:00

NobodyXu requested changes Jun 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework riscv -march and -mabi detection #796

Rework riscv -march and -mabi detection #796

dramforever commented Mar 5, 2023

JohnTitor Mar 15, 2023

dramforever Mar 15, 2023

luojia65 Mar 19, 2023

dramforever Mar 19, 2023

dramforever commented Apr 8, 2023 •

edited

Loading

tea commented Jun 24, 2024

NobodyXu commented Jun 24, 2024

dramforever commented Jun 25, 2024

dramforever commented Jun 25, 2024

NobodyXu Jun 26, 2024

dramforever Jun 26, 2024

NobodyXu Jun 26, 2024

dramforever Jun 26, 2024

Vollbrecht Jun 26, 2024 •

edited

Loading

NobodyXu Jun 26, 2024

Vollbrecht Jun 26, 2024

NobodyXu Jun 26, 2024 •

edited

Loading

dramforever Jun 26, 2024

NobodyXu Jun 27, 2024

dramforever commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

dramforever commented Jun 26, 2024 •

edited

Loading

tea commented Sep 21, 2024

NobodyXu commented Sep 21, 2024

dramforever commented Sep 21, 2024

NobodyXu commented Sep 21, 2024

Rework riscv -march and -mabi detection #796

Are you sure you want to change the base?

Rework riscv -march and -mabi detection #796

Conversation

dramforever commented Mar 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dramforever commented Apr 8, 2023 • edited Loading

tea commented Jun 24, 2024

NobodyXu commented Jun 24, 2024

dramforever commented Jun 25, 2024

dramforever commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vollbrecht Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobodyXu Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dramforever commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

dramforever commented Jun 26, 2024 • edited Loading

tea commented Sep 21, 2024

NobodyXu commented Sep 21, 2024

dramforever commented Sep 21, 2024

NobodyXu commented Sep 21, 2024

dramforever commented Apr 8, 2023 •

edited

Loading

Vollbrecht Jun 26, 2024 •

edited

Loading

NobodyXu Jun 26, 2024 •

edited

Loading

dramforever commented Jun 26, 2024 •

edited

Loading