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: function inlining produces incorrect results in certain conditions [1.12 backport] #30567

Closed
gopherbot opened this issue Mar 4, 2019 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@gopherbot
Copy link

@mvdan requested issue #30566 to be considered for backport to the next 1.12 minor release.

@gopherbot please backport to 1.12.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Mar 4, 2019
@gopherbot gopherbot added this to the Go1.12.1 milestone Mar 4, 2019
@gopherbot
Copy link
Author

Change https://golang.org/cl/165858 mentions this issue: [release-branch.go1.12] cmd/compile: fix ordering for short-circuiting ops

@julieqiu
Copy link
Member

@mvdan - there isn't a reason provided in the gopherbot message. Would you mind providing one for this backport?

@randall77
Copy link
Contributor

There's an underlying bug which is fixed with the CL linked above. That bug has been in existence since at least 1.2.
However, with temporary variable reuse introduced in 1.12, it now causes actual incorrect results more often. So I think it qualifies for a backport.

@bradfitz bradfitz added the CherryPickApproved Used during the release process for point releases label Mar 13, 2019
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Mar 13, 2019
@gopherbot
Copy link
Author

Closed by merging f062f48 to release-branch.go1.12.

gopherbot pushed a commit that referenced this issue Mar 13, 2019
…g ops

Make sure the side effects inside short-circuited operations (&& and ||)
happen correctly.

Before this CL, we attached the side effects to the node itself using
exprInPlace. That caused other side effects in sibling expressions
to get reordered with respect to the short circuit side effect.

Instead, rewrite a && b like:

r := a
if r {
  r = b
}

That code we can keep correctly ordered with respect to other
side-effects extracted from part of a big expression.

exprInPlace seems generally unsafe. But this was the only case where
exprInPlace is called not at the top level of an expression, so I
don't think the other uses can actually trigger an issue (there can't
be a sibling expression). TODO: maybe those cases don't need "in
place", and we can retire that function generally.

This CL needed a small tweak to the SSA generation of OIF so that the
short circuit optimization still triggers. The short circuit optimization
looks for triangle but not diamonds, so don't bother allocating a block
if it will be empty.

Go 1 benchmarks are in the noise.

Fixes #30567

Change-Id: I19c04296bea63cbd6ad05f87a63b005029123610
Reviewed-on: https://go-review.googlesource.com/c/go/+/165617
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 4a9064e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/165858
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants