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

@bors r+ rollup=never

📌 Commit 95269c2 has been approved by cuviper

⌛ Testing commit 95269c2 with merge 1272a1a...

💔 Test failed - checks-actions

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

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.

@bors r-

The PR failed but bors forgot about that during synchronize.

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...)

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.

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

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.

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

Awaiting bors try build completion.

⌛ Trying commit b28b231 with merge 088a806...

☀️ Try build successful - checks-actions
Build commit: 088a806 (088a806ae338b7577aa6f01e5700a94f9feb139c)

Queued 088a806 with parent 937f629, future comparison URL.

@rustbot label: +S-waiting-on-perf

Finished benchmarking try commit (088a806): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

Now that #80796 has merged, can you rebase again to separate this PR from that change?

Rebased.

@bors r+

📌 Commit cd25807 has been approved by cuviper

⌛ Testing commit cd25807 with merge 635ccfe...

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 635ccfe to master...