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

gccgo: incorrect order of evaluation #26495

Closed
ianlancetaylor opened this issue Jul 20, 2018 · 3 comments
Closed

gccgo: incorrect order of evaluation #26495

ianlancetaylor opened this issue Jul 20, 2018 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

This program should print 0. When built with current gccgo, it prints 1.

The immediate problem is that the convert_shortcuts pass moves the function calls to FV1 and FV2 out of line so that they are executed before the call to F.

CC @cherrymui @thanm

package main

type S struct{ a [16]string }

var V1, V2 S

func FV1() S { return V1 }

func FV2() S { return V2 }

func F() int {
	V1 = S{[16]string{"hi"}}
	return 0
}

func G(f bool) int {
	if f {
		return 1
	}
	return 0
}

func H() int {
	return F() + G(FV1() == FV2())
}

func main() {
	println(H())
}
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 20, 2018
@ianlancetaylor ianlancetaylor added this to the Gccgo milestone Jul 20, 2018
@go101
Copy link

go101 commented Jul 20, 2018

related: #24448

@gopherbot
Copy link

Change https://golang.org/cl/125299 mentions this issue: compiler: do order_evaluations before remove_shortcuts

@gopherbot
Copy link

Change https://golang.org/cl/125301 mentions this issue: test: add test for gccgo bug #26495

hubot pushed a commit to gcc-mirror/gcc that referenced this issue Jul 20, 2018
    
    In remove_shortcuts, the shortcut expressions (&&, ||) are
    rewritten to if statements, which are lifted out before the
    statement containing the shortcut expression. If the containing
    statement has other (sub)expressions that should be evaluated
    before the shortcut expression, which has not been lifted out,
    this will result in wrong evaluation order.
    
    For example, F() + G(A() && B()), the evaluation order per spec
    is F, A, B (if A returns true), G. If we lift A() and B() out
    first, they will be called before F, which is wrong.
    
    To fix this, we split order_evaluations to two phases. The first
    phase, which runs before remove_shortcuts, skips shortcut
    expressions' components. So it won't lift out subexpressions that
    are evaluated conditionally. The shortcut expression itself is
    ordered, since it may have side effects. Then we run
    remove_shortcuts. At this point the subexpressions that should be
    evaluated before the shortcut expression are already lifted out.
    remove_shortcuts also runs the second phase of order_evaluations
    to order the components of shortcut expressions, which were
    skipped during the first phase.
    
    Reorder the code blocks of remove_shortcuts and order_evaluations,
    since remove_shortcuts now calls Order_eval.
    
    Fixes golang/go#26495.
    
    Reviewed-on: https://go-review.googlesource.com/125299


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@262908 138bc75d-0d04-0410-961f-82ee72b054a4
gopherbot pushed a commit that referenced this issue Jul 20, 2018
Gccgo produced incorrect order of evaluation for expressions
involving &&, || subexpressions. The fix is CL 125299.

Updates #26495.

Change-Id: I18d873281709f3160b3e09f0b2e46f5c120e1cab
Reviewed-on: https://go-review.googlesource.com/125301
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Jul 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants