prefer actual ABI-controling fields over target.abi when making ABI decisions#152941
prefer actual ABI-controling fields over target.abi when making ABI decisions#152941RalfJung wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
2ed4ab0 to
b1fcdae
Compare
This comment has been minimized.
This comment has been minimized.
f2e7433 to
d7ea008
Compare
| // Everything else is assumed to use a hardfloat ABI. neon and fp-armv8 must be enabled. | ||
| // `FeatureConstraints` uses Rust feature names, hence only "neon" shows up. | ||
| FeatureConstraints { required: &["neon"], incompatible: &[] } | ||
| match self.rustc_abi { |
There was a problem hiding this comment.
This actually seems like a great improvement in consistency; every other match in this function is about rustc_abi / llvm_abiname / llvm_floatabi.
|
Linux just uses the built-in |
|
Okay, sounds good then. (That value has been renamed to "softfloat" recently but we accept the old one as well.) |
d7ea008 to
c66597e
Compare
| // Rust does not currently support any powerpc softfloat targets. | ||
| let target = &bx.cx.tcx.sess.target; | ||
| let is_soft_float_abi = target.abi == Abi::SoftFloat; | ||
| let is_soft_float_abi = target.rustc_abi == Some(RustcAbi::Softfloat); |
There was a problem hiding this comment.
We don't actually use RustcAbi::Softfloat on powerpc currently.
@Gelbpunkt Do we even have a powerpc softfloat target and how does it work? @folkertdev which target was this check supposed to deal with?
There was a problem hiding this comment.
We don't have softfloat powerpc targets at the moment, but I'm pretty sure the RfL people will want to add one? Not sure what the plans are for that...
There was a problem hiding this comment.
I assume we can then just leave this in in preparation for when a softfloat powerpc target gets added, since it'll presumably use the RustcAbi mechanism for that (AFAIK LLVM has no proper ABI control for powerpc). @folkertdev would still be nice if we could get a quick confirmation of that. :)
There was a problem hiding this comment.
Oh wait I just realized we actually assert that this is non-softfloat immediately in the next line. Given precedent set for other targets then yeah checking rustc_abi here is definitely the right call; this just slipped through during review of the original code (hard to blame anyone for that, checking target.abi is extremely natural, but sadly wrong).
|
@bors r=mati865 |
|
I wish we did something more structural here, as I've voiced, but I have no objection to this merging since the previous version also is Technically Wrong so like... Technically Right But Confusing is I guess a better starting ground for building a more structural change. |
prefer actual ABI-controling fields over target.abi when making ABI decisions We don't actually check that `abi` is consistent with the fields that control the ABI, e.g. one could set `llvm_abiname` to "ilp32e" on a riscv target without setting a matching `abi`. So, if we need to make actual decisions, better to use the source of truth we forward to LLVM than the informational string we forward to the user. This is a breaking change for aarch64 JSON target specs: setting `abi` to "softfloat" is no longer enough; one has to also set `rustc_abi` to "softfloat". That is consistent with riscv and arm32, but it's still surprising. Cc @Darksonn in case this affects the Linux kernel. Also see rust-lang#153035 which does something similar for PowerPC, and [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/De-spaghettifying.20ABI.20controls/with/575095372). Happy to delay this PR if someone has a better idea. Cc @folkertdev @workingjubilee
prefer actual ABI-controling fields over target.abi when making ABI decisions We don't actually check that `abi` is consistent with the fields that control the ABI, e.g. one could set `llvm_abiname` to "ilp32e" on a riscv target without setting a matching `abi`. So, if we need to make actual decisions, better to use the source of truth we forward to LLVM than the informational string we forward to the user. This is a breaking change for aarch64 JSON target specs: setting `abi` to "softfloat" is no longer enough; one has to also set `rustc_abi` to "softfloat". That is consistent with riscv and arm32, but it's still surprising. Cc @Darksonn in case this affects the Linux kernel. Also see rust-lang#153035 which does something similar for PowerPC, and [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/De-spaghettifying.20ABI.20controls/with/575095372). Happy to delay this PR if someone has a better idea. Cc @folkertdev @workingjubilee
…uwer Rollup of 8 pull requests Successful merges: - #151864 (Implement AST -> HIR generics propagation in delegation) - #152941 (prefer actual ABI-controling fields over target.abi when making ABI decisions) - #153227 (Don’t report missing fields in struct exprs with syntax errors.) - #152966 (Migrate 11 tests from tests/ui/issues to specific directories) - #153003 (rustdoc: make `--emit` and `--out-dir` mimic rustc) - #153034 (Remove unhelpful hint from trivial bound errors) - #153221 (Add release notes for 1.94.0) - #153297 (Update the name of the Hermit operating system)
prefer actual ABI-controling fields over target.abi when making ABI decisions We don't actually check that `abi` is consistent with the fields that control the ABI, e.g. one could set `llvm_abiname` to "ilp32e" on a riscv target without setting a matching `abi`. So, if we need to make actual decisions, better to use the source of truth we forward to LLVM than the informational string we forward to the user. This is a breaking change for aarch64 JSON target specs: setting `abi` to "softfloat" is no longer enough; one has to also set `rustc_abi` to "softfloat". That is consistent with riscv and arm32, but it's still surprising. Cc @Darksonn in case this affects the Linux kernel. Also see rust-lang#153035 which does something similar for PowerPC, and [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/De-spaghettifying.20ABI.20controls/with/575095372). Happy to delay this PR if someone has a better idea. Cc @folkertdev @workingjubilee
prefer actual ABI-controling fields over target.abi when making ABI decisions We don't actually check that `abi` is consistent with the fields that control the ABI, e.g. one could set `llvm_abiname` to "ilp32e" on a riscv target without setting a matching `abi`. So, if we need to make actual decisions, better to use the source of truth we forward to LLVM than the informational string we forward to the user. This is a breaking change for aarch64 JSON target specs: setting `abi` to "softfloat" is no longer enough; one has to also set `rustc_abi` to "softfloat". That is consistent with riscv and arm32, but it's still surprising. Cc @Darksonn in case this affects the Linux kernel. Also see rust-lang#153035 which does something similar for PowerPC, and [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/De-spaghettifying.20ABI.20controls/with/575095372). Happy to delay this PR if someone has a better idea. Cc @folkertdev @workingjubilee
…uwer Rollup of 12 pull requests Successful merges: - #152941 (prefer actual ABI-controling fields over target.abi when making ABI decisions) - #153227 (Don’t report missing fields in struct exprs with syntax errors.) - #153265 (Clarified doc comments + added tests confirming current behavior for intersperse/intersperse_with) - #152966 (Migrate 11 tests from tests/ui/issues to specific directories) - #153003 (rustdoc: make `--emit` and `--out-dir` mimic rustc) - #153034 (Remove unhelpful hint from trivial bound errors) - #153152 (Migration of LintDiagnostic - part 5) - #153177 (disable the ptr_fragment_in_final test on s390x) - #153221 (Add release notes for 1.94.0) - #153279 (feat: Provide an '.item_kind()' method on ItemEnum) - #153297 (Update the name of the Hermit operating system) - #153309 (Cleanup of c-variadic link test)
|
You mean voiced here? As indicated in my reply, I'm not sure how that's supposed to fully replace |
We don't actually check that
abiis consistent with the fields that control the ABI, e.g. one could setllvm_abinameto "ilp32e" on a riscv target without setting a matchingabi. So, if we need to make actual decisions, better to use the source of truth we forward to LLVM than the informational string we forward to the user.This is a breaking change for aarch64 JSON target specs: setting
abito "softfloat" is no longer enough; one has to also setrustc_abito "softfloat". That is consistent with riscv and arm32, but it's still surprising. Cc @Darksonn in case this affects the Linux kernel.Also see #153035 which does something similar for PowerPC, and Zulip. Happy to delay this PR if someone has a better idea.
Cc @folkertdev @workingjubilee