Use probe-stack=inline-asm in LLVM 11 #77885

erikdesjardins · 2020-10-13T02:37:48Z

Fixes (?) #74405, related to #43241

r? @cuviper

src/test/assembly/stack-probes.rs

cuviper · 2020-10-14T21:50:00Z

Thanks! This may have perf implications, so I'm excluding it from rollups.

@bors r rollup=never

bors · 2020-10-14T21:50:01Z

📌 Commit 95269c2d2ee2d59305d0599f0b389f4583cdb5ea has been approved by cuviper

bors · 2020-10-15T01:10:46Z

⌛ Testing commit 95269c2d2ee2d59305d0599f0b389f4583cdb5ea with merge 1272a1aeb3d394ebcf93f59298e04cd63d45e050...

bors · 2020-10-15T01:43:17Z

💔 Test failed - checks-actions

cuviper · 2020-10-15T16:22:37Z

The specific failures start here. I'm not sure why this change would cause new stack overflows, but it needs investigation.

failures:

---- [ui] ui/unsized-locals/by-value-trait-object-safety-rpass.rs stdout ----

error: test run failed!
status: signal: 6
command: "/checkout/obj/build/i686-unknown-linux-gnu/test/ui/unsized-locals/by-value-trait-object-safety-rpass/a"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------

thread 'main' has overflowed its stack
fatal runtime error: stack overflow

------------------------------------------


---- [ui] ui/unsized-locals/by-value-trait-object-safety-withdefault.rs stdout ----

error: test run failed!
status: signal: 6
command: "/checkout/obj/build/i686-unknown-linux-gnu/test/ui/unsized-locals/by-value-trait-object-safety-withdefault/a"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------

thread 'main' has overflowed its stack
fatal runtime error: stack overflow

------------------------------------------


---- [ui] ui/unsized-locals/autoderef.rs stdout ----

error: test run failed!
status: signal: 6
command: "/checkout/obj/build/i686-unknown-linux-gnu/test/ui/unsized-locals/autoderef/a"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------

thread 'main' has overflowed its stack
fatal runtime error: stack overflow

------------------------------------------



failures:
    [ui] ui/unsized-locals/autoderef.rs
    [ui] ui/unsized-locals/by-value-trait-object-safety-rpass.rs
    [ui] ui/unsized-locals/by-value-trait-object-safety-withdefault.rs

test result: FAILED. 10827 passed; 3 failed; 109 ignored; 0 measured; 0 filtered out

erikdesjardins · 2020-10-17T18:39:48Z

Looks like a comparison is inverted. This is a code snippet from by-value-trait-object-safety-rpass. Immediately before this, the size of the alloca is loaded into ebx, which in our case is 0 because the unsized trait object has size 0.

	add	ebx, 15
	and	ebx, -16
	mov	eax, esp
	sub	eax, ebx
	mov	dword ptr [ebp - 368], edx
	mov	dword ptr [ebp - 372], esi
	mov	dword ptr [ebp - 376], edi
	mov	dword ptr [ebp - 380], eax
.LBB87_43:
	mov	eax, dword ptr [ebp - 380]
	cmp	eax, esp
	jl	.LBB87_45
	mov	dword ptr [esp], 0
	sub	esp, 4096
	jmp	.LBB87_43
.LBB87_45:
	mov	eax, dword ptr [ebp - 380]
	mov	esp, eax

Due to:

cmp	eax, esp
jl	.LBB87_45

