Stream: t-compiler

Topic: change Rust::Scalar ABI to pass Floats by memory


gnzlbg (Sep 17 2019 at 10:58, on Zulip):

@eddyb I'm not sure which code needs modifying to achieve that

gnzlbg (Sep 17 2019 at 10:59, on Zulip):

but that would be a quick fix for a couple of soundness bugs

eddyb (Sep 17 2019 at 13:31, on Zulip):

WOW

eddyb (Sep 17 2019 at 13:32, on Zulip):

you can look at how this is done for vectors

rkruppe (Sep 17 2019 at 13:34, on Zulip):

Ugh, please no

eddyb (Sep 17 2019 at 13:35, on Zulip):

@gnzlbg https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs#L2819-L2844

eddyb (Sep 17 2019 at 13:35, on Zulip):

knock yourself out :P

eddyb (Sep 17 2019 at 13:36, on Zulip):

(note that you need to do also do it for ScalarPair where at least one of the scalars is a float)

rkruppe (Sep 17 2019 at 13:36, on Zulip):

Vector values are usually concentrated in big fully-inlined hot loops so passing them through memory is not a huge problem, but passing scalar floats by value can easily be a part of hot (scalar) code.

eddyb (Sep 17 2019 at 13:36, on Zulip):

@rkruppe at this point I'm worried to even ask or debate it, lol

gnzlbg (Sep 17 2019 at 13:36, on Zulip):

So one can just add a match arm there for Abi::Scalar(Float..) and make the argument indirect ?

eddyb (Sep 17 2019 at 13:37, on Zulip):

we can do that on a PR or w/e

eddyb (Sep 17 2019 at 13:37, on Zulip):

suuuure

eddyb (Sep 17 2019 at 13:37, on Zulip):

no promises it won't break in strange ways

gnzlbg (Sep 17 2019 at 13:37, on Zulip):

sure

rkruppe (Sep 17 2019 at 13:37, on Zulip):

And the issue this would paper over only occurs a very weird scenario (+soft-float or -sse applied inconsistently) which we've never supposed and which has other serious problems as well

gnzlbg (Sep 17 2019 at 13:38, on Zulip):

It wouldn't fix, e.g., extern "C" functions

eddyb (Sep 17 2019 at 13:38, on Zulip):

(also, this doesn't change anything about C, so what you'll likely cause is people abusing extern "C" fns in Rust to get perf back or something)

gnzlbg (Sep 17 2019 at 13:38, on Zulip):

if libstd standard exposed one, the ABI would still mismatch a declaration in a crate

gnzlbg (Sep 17 2019 at 13:39, on Zulip):

this would only ""fix"" the issue for extern "Rust"

gnzlbg (Sep 17 2019 at 13:39, on Zulip):

maybe

gnzlbg (Sep 17 2019 at 13:39, on Zulip):

That will depend on what the impact is

rkruppe (Sep 17 2019 at 13:39, on Zulip):

More importantly it wouldn't fix the fact that you're getting a libstd compiled with sse/hardfloat enabled, so anyone trying to get kernel-compatible code without recompiling the sysroot is doing it wrong to begin with

gnzlbg (Sep 17 2019 at 13:40, on Zulip):

yep, but people doing that already need to re-compile everything

rkruppe (Sep 17 2019 at 13:40, on Zulip):

What is the use case for -Ctarget-feature=+soft-float that doesn't require that?

gnzlbg (Sep 17 2019 at 13:41, on Zulip):

I suppose we can just forbid that target feature for some targets

rkruppe (Sep 17 2019 at 13:41, on Zulip):

???

gnzlbg (Sep 17 2019 at 13:41, on Zulip):

and those who want it would have to create their own new targets

rkruppe (Sep 17 2019 at 13:41, on Zulip):

Oh, maybe, I guess

gnzlbg (Sep 17 2019 at 13:41, on Zulip):

As in, it would be part of the target specification file or similar

rkruppe (Sep 17 2019 at 13:42, on Zulip):

But Xargo with an ordinary in-tree target + -Ctarget-feature works fine

gnzlbg (Sep 17 2019 at 13:42, on Zulip):

for soft-float I don't know, but somebody might think that -C target-feature=-sse forces all code to use the FPU

