__{read,write}eflags deprecated

[hanna-kruppe]

EDIT: deprecation reason: any eflags manipulation would need to ditch the intrinsics in favor of inline asm to be completely correct because compiler-generated code could use the eflags register in between __{read,write}eflags calls.

Original comment:

---

rust-lang/rust#51691 has brought these two intrinsics to my attention, and I believe they should not have been included in stdsimd for two reasons:

• as far as I can tell, they are not officially provided by Intel -- I can't find them in https://software.intel.com/sites/landingpage/IntrinsicsGuide/ -- they're apparently just slipped in sneakily by some compilers (admittedly including icc)
• they are gigantic footguns, I would even go as far as saying they cannot possibly used correctly

To elaborate on the second point: as the OP of the linked rust issue already pointed out, usage of these intrinsics is problematic if compiler-generated code uses the eflags register in between __{read,write}eflags calls. LLVM does not (and indeed can't really, they way it's architected) provide any way to get anything resembling a guarantee it won't do that. Not for eflags, and not for any other register (except reserved ones which no compiler-generated code touches at all). So these intrinsics are simply impossible to use reliably.

The supported way to manually mess with a register that the compiler wants to use is to have one big inline asm block that does all the manipulation in one go (and declares it as clobbered, if appropriate). rust-lang/rust#51691 mentioned doing that for x86::has_cpuid, but I'm pretty sure any eflags manipulation would need to ditch the intrinsics in favor of inline asm to be completely correct.

I realize the possibility of miscompiles caused by this is a bit remote, and there's precedent in C compilers, but could we consider at least deprecating these intrinsics seeing as they're not even documented in the intel intrinsics guide? (Of course, I'd be even happier if they were removed entirely -- luckily they aren't stabilized yet.)

[gnzlbg]

as far as I can tell, they are not officially provided by Intel -- I can't find them in https://software.intel.com/sites/landingpage/IntrinsicsGuide/ -- they're apparently just slipped in sneakily by some compilers (admittedly including icc)

FWIW being in the Intel intrinsic guide isn't a pre-requisite for being part of std::arch for x86 and x86_64. Some intrinsics there are only provided by ICC and not by any other C compiler (and are not part of std::arch), and some intrinsics are not there but provided by every single C compiler, and are part of std::arch. For example, clang and gcc also provide these.

But now to your point:

To elaborate on the second point: as the OP of the linked rust issue already pointed out, usage of these intrinsics is problematic if compiler-generated code uses the eflags register in between __{read,write}eflags calls. LLVM does not (and indeed can't really, they way it's architected) provide any way to get anything resembling a guarantee it won't do that. Not for eflags, and not for any other register (except reserved ones which no compiler-generated code touches at all). So these intrinsics are simply impossible to use reliably.

The plan of the embedded working group was to expose more assembly operations like these: rust-embedded/wg#63 where I pretty much raised the same concerns than you did, that these intrinsics are not equivalent (and can't be) to a single inline assembly block where they are used together.

cc @japaric - @rkruppe please feel to raise your concerns there as well

[gnzlbg]

but could we consider at least deprecating these intrinsics

While I personally think we should do this, we should fix the general direction for these intrinsics in the framework of embedded Rust before. I think that this means that we need to pursue stable inline assembly instead of adding these types of intrinsics because all the issues you raised.

[hanna-kruppe]

FWIW being in the Intel intrinsic guide isn't a pre-requisite for being part of std::arch

Okay fair, but if they're not in the guide it seems easier to justify not providing them at all.

The plan of the embedded working group was to expose more assembly operations like these: rust-embedded/wg#63 where I pretty much raised the same concerns than you did, that these intrinsics are not equivalent (and can't be) to a single inline assembly block where they are used together.

The specific operations I saw discussed there (reading the return address and current pc) don't have this problem to the same extent, because the register isn't being written. So there's only a single isolated read, and that might not give you the precise value you'd have expected because it's been moved around a bit, but nothing interferes fundamentally with the operation you're doing. In contrast, any read-modify-write dance requires that the register's value flows undisturbed from one intrinsic call to the next, which is the root of the problem.

Edit: I should have read on, there's pairs of "read/write register" functions further below. However, from my very limited Arm literacy I get the impression that most or all of them are about control and status registers that aren't touched by compiler generated code. If true, those don't have any issues at all.

And unlike with PC and return address, I don't know of any uses of eflags that only read or only write eflags once. Even if you only want to unconditionally set one of the higher bits that aren't used by the compiler, at minimum you need to read once and write once to preserve the other bits.

---

As an aside, I now remember there is a saving grace for these intrinsics: it's at least reasonable to expect the compiler to treat the various bits separately and only save/restore/fiddle with the ones it has any use for (ZF, SF, maybe DF, etc.). LLVM has recently made a (very painful) move in that direction because it resulted in miscompiles of "disable interrupts; do something; enable interrupts" sequences in FreeBSD (see https://bugs.llvm.org/show_bug.cgi?id=36028). But even though that means some uses of these intrinsics might be redeemable, it still seems extremely fragile and I'd still strongly recommend inline asm blocks or targeted intrinsics like "enable interrupt", "disable_interrupt", "check if CPUID is available", etc. (like @nagisa discussed in rust-embedded/wg#63 (comment))

[gnzlbg]

Okay fair, but if they're not in the guide it seems easier to justify not providing them at all.

Sure, the argument raised back then is that all compilers provided these, but if they cannot work as intended it makes sense to deprecate these and eventually remove them since they are not stable.

[gnzlbg]

@rkruppe

(Of course, I'd be even happier if they were removed entirely -- luckily they aren't stabilized yet.)

I was writing a PR to deprecate these but they have been stabilized AFAICT: https://github.com/rust-lang-nursery/stdsimd/blob/master/coresimd/x86/eflags.rs#L20 :/

[hanna-kruppe]

Huh. I could have sworn I tested on the playground that they weren't stable (and that _mm_set_ps was, so I'm reasonably sure I didn't just use a too-old version). Oh well, we can still deprecate and possibly even slap doc(hidden) on them.

[gnzlbg]

I have a PR to deprecate these: #494

The deprecation reason points to this issue, so i've edited the top comment to summarize the reason.

[gnzlbg]

cc @BurntSushi I don't know how to contact the library team (googling doesn't help) but since you are in the team, would you mind bringing this issue to their attention?

This deprecation should definitely go through an FCP or similar.

[BurntSushi]

cc @rust-lang-nursery/libs

[japaric]

@gnzlbg

please feel to raise your concerns there as well

We discussed this in the last embedded WG meeting. The TL;DR is that we don't think this is a
problem in the embedded case.

Sure, some intrinsics can lead to unexpected behavior because LLVM can't guarantee register ops
won't be re-ordered but (a) the problematic intrinsics are hardly used in practice (*) so they can
be left unstable, be omitted (i.e. we don't add them), or the footgun can be documented (if we
have to add them and stabilize them for completion's sake).

Also, (b) people that need precise control over the order in which registers are read / written
(e.g. they are writing a context switching routine) will be using an external assembly file (stable)
or global_asm! (nightly) instead of intrinsics / inline assembly.

(*) The intrinsics that will be used most often (bkpt, enable / disable interrupts, read / write
BASEPRI) behave correctly with a "memory" clobber and "volatile" modifier. We have been using them
like that (wrappers around asm!) for quite some time (cf. cortex-m crate).

[gnzlbg]

(if we have to add them and stabilize them for completion's sake).

(*) The intrinsics that will be used most often (bkpt, enable / disable interrupts, read / write
BASEPRI) behave correctly with a "memory" clobber and "volatile" modifier. We have been using them
like that (wrappers around asm!) for quite some time (cf. cortex-m crate).

The __{read/write}eflags have been used on all x86 targets for over a year without issues, but just because they work now does not mean that they can be used correctly.

The worst that can happen is that a lot of code relies on intrinsics that appear to work, and that we break at some point just to discover that "they can't be repaired".

We should be thinking if what we guarantee about these makes them usable or not. So I would prefer if we just added intrinsics that can be used correctly, and if we tried to be skeptic about those for which we are not sure. If someone compiles a list maybe we could review with the rust-memory-model group and make a decision there.

In retrospect for x86 we were more focused on allowing everything that C allows than about thinking "does this makes sense in Rust". For ARM and other architectures we should try to do both.

[gnzlbg]

These intrinsics have been deprecated.