...if ebx is nonzero, the new stack pointer in eax will be below esp, and it will exit the loop without probing.
...if ebx is zero, the new stack pointer in eax will be equal to, and then greater than esp, and it will probe infinitely.
(and that's what happens running it locally under gdb)

The same thing shows up in Clang 11 with -fstack-clash-protection (https://godbolt.org/z/vxbaxd) with just:

int size;

void foo(void*);

int main() {
    foo(alloca(size));
}

I suppose nobody noticed this in C because it's rare to call alloca(0), and alloca(<nonzero>) will still run, it just won't actually probe anything.

Assuming I haven't misinterpreted something, can you report this to LLVM? I don't have an account yet.

pietroalbini · 2020-10-19T10:53:58Z

@bors r-

The PR failed but bors forgot about that during synchronize.

cuviper · 2020-10-19T17:36:49Z

Hmm, I thought we fixed that cmp direction in D82867, but it's possible we missed a case...
(edit: actually, I think that change may have mis-corrected the cmp for dynamic alloca... still investigating...)

erikdesjardins · 2020-10-19T22:19:35Z

Yeah, the changes to stack-clash-dynamic-alloca.ll in that revision look wrong; it was correct before (although maybe replace jl .LBB0_3 with jle to avoid probing for zero size allocas).
stack-clash-large.ll looks right if it's guaranteed that we always probe an exact multiple of the page size for static allocas.

serge-sans-paille · 2020-10-27T10:22:47Z

@erikdesjardins / @cuviper https://reviews.llvm.org/D90216 should do the trick. Thanks for spotting this!

cuviper · 2020-10-27T19:55:31Z

Ah, so there are two parts to this...

I suppose nobody noticed this in C because it's rare to call alloca(0),

This actually was already reported in bug 47657 and fixed in D88548.

and alloca(<nonzero>) will still run, it just won't actually probe anything.

This problem remains, but I just confirmed locally that it is fixed by D90216. I stepped through the assembly in gdb just to be sure. 🙂

I think both fixes would be good to have in LLVM 11.0.1. For Rust's part, we can backport the fixes to our bundled LLVM fork, but we should be careful about how this is enabled for external LLVM. I don't think we usually check the patch version for functionality, but maybe this one justifies it -- assuming 11.0.1 does get fixed.

- Perform the probing in the correct direction. Related to rust-lang/rust#77885 (comment) - The first touch on a dynamic alloca cannot use a mov because it clobbers existing space. Use a xor 0 instead Differential Revision: https://reviews.llvm.org/D90216

crlf0710 · 2020-11-13T12:21:03Z

Triage: So it seems this is blocked on a llvm bug.

- Perform the probing in the correct direction. Related to rust-lang/rust#77885 (comment) - The first touch on a dynamic alloca cannot use a mov because it clobbers existing space. Use a xor 0 instead Differential Revision: https://reviews.llvm.org/D90216 (cherry picked from commit 0f60bcc)

…everted here.

pnkfelix · 2021-03-23T15:42:18Z

src/test/codegen/stack-probes.rs

@@ -13,11 13,12 @@
 // ignore-emscripten
 // ignore-windows
 // compile-flags: -C no-prepopulate-passes
+// min-llvm-version: 11.0.1


by the way, doesn't adding this mean that we are effectively no longer unit-testing our old stack-probe strategy, since we will just skip this test if the LLVM version is not sufficiently new?

Maybe we should have two files, one for LLVM < 11.0.1 and one for LLVM >= 11.0.1?

We don't generally test very meticulously the setups that use LLVM versions outside of what is in the Rust repository. This is reflected in e.g. tests for all our (oft backported) soundness fixes only being applicable to the newest LLVM versions and our fork.

So far it has been the unwritten policy that it is up to the users who build with alternative LLVM versions to ensure that their builds do what they expect. (This is also another of those things that I should eventually look into codifying in some document outlining our LLVM support strategy)

…7885-for-issue-83139, r=nagisa [stable] probe-stack=call everywhere again, for now. To buy time on issue 83139, revert effect of PR 77885: We will not conditionally enable probe-stack=inline-asm on LLVM 11 anymore on any of our targets that opted into doing so on PR rust-lang#77885 (and were subsequently configured to do so in a fine grained manner on PR rust-lang#80838). After we resolve 83139 (potentially by backporting a fix to LLVM, or potentially by deciding that one cannot rely on the quality of our DWARF output in the manner described in issue 83139), we can change this back. cc rust-lang#83139

- Perform the probing in the correct direction. Related to rust-lang/rust#77885 (comment) - The first touch on a dynamic alloca cannot use a mov because it clobbers existing space. Use a xor 0 instead Differential Revision: https://reviews.llvm.org/D90216

Pkgsrc changes: * Remove one SunOS patch, apparently no longer needed. * Adapt one patch for Darwin, adjust cargo checksum accordingly. * Adjust bootstraps to version 1.50.0. Version 1.51.0 (2021-03-25) ============================ Language -------- - [You can now parameterize items such as functions, traits, and `struct`s by constant values in addition to by types and lifetimes.][79135] Also known as "const generics" E.g. you can now write the following. Note: Only values of primitive integers, `bool`, or `char` types are currently permitted. ```rust struct GenericArray<T, const LENGTH: usize> { inner: [T; LENGTH] } impl<T, const LENGTH: usize> GenericArray<T, LENGTH> { const fn last(&self) -> Option<&T> { if LENGTH == 0 { None } else { Some(&self.inner[LENGTH - 1]) } } } ``` Compiler -------- - [Added the `-Csplit-debuginfo` codegen option for macOS platforms.][79570] This option controls whether debug information is split across multiple files or packed into a single file. **Note** This option is unstable on other platforms. - [Added tier 3\* support for `aarch64_be-unknown-linux-gnu`, `aarch64-unknown-linux-gnu_ilp32`, and `aarch64_be-unknown-linux-gnu_ilp32` targets.][81455] - [Added tier 3 support for `i386-unknown-linux-gnu` and `i486-unknown-linux-gnu` targets.][80662] - [The `target-cpu=native` option will now detect individual features of CPUs.][80749] - [Rust now uses `inline-asm` for stack probes when used with LLVM 11.0.1 ][77885] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`Box::downcast` is now also implemented for any `dyn Any Send Sync` object.][80945] - [`str` now implements `AsMut<str>`.][80279] - [`u64` and `u128` now implement `From<char>`.][79502] - [`Error` is now implemented for `&T` where `T` implements `Error`.][75180] - [`Poll::{map_ok, map_err}` are now implemented for `Poll<Option<Result<T, E>>>`.][80968] - [`unsigned_abs` is now implemented for all signed integer types.][80959] - [`io::Empty` now implements `io::Seek`.][78044] - [`rc::Weak<T>` and `sync::Weak<T>`'s methods such as `as_ptr` are now implemented for `T: ?Sized` types.][80764] Stabilized APIs --------------- - [`Arc::decrement_strong_count`] - [`Arc::increment_strong_count`] - [`Once::call_once_force`] - [`Peekable::next_if_eq`] - [`Peekable::next_if`] - [`Seek::stream_position`] - [`array::IntoIter`] - [`panic::panic_any`] - [`ptr::addr_of!`] - [`ptr::addr_of_mut!`] - [`slice::fill_with`] - [`slice::split_inclusive_mut`] - [`slice::split_inclusive`] - [`slice::strip_prefix`] - [`slice::strip_suffix`] - [`str::split_inclusive`] - [`sync::OnceState`] - [`task::Wake`] Cargo ----- - [Added the `split-debuginfo` profile option to control the -Csplit-debuginfo codegen option.][cargo/9112] - [Added the `resolver` field to `Cargo.toml` to enable the new feature resolver and CLI option behavior.][cargo/8997] Version 2 of the feature resolver will try to avoid unifying features of dependencies where that unification could be unwanted. Such as using the same dependency with a `std` feature in a build scripts and proc-macros, while using the `no-std` feature in the final binary. See the [Cargo book documentation][[email protected]] for more information on the feature. Rustdoc ------- - [Rustdoc will now include documentation for methods available from `Deref` traits.][80653] - [You can now provide a `--default-theme` flag which sets the default theme to use for documentation.][79642] Various improvements to intra-doc links: - [You can link to non-path primitives such as `slice`.][80181] - [You can link to associated items.][74489] - [You can now include generic parameters when linking to items, like `Vec<T>`.][76934] Misc ---- - [You can now pass `--include-ignored` to tests (e.g. with `cargo test -- --include-ignored`) to include testing tests marked `#[ignore]`.][80053] Compatibility Notes ------------------- - [WASI platforms no longer use the `wasm-bindgen` ABI, and instead use the wasm32 ABI.][79998] - [`rustc` no longer promotes division, modulo and indexing operations to `const` that could fail.][80579] - [The minimum version of glibc for the following platforms has been bumped to version 2.31 for the distributed artifacts.][81521] - `armv5te-unknown-linux-gnueabi` - `sparc64-unknown-linux-gnu` - `thumbv7neon-unknown-linux-gnueabihf` - `armv7-unknown-linux-gnueabi` - `x86_64-unknown-linux-gnux32` Internal Only ------------- - [Consistently avoid constructing optimized MIR when not doing codegen][80718] [79135]: rust-lang/rust#79135 [74489]: rust-lang/rust#74489 [76934]: rust-lang/rust#76934 [79570]: rust-lang/rust#79570 [80181]: rust-lang/rust#80181 [79642]: rust-lang/rust#79642 [80945]: rust-lang/rust#80945 [80279]: rust-lang/rust#80279 [80053]: rust-lang/rust#80053 [79502]: rust-lang/rust#79502 [75180]: rust-lang/rust#75180 [79135]: rust-lang/rust#79135 [81521]: rust-lang/rust#81521 [80968]: rust-lang/rust#80968 [80959]: rust-lang/rust#80959 [80718]: rust-lang/rust#80718 [80653]: rust-lang/rust#80653 [80579]: rust-lang/rust#80579 [79998]: rust-lang/rust#79998 [78044]: rust-lang/rust#78044 [81455]: rust-lang/rust#81455 [80764]: rust-lang/rust#80764 [80749]: rust-lang/rust#80749 [80662]: rust-lang/rust#80662 [77885]: rust-lang/rust#77885 [cargo/8997]: rust-lang/cargo#8997 [cargo/9112]: rust-lang/cargo#9112 [[email protected]]: https://doc.rust-lang.org/nightly/cargo/reference/features.html#feature-resolver-version-2 [`Once::call_once_force`]: https://doc.rust-lang.org/stable/std/sync/struct.Once.html#method.call_once_force [`sync::OnceState`]: https://doc.rust-lang.org/stable/std/sync/struct.OnceState.html [`panic::panic_any`]: https://doc.rust-lang.org/stable/std/panic/fn.panic_any.html [`slice::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.strip_prefix [`slice::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.strip_prefix [`Arc::increment_strong_count`]: https://doc.rust-lang.org/nightly/std/sync/struct.Arc.html#method.increment_strong_count [`Arc::decrement_strong_count`]: https://doc.rust-lang.org/nightly/std/sync/struct.Arc.html#method.decrement_strong_count [`slice::fill_with`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.fill_with [`ptr::addr_of!`]: https://doc.rust-lang.org/nightly/std/ptr/macro.addr_of.html [`ptr::addr_of_mut!`]: https://doc.rust-lang.org/nightly/std/ptr/macro.addr_of_mut.html [`array::IntoIter`]: https://doc.rust-lang.org/nightly/std/array/struct.IntoIter.html [`slice::split_inclusive`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.split_inclusive [`slice::split_inclusive_mut`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.split_inclusive_mut [`str::split_inclusive`]: https://doc.rust-lang.org/nightly/std/primitive.str.html#method.split_inclusive [`task::Wake`]: https://doc.rust-lang.org/nightly/std/task/trait.Wake.html [`Seek::stream_position`]: https://doc.rust-lang.org/nightly/std/io/trait.Seek.html#method.stream_position [`Peekable::next_if`]: https://doc.rust-lang.org/nightly/std/iter/struct.Peekable.html#method.next_if [`Peekable::next_if_eq`]: https://doc.rust-lang.org/nightly/std/iter/struct.Peekable.html#method.next_if_eq

…verted in this PR.

…in, for now. We had already reverted the change on stable back in PR rust-lang#83412. Since then, we've had some movement on issue rust-lang#83139, but not a 100% fix. But also since then, we had bug reported, issue rust-lang#84667, that looks like outright codegen breakage, rather than problems confined to debuginfo issues. So we are reverting PR rust-lang#77885 on stable and beta. We'll reland PR rust-lang#77885 (or some variant) switching back to an LLVM-dependent selection of out-of-line call vs inline-asm, after these other issues have been resolved.

…verted in this PR.

…ark-Simulacrum Revert PR 77885 everywhere Change to probe-stack=call (instead of inline-or-call) everywhere again, for now. We had already reverted the change on stable back in PR rust-lang#83412. Since then, we've had some movement on issue rust-lang#83139, but not a 100% fix. But also since then, we had bug reported, issue rust-lang#84667, that looks like outright codegen breakage, rather than problems confined to debuginfo issues. So we are reverting PR rust-lang#77885 on stable and beta. We'll reland PR rust-lang#77885 (or some variant) switching back to an LLVM-dependent selection of out-of-line call vs inline-asm, after these other issues have been resolved.

…verted in this PR.

…imulacrum [stable] 1.52.0 release This includes the release notes (rust-lang#84183) as well as cherry-picked commits from: * [beta] revert PR rust-lang#77885 rust-lang#84710 * [beta] remove assert_matches rust-lang#84759 * Revert PR 81473 to resolve (on beta) issues 81626 and 81658. rust-lang#83171 * [beta] rustdoc revert deref recur rust-lang#84868 * Fix ICE of for-loop mut borrowck where no suggestions are available rust-lang#83401 Additionally in "fresh work" we're also: * reverting: directly expose copy and copy_nonoverlapping intrinsics rust-lang#81238 to avoid rust-lang#84297 on 1.52

- Perform the probing in the correct direction. Related to rust-lang/rust#77885 (comment) - The first touch on a dynamic alloca cannot use a mov because it clobbers existing space. Use a xor 0 instead Differential Revision: https://reviews.llvm.org/D90216 (cherry picked from commit 3c4d49e)

Enable inline stack probes on X86 with LLVM 16 The known problems with x86 inline-asm stack probes have been solved on LLVM main (16), so this flips the switch. Anyone using bleeding-edge LLVM with rustc can start testing this, as I have done locally. We'll get more direct rust-ci when LLVM 16 branches and we start our upgrade, and we can always patch or disable it then if we find new problems. The previous attempt was rust-lang#77885, reverted in rust-lang#84708.

- Perform the probing in the correct direction. Related to rust-lang/rust#77885 (comment) - The first touch on a dynamic alloca cannot use a mov because it clobbers existing space. Use a xor 0 instead Differential Revision: https://reviews.llvm.org/D90216

Enable inline stack probes on X86 with LLVM 16 The known problems with x86 inline-asm stack probes have been solved on LLVM main (16), so this flips the switch. Anyone using bleeding-edge LLVM with rustc can start testing this, as I have done locally. We'll get more direct rust-ci when LLVM 16 branches and we start our upgrade, and we can always patch or disable it then if we find new problems. The previous attempt was rust-lang#77885, reverted in rust-lang#84708.

rust-highfive assigned cuviper Oct 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2020

erikdesjardins commented Oct 13, 2020

View reviewed changes

src/test/assembly/stack-probes.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 15, 2020

cuviper added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2020

bjorn3 mentioned this pull request Oct 18, 2020

Cranelift: Support stack probes without external function call bytecodealliance/wasmtime#2299

Closed

crlf0710 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2020

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Mar 23, 2021

removed tests introduced or changed by PR rust-lang#77885, which is r…

b75894e

…everted here.

pnkfelix reviewed Mar 23, 2021

View reviewed changes

This was referenced Apr 29, 2021

In latest nightly observing a to-be returned variable changes its value. #84667

Closed

Revert PR 77885 everywhere #84708

Merged

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Apr 29, 2021

Remove tests introduced or cahnged by PR rust-lang#77885, which is re…

6eceacf

…verted in this PR.

pnkfelix mentioned this pull request Apr 29, 2021

[beta] revert PR #77885 #84710

Closed

bors pushed a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2021

Remove tests introduced or cahnged by PR rust-lang#77885, which is re…

db4c544

…verted in this PR.

Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request May 3, 2021

Remove tests introduced or cahnged by PR rust-lang#77885, which is re…

463b3a2

…verted in this PR.

Mark-Simulacrum mentioned this pull request May 3, 2021

[stable] 1.52.0 release #84864

Merged

pietroalbini mentioned this pull request May 4, 2021

[beta] Prepare beta 1.53.0 #84909

Merged

ojeda mentioned this pull request Mar 9, 2022

rustc wanted features & bugfixes Rust-for-Linux/linux#355

Open

47 tasks

cuviper mentioned this pull request Sep 30, 2022

Enable inline stack probes on X86 with LLVM 16 #102503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use probe-stack=inline-asm in LLVM 11 #77885

Use probe-stack=inline-asm in LLVM 11 #77885

erikdesjardins commented Oct 13, 2020

cuviper commented Oct 14, 2020

bors commented Oct 14, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

cuviper commented Oct 15, 2020

erikdesjardins commented Oct 17, 2020 •

edited

Loading

pietroalbini commented Oct 19, 2020

cuviper commented Oct 19, 2020 •

edited

Loading

erikdesjardins commented Oct 19, 2020 •

edited

Loading

serge-sans-paille commented Oct 27, 2020

cuviper commented Oct 27, 2020

crlf0710 commented Nov 13, 2020

pnkfelix Mar 23, 2021

nagisa Mar 23, 2021

Use probe-stack=inline-asm in LLVM 11 #77885

Use probe-stack=inline-asm in LLVM 11 #77885

Conversation

erikdesjardins commented Oct 13, 2020

cuviper commented Oct 14, 2020

bors commented Oct 14, 2020

bors commented Oct 15, 2020

bors commented Oct 15, 2020

cuviper commented Oct 15, 2020

erikdesjardins commented Oct 17, 2020 • edited Loading

pietroalbini commented Oct 19, 2020

cuviper commented Oct 19, 2020 • edited Loading

erikdesjardins commented Oct 19, 2020 • edited Loading

serge-sans-paille commented Oct 27, 2020

cuviper commented Oct 27, 2020

crlf0710 commented Nov 13, 2020

pnkfelix Mar 23, 2021

Choose a reason for hiding this comment

nagisa Mar 23, 2021

Choose a reason for hiding this comment

erikdesjardins commented Oct 17, 2020 •

edited

Loading

cuviper commented Oct 19, 2020 •

edited

Loading

erikdesjardins commented Oct 19, 2020 •

edited

Loading