gnzlbg (Sep 17 2019 at 13:42, on Zulip):

and they might want to try to hack their way to get higher precision or something

rkruppe (Sep 17 2019 at 13:42, on Zulip):

Ok, correction, what is a good use case?

gnzlbg (Sep 17 2019 at 13:42, on Zulip):

that won't do what they want

gnzlbg (Sep 17 2019 at 13:43, on Zulip):

but it wouldn't be broken either

gnzlbg (Sep 17 2019 at 13:43, on Zulip):

I think a good use-case would be thumb mode on arm

gnzlbg (Sep 17 2019 at 13:43, on Zulip):

but that still requires choosing the default mode and then explicitly going from arm to thumb

rkruppe (Sep 17 2019 at 13:43, on Zulip):

thumb mode doesn't cause abi mismatches tho

gnzlbg (Sep 17 2019 at 13:44, on Zulip):

no, but it won't do what the user wants

gnzlbg (Sep 17 2019 at 13:44, on Zulip):

e.g. it won't change the mode for the code in the whole binary

rkruppe (Sep 17 2019 at 13:45, on Zulip):

Ok, sure

rkruppe (Sep 17 2019 at 13:45, on Zulip):

Now where does that leave us

gnzlbg (Sep 17 2019 at 13:45, on Zulip):

once cargo supports compiling libstd, any RUSTFLAG should cause a recompilation

gnzlbg (Sep 17 2019 at 13:45, on Zulip):

and at that point we maybe can just forbid linking crates with mismatching target features

gnzlbg (Sep 17 2019 at 13:45, on Zulip):

the question is whether it is worth it to do something else in the meantime

gnzlbg (Sep 17 2019 at 13:46, on Zulip):

one can trigger all these issues without -C target-feature, e.g., choose a x86 target without SSE, and call a #[target_feature(enable = "sse")] function

gnzlbg (Sep 17 2019 at 13:47, on Zulip):

so forbidding target features per target, or doing link time checking won't catch all unsoundness

gnzlbg (Sep 17 2019 at 13:47, on Zulip):

the proper fix would be to detect those incompatibilities on a per function basis, and use different call ABIs

gnzlbg (Sep 17 2019 at 13:48, on Zulip):

the float* change is a one liner, I don't know how hard it would be to track target features on call ABIs

rkruppe (Sep 17 2019 at 13:51, on Zulip):

The float change is a one liner but one which most likely adversely impacts the performance of the vast, vast majority of code which doesn't have any problem

rkruppe (Sep 17 2019 at 13:52, on Zulip):

That the alternatives are harder doesn't make it good

gnzlbg (Sep 17 2019 at 13:52, on Zulip):

true

gnzlbg (Sep 17 2019 at 14:02, on Zulip):

i can't estimate what the impact of the change is for Rust

gnzlbg (Sep 17 2019 at 14:03, on Zulip):

every function call using floats that's not inlined would pay for a stack spill

gnzlbg (Sep 17 2019 at 14:18, on Zulip):

@eddyb we'd probably need to change this here: https://github.com/rust-lang/rust/blob/master/src/librustc_target/spec/abi.rs#L7

gnzlbg (Sep 17 2019 at 14:18, on Zulip):

rename Abi to AbiKind

gnzlbg (Sep 17 2019 at 14:18, on Zulip):

and add a struct Abi(AbiKind, TargetFeatures); type instead

gnzlbg (Sep 17 2019 at 14:19, on Zulip):

at some point we'd need to set the TargetFeatures for each function on its ABI, by using the module and the #[target_feature] attributes

gnzlbg (Sep 17 2019 at 14:19, on Zulip):

I think we already do that somewhere

gnzlbg (Sep 17 2019 at 14:20, on Zulip):

but we'd need to make those part of the ABI

gnzlbg (Sep 17 2019 at 14:20, on Zulip):

And then when calling a function we'd need to generate code depending on the target features of the caller and callee

RalfJ (Sep 18 2019 at 09:13, on Zulip):

but that would be a quick fix for a couple of soundness bugs

any references?

gnzlbg (Sep 18 2019 at 09:15, on Zulip):

https://github.com/rust-lang/rust/issues/63466 is one of the most recent ones

Last update: Nov 16 2019 at 01:10UTC