Strange behaviour with loops [1.15 backport] #45187

gopherbot · 2021-03-23T20:11:48Z

@randall77 requested issue #45175 to be considered for backport to the next 1.15 minor release.

Here's a simpler reproducer:
package main

//go:noinline
func f(c bool) int {
	b := true
	x := 0
	y := 1
	for b {
		b = false
		y = x
		x = 2
		if c {
			return 3
		}
	}
	return y
}

func main() {
	println(f(false))
}
This reproducer incorrectly prints 2.
It should print 0. And it does, with -gcflags=-d=ssa/short_circuit/off.
The problem is that during short circuit, we don't take into account that the arguments to a phi are from the previous iteration, not the current iteration. That confusion leads to assigning y the current iteration's value of x, not the previous iteration's value of x.

The bug looks like it started in 1.15.

In shortcircuit.go, we need to be a bit more choosy in replaceUses to not replace uses inside a phi, as we're replacing the current iteration's values, not those which are from the previous iteration. At least, something like that; it may not be quite so simple.

@josharian

@gopherbot please open backport issues.

The text was updated successfully, but these errors were encountered:

gopherbot · 2021-03-24T18:12:23Z

Change https://golang.org/cl/304529 mentions this issue: [release-branch.go1.15] cmd/compile: disable shortcircuit optimization for intertwined phi values

randall77 · 2021-03-25T17:44:02Z

This bug is rare but causes incorrect code to be generated for loops.

dmitshur · 2021-03-25T21:30:05Z

Approving per discussion in a release meeting. This backport applies to both 1.16 (#45192) and 1.15 (this issue).

…n for intertwined phi values We need to be careful that when doing value graph surgery, we not re-substitute a value that has already been substituted. That can lead to confusing a previous iteration's value with the current iteration's value. The simple fix in this CL just aborts the optimization if it detects intertwined phis (a phi which is the argument to another phi). It might be possible to keep the optimization with a more complicated CL, but: 1) This CL is clearly safe to backport. 2) There were no instances of this abort triggering in all.bash, prior to the test introduced in this CL. Fixes #45187 Change-Id: I2411dca03948653c053291f6829a76bec0c32330 Reviewed-on: https://go-review.googlesource.com/c/go/+/304251 Trust: Keith Randall <khr@golang.org> Trust: Josh Bleecher Snyder <josharian@gmail.com> Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com> (cherry picked from commit 771c57e) Reviewed-on: https://go-review.googlesource.com/c/go/+/304529

gopherbot · 2021-03-31T14:39:26Z

Closed by merging f17b659 to release-branch.go1.15.

Pick up fixes to the following Go bugs: * golang/go#45076 * golang/go#45187 * golang/go#42884 * [ ] Adjust the Pebble tests to run in new version. * [x] Adjust version in Docker image ([source](./builder/Dockerfile)). * [x] Rebuild and push the Docker image (following [Basic Process](#basic-process)) * [x] Bump the version in `WORKSPACE` under `go_register_toolchains`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases). * [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)). * [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release. * [ ] Bump the go version in `go.mod`. You may also need to rerun `make vendor_rebuild` if vendoring has changed. * [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh#L40-42)). * [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`). * [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects. * [x] Adjust `GO_VERSION` in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh)) and ask the Developer Infrastructure team to deploy new images. Resolves cockroachdb#63836. Release note (general change): Upgrade the CRDB binary to Go 1.15.10

63785: sql: use catalog.Mutation, catalog.Column and catalog.Index where possible r=postamar a=postamar sql: use catalog.Mutation where possible This commit is a refactor which introduces catalog.Mutation wherever possible, in an effort to avoid using the descpb.DescriptorMutation type directly. Release note: None sql: use catalog.Column and catalog.Index where possible This commit is a refactor which introduces the catalog.Column and catalog.Index interfaces wherever possible. These had recently been added in an effort to avoid using the descpb.ColumnDescriptor and descpb.IndexDescriptor types directly. This commit generalizes their usage throughout the sql packages. Fixes #63755. Release note: None 63787: roachtest: added hibernate ignorelist for 21.1 r=rafiss a=mnovelodou Previously, some random issues with hibernate were afecting roachtest This was inadequate because they are false failures To address this, this patch set these tests in ignore list Release note: none 63839: cli: bump the cobra dependency and add autocompletion for fish r=stevendanna a=knz Fixes #63838 . Fixes #50187. This updates the cobra dep which hadn't been updated since 2017. Also picks up the new support for fish autocompletions. Release note (cli change): Certain errors caused by invalid command-line arguments are now printed on the process' standard error stream, instead of standard output. Release note (cli change): The `cockroach gen autocomplete` command has been updated and can now produce autocompletion definitions for the `fish` shell. 63853: build: upgrade go to 1.15.11 r=rail a=rickystewart Pick up fixes to the following Go bugs: * golang/go#45076 * golang/go#45187 * golang/go#42884 * [ ] Adjust the Pebble tests to run in new version. * [x] Adjust version in Docker image ([source](./builder/Dockerfile)). * [x] Rebuild and push the Docker image (following [Basic Process](#basic-process)) * [x] Bump the version in `WORKSPACE` under `go_register_toolchains`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases). * [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)). * [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release. * [ ] Bump the go version in `go.mod`. You may also need to rerun `make vendor_rebuild` if vendoring has changed. * [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh#L40-42)). * [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`). * [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects. * [x] Adjust `GO_VERSION` in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh)) and ask the Developer Infrastructure team to deploy new images. Resolves #63836. Release note (general change): Upgrade the CRDB binary to Go 1.15.10 63857: changefeedccl: Make `kafka_sink_config` a valid option. r=stevendanna a=miretskiy Add `kafka_sink_config` to the list of allowed changefeed options. Release Notes: None Co-authored-by: Marius Posta <marius@cockroachlabs.com> Co-authored-by: MiguelNovelo <miguel.novelo@digitalonus.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>

Pick up fixes to the following Go bugs: * golang/go#45076 * golang/go#45187 * golang/go#42884 * [ ] Adjust the Pebble tests to run in new version. * [x] Adjust version in Docker image ([source](./builder/Dockerfile)). * [x] Rebuild and push the Docker image (following [Basic Process](#basic-process)) * [x] Bump the version in `WORKSPACE` under `go_register_toolchains`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases). * [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)). * [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release. * [ ] Bump the go version in `go.mod`. You may also need to rerun `make vendor_rebuild` if vendoring has changed. * [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh#L40-42)). * [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`). * [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects. * [x] Adjust `GO_VERSION` in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh)) and ask the Developer Infrastructure team to deploy new images. Resolves cockroachdb#63836. Release note (general change): Upgrade the CRDB binary to Go 1.15.10

Technically, the minimum required is 1.15.3: https://github.com/cockroachdb/cockroach/blob/release-21.1/build/go-version-check.sh#L10 However, 1.15.11 avoids these Go bugs: golang/go#45076 golang/go#45187 golang/go#42884 So to prevent users from running into those, we listing 1.15.11 as the minimum required version, followed by a note that you can use IGNORE_GOVERS=1 to try building with older versions. Fixes #10468 Fixes #9081

gopherbot added the CherryPickCandidate Used during the release process for point releases label Mar 23, 2021

gopherbot mentioned this issue Mar 23, 2021

Strange behaviour with loops #45175

Closed

gopherbot added this to the Go1.15.11 milestone Mar 23, 2021

randall77 self-assigned this Mar 23, 2021

dmitshur mentioned this issue Mar 25, 2021

Strange behaviour with loops [1.16 backport] #45192

Closed

dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Mar 25, 2021

gopherbot closed this as completed Mar 31, 2021

knz mentioned this issue Apr 19, 2021

build: upgrade builder image to go 1.15.11 cockroachdb/cockroach#63836

Closed

rickystewart mentioned this issue Apr 19, 2021

build: upgrade go to 1.15.11 cockroachdb/cockroach#63853

Merged

11 tasks

rickystewart mentioned this issue Apr 19, 2021

release-21.1: build: upgrade go to 1.15.11 cockroachdb/cockroach#63865

Merged

11 tasks

jseldess mentioned this issue May 4, 2021

Update Go version requirement for building 21.1 from source cockroachdb/docs#10489

Merged

golang locked and limited conversation to collaborators Mar 31, 2022

gopherbot added the FrozenDueToAge label Mar 31, 2022

rsc unassigned randall77 Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strange behaviour with loops [1.15 backport] #45187

Strange behaviour with loops [1.15 backport] #45187

gopherbot commented Mar 23, 2021

gopherbot commented Mar 24, 2021

randall77 commented Mar 25, 2021

dmitshur commented Mar 25, 2021

gopherbot commented Mar 31, 2021

Strange behaviour with loops [1.15 backport] #45187

Strange behaviour with loops [1.15 backport] #45187

Comments

gopherbot commented Mar 23, 2021

gopherbot commented Mar 24, 2021

randall77 commented Mar 25, 2021

dmitshur commented Mar 25, 2021

gopherbot commented Mar 31, 2021