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

hint::spin_loop invokes undefined behavior on targets without SSE2 #59237

Closed
gnzlbg opened this issue Mar 16, 2019 · 4 comments
Closed

hint::spin_loop invokes undefined behavior on targets without SSE2 #59237

gnzlbg opened this issue Mar 16, 2019 · 4 comments
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-x86_32 Target: x86 processors, 32 bit (like i686-*) O-x86_64 Target: x86-64 processors (like x86_64-*)

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 16, 2019

Currently hint::spin_loop is implemented with the pause instruction on all x86 / x86_64 targets, but this instruction is only available on CPUs with SSE2. The behavior of pause on CPUs without SSE2 is undefined.


This hint should not be implemented with inline assembly, but should call the appropriate core::arch intrinsics instead. Those explicitly state which features are required for each operation on each target.

gnzlbg added a commit to gnzlbg/rust that referenced this issue Mar 16, 2019
The pause instruction requires SSE2 but was being unconditionally used
on targets without it, resulting in undefined behavior.

This PR fixes that by only using the pause intrinsic if SSE2 is available.

It also removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.

Closes rust-lang#59237 .
@Centril Centril added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Mar 16, 2019
@Fanael
Copy link

Fanael commented Mar 17, 2019

This is incorrect: pause is defined in a way that makes it a harmless NOP on all earlier CPUs, and is even explicitly defined as such in Intel manuals:

This instruction was introduced in the Pentium 4 processors, but is backward compatible with all IA-32 processors. In earlier IA-32 processors, the PAUSE instruction operates like a NOP instruction.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 17, 2019

@Fanael indeed. Is there an advantage to emitting pause in earlier processors with respect to emitting nothing ?

@Fanael
Copy link

Fanael commented Mar 17, 2019

Not that I know, there is no advantage on any of Intel's old microarchitectures, they just interpret it as a nop and have no special way of handling spin loops. It might be different on AMD's K7 or VIA Nehemiah, but I don't have that hardware to test, and I doubt it makes any difference anyway. It lets you avoid an ugly compile time conditional and that's it I think.

@kennytm kennytm added O-x86 O-x86_64 Target: x86-64 processors (like x86_64-*) labels Mar 17, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 17, 2019

This bug report is invalid, thanks @Fanael .

@gnzlbg gnzlbg closed this as completed Mar 17, 2019
gnzlbg added a commit to gnzlbg/rust that referenced this issue Mar 21, 2019
The pause instruction requires SSE2 but was being unconditionally used
on targets without it, resulting in undefined behavior.

This PR fixes that by only using the pause intrinsic if SSE2 is available.

It also removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.

Closes rust-lang#59237 .
@Noratrieb Noratrieb added O-x86_32 Target: x86 processors, 32 bit (like i686-*) and removed O-x86-all labels Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-x86_32 Target: x86 processors, 32 bit (like i686-*) O-x86_64 Target: x86-64 processors (like x86_64-*)
Projects
None yet
Development

No branches or pull requests

5 participants