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

cmd/compile: possible past-the-end pointer in range loop #56699

Closed
randall77 opened this issue Nov 11, 2022 · 1 comment
Closed

cmd/compile: possible past-the-end pointer in range loop #56699

randall77 opened this issue Nov 11, 2022 · 1 comment
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge release-blocker
Milestone

Comments

@randall77
Copy link
Contributor

func f(s []any) {
	for _, a := range s {
		g(a)
	}
}
func g(a any)

Compile this and we get the inner loop:

	0x0022 00034 (/Users/khr/gowork/tmp1.go:4)	MOVQ	CX, main..autotmp_8+16(SP)
	0x0027 00039 (/Users/khr/gowork/tmp1.go:4)	MOVQ	AX, main..autotmp_9+24(SP)
	0x002c 00044 (/Users/khr/gowork/tmp1.go:4)	MOVQ	(AX), CX
	0x002f 00047 (/Users/khr/gowork/tmp1.go:4)	MOVQ	8(AX), BX
	0x0033 00051 (/Users/khr/gowork/tmp1.go:5)	MOVQ	CX, AX
	0x0036 00054 (/Users/khr/gowork/tmp1.go:5)	PCDATA	$1, $1
	0x0036 00054 (/Users/khr/gowork/tmp1.go:5)	CALL	main.g(SB)
	0x003b 00059 (/Users/khr/gowork/tmp1.go:4)	MOVQ	main..autotmp_9+24(SP), AX
	0x0040 00064 (/Users/khr/gowork/tmp1.go:4)	ADDQ	$16, AX
	0x0044 00068 (/Users/khr/gowork/tmp1.go:4)	MOVQ	main..autotmp_8+16(SP), CX
	0x0049 00073 (/Users/khr/gowork/tmp1.go:4)	INCQ	CX
	0x004c 00076 (/Users/khr/gowork/tmp1.go:4)	MOVQ	main.s+56(SP), BX
	0x0051 00081 (/Users/khr/gowork/tmp1.go:4)	CMPQ	BX, CX
	0x0054 00084 (/Users/khr/gowork/tmp1.go:4)	JGT	34

Note that autotmp_9+24 contains the spilled value of the current iteration pointer, which we increment by 16 (the size of any) at the end of each iteration.

During the scheduling pass, we need to decide which to do first, the call to g or the increment of the pointer. In the case above, we decided to do the call first and then the increment. That is the safe order, because then we spill the pre-incremented pointer.
If instead we chose to do the increment first, then the call, then we spill the post-incremented pointer. That's bad, as when we are inside g the spilled pointer is live on the stack (and scanned precisely, not conservatively), and may point past the end of the slice's underlying object.

This is an unintended consequence of CL 414876. There I thought that the past-the-end pointer was never live across any call, but that's not true if it gets scheduled before an unconditional call at the end of the loop body, and then spilled.

I don't have any example yet where this issue actually causes a problem at tip. Currently the scheduler accidentally always(?) does the increment after any calls. But changes I've been working on in the scheduler fail to maintain this unspoken invariant and cause programs to fail badly.

There's currently no ordering edge that forces the add to happen as the last thing in the loop body. That information is there early on in SSA, when the ptr->uintptr->ptr conversions are still explicit and have memory args, but that memory arg goes away in the opt pass and we no longer have any ordering constraint in the SSA graph to preclude the add from happening before the call.

@randall77 randall77 added this to the Go1.20 milestone Nov 11, 2022
@randall77 randall77 self-assigned this Nov 11, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 11, 2022
@gopherbot
Copy link

Change https://go.dev/cl/449995 mentions this issue: cmd/compile: be more careful about pointer incrementing in range loops

@golang golang locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

2 participants