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: different behaviors between short and normal variable declarations #36449

Closed
go101 opened this issue Jan 8, 2020 · 12 comments
Closed
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@go101
Copy link

go101 commented Jan 8, 2020

This issue is filed per @mdempsky's suggestion.

What version of Go are you using (go version)?

$ go version
go version go1.14beta1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import "fmt"

func f(p *int) int {
	*p = 123
	return *p
}

func bar() {
	var x int
	y, z := x, f(&x)
	fmt.Println(y, z) // 123 123
}

func baz() {
	var x int
	var y, z = x, f(&x)
	fmt.Println(y, z) // 0 123
}

func main() {
	bar()
	baz()
}

What did you expect to see?

Same outputs for the two functions.

What did you see instead?

Different outputs.

This is an inconsistency in gc, though it looks this doesn't violate Go spec, as some expression evaluation orders in Go are unspecified. But maybe it would be great to make the outputs in this examples consistent.

[edit]: this inconsistency has been removed since Go toolchain 1.20. But another found one still exists.

@go101
Copy link
Author

go101 commented Jan 8, 2020

It looks the output of the short declaration case behaves the same as pure assignments:

func foo() {
	var x int
	var y, z int
	y, z = x, f(&x)
	fmt.Println(y, z) // 123 123
}

@toothrot toothrot added this to the Backlog milestone Jan 8, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 8, 2020
@toothrot
Copy link
Contributor

toothrot commented Jan 8, 2020

@mdempsky
Copy link
Member

mdempsky commented Jan 8, 2020

It looks like this is because dcl.go:variter emits N:N variable declaration+initialization statements as var v1 t; v1 = e1; var v2 t; v2 = e2; .... This is safe because newly declared variables don't come into scope until after the statement, and short variable declarations must always declare new variables, so we can always initialize them in order.

On the other hand, for v1, v2 := e1, e2 we only require at least one variable to be newly declared. If v1 or v2 is already within scope, then it might appear in the RHS, and we need to be more careful.

As @go101 points out, the discrepancy is still conforming to the Go spec, so it's not something we have to fix.

I suspect the easiest fix (assuming we think it's worth fixing) would be changing variter to emit a single OAS2 node instead of multiple OAS nodes. I'm a little nervous if this has any unintended consequences via n.Name.Defn, but I don't immediately spot anything.

Another question is if it has any effect on compiler performance or on compiled program performance. I haven't looked into these at all yet.

@cuonglm
Copy link
Member

cuonglm commented Jan 8, 2020

Another question is if it has any effect on compiler performance or on compiled program performance. I haven't looked into these at all yet.

I tried with a simple patch:

diff --git a/src/cmd/compile/internal/gc/dcl.go b/src/cmd/compile/internal/gc/dcl.go
index 54c6a24df5..8402d818d8 100644
--- a/src/cmd/compile/internal/gc/dcl.go
+++ b/src/cmd/compile/internal/gc/dcl.go
@@ -162,6 +162,22 @@ func variter(vl []*Node, t *Node, el []*Node) []*Node {
                return append(init, as2)
        }

+       if len(vl) > 1 && len(el) == len(vl) && Curfn != nil {
+               as2 := nod(OAS2, nil, nil)
+               as2.List.Set(vl)
+               as2.Rlist.Set(el)
+               for _, v := range vl {
+                       v.Op = ONAME
+                       declare(v, dclcontext)
+                       v.Name.Param.Ntype = t
+                       v.Name.Defn = as2
+                       init = append(init, nod(ODCL, v, nil))
+               }
+               as2.SetColas(true)
+
+               return append(init, as2)
+       }

With this, go tool dist test passes, compilecmp result:

$ compilecmp
03:27:22 compilecmp   master   HEAD
03:27:22 master: cmd/go: adjust heuristics for skipping +incompatible versions
03:27:22 HEAD: xxx
03:27:22 cp <master> /Users/cuonglm/.compilecmp/817afe83578d869b36e8697344bb2d557c86b264
03:27:24 /Users/cuonglm/.compilecmp/817afe83578d869b36e8697344bb2d557c86b264/src/make.bash
03:28:15 cp <HEAD> /Users/cuonglm/.compilecmp/49890043fd25044d53b4167fb4214b7382b855f7
03:28:17 /Users/cuonglm/.compilecmp/49890043fd25044d53b4167fb4214b7382b855f7/src/make.bash
03:29:08 before: /Users/cuonglm/.compilecmp/817afe83578d869b36e8697344bb2d557c86b264
03:29:08 after: /Users/cuonglm/.compilecmp/49890043fd25044d53b4167fb4214b7382b855f7

benchstat -geomean  /var/folders/y4/hs76ltbn7sb66lw_6934kq4m0000gn/T/737143426 /var/folders/y4/hs76ltbn7sb66lw_6934kq4m0000gn/T/497012217
completed   15 of   15, estimated time remaining 0s (eta 3:39AM)
name          old time/op       new time/op       delta
Template            200ms ±13%        191ms ±14%   -4.18%  (p=0.033 n=15+14)
Unicode            85.2ms ±18%       76.7ms ± 8%   -9.94%  (p=0.000 n=15+13)
GoTypes             662ms ±13%        655ms ± 7%     ~     (p=0.652 n=14+15)
Compiler            3.21s ±11%        3.20s ±16%     ~     (p=0.653 n=15+15)
SSA                 10.1s ± 9%        10.2s ± 8%     ~     (p=0.603 n=14+14)
Flate               122ms ±10%        128ms ±23%     ~     (p=0.747 n=14+15)
GoParser            149ms ± 4%        155ms ±14%     ~     (p=0.052 n=13+15)
Reflect             393ms ± 4%        444ms ±23%  +12.77%  (p=0.013 n=13+15)
Tar                 165ms ± 4%        169ms ± 6%   +2.17%  (p=0.029 n=15+14)
XML                 221ms ± 3%        226ms ± 4%   +2.41%  (p=0.033 n=12+12)
LinkCompiler        1.11s ±25%        1.11s ±27%     ~     (p=0.713 n=15+15)
[Geo mean]          435ms             439ms        +1.01%

name          old user-time/op  new user-time/op  delta
Template            264ms ±12%        253ms ± 4%     ~     (p=0.052 n=15+13)
Unicode             136ms ±12%        126ms ± 5%   -7.44%  (p=0.000 n=15+12)
GoTypes             909ms ± 5%        907ms ± 7%     ~     (p=0.786 n=13+15)
Compiler            4.60s ± 9%        4.57s ± 8%     ~     (p=0.813 n=15+14)
SSA                 14.5s ± 4%        14.8s ± 6%     ~     (p=0.185 n=13+15)
Flate               158ms ±11%        166ms ±22%     ~     (p=0.847 n=14+15)
GoParser            190ms ± 4%        194ms ±13%     ~     (p=0.519 n=13+14)
Reflect             548ms ± 4%        610ms ±19%  +11.36%  (p=0.013 n=13+15)
Tar                 214ms ± 3%        214ms ± 4%     ~     (p=0.905 n=14+13)
XML                 290ms ± 3%        295ms ± 6%     ~     (p=0.143 n=12+12)
LinkCompiler        1.18s ±13%        1.17s ±10%     ~     (p=0.653 n=15+15)
[Geo mean]          583ms             587ms        +0.68%

name          old alloc/op      new alloc/op      delta
Template           36.5MB ± 0%       36.5MB ± 0%     ~     (p=0.838 n=15+15)
Unicode            28.4MB ± 0%       28.4MB ± 0%     ~     (p=0.252 n=14+15)
GoTypes             123MB ± 0%        123MB ± 0%     ~     (p=0.174 n=15+15)
Compiler            571MB ± 0%        571MB ± 0%   +0.04%  (p=0.000 n=15+15)
SSA                1.96GB ± 0%       1.96GB ± 0%     ~     (p=0.653 n=15+15)
Flate              22.9MB ± 0%       22.9MB ± 0%     ~     (p=0.363 n=13+15)
GoParser           28.2MB ± 0%       28.2MB ± 0%     ~     (p=0.567 n=15+15)
Reflect            78.9MB ± 0%       78.9MB ± 0%     ~     (p=0.683 n=15+15)
Tar                34.3MB ± 0%       34.3MB ± 0%     ~     (p=0.653 n=15+15)
XML                44.4MB ± 0%       44.4MB ± 0%     ~     (p=0.910 n=14+14)
LinkCompiler        263MB ± 0%        263MB ± 0%     ~     (p=0.148 n=15+15)
[Geo mean]         89.3MB            89.3MB        +0.00%

name          old allocs/op     new allocs/op     delta
Template             372k ± 0%         372k ± 0%     ~     (p=0.814 n=15+15)
Unicode              333k ± 0%         333k ± 0%     ~     (p=0.598 n=14+15)
GoTypes             1.33M ± 0%        1.33M ± 0%     ~     (p=0.281 n=15+15)
Compiler            5.53M ± 0%        5.53M ± 0%   +0.04%  (p=0.000 n=15+14)
SSA                 18.3M ± 0%        18.3M ± 0%     ~     (p=0.713 n=15+15)
Flate                234k ± 0%         234k ± 0%     ~     (p=0.065 n=15+14)
GoParser             305k ± 0%         305k ± 0%     ~     (p=0.060 n=15+15)
Reflect              980k ± 0%         980k ± 0%     ~     (p=0.602 n=15+15)
Tar                  345k ± 0%         345k ± 0%   -0.01%  (p=0.005 n=15+12)
XML                  425k ± 0%         425k ± 0%     ~     (p=0.991 n=14+15)
LinkCompiler         597k ± 0%         597k ± 0%     ~     (p=0.798 n=15+15)
[Geo mean]           813k              813k        +0.00%

name          old text-bytes    new text-bytes    delta
HelloSize           831kB ± 0%        831kB ± 0%     ~     (all equal)

name          old data-bytes    new data-bytes    delta
HelloSize           192kB ± 0%        192kB ± 0%     ~     (all equal)

name          old exe-bytes     new exe-bytes     delta
HelloSize          1.20MB ± 0%       1.20MB ± 0%     ~     (all equal)



file    before    after     Δ       %
compile 24493832  24502024  +8192   +0.033%
total   135247388 135255580 +8192   +0.006%

@go101
Copy link
Author

go101 commented Jan 9, 2020

Is it ok for compilers to automatically convert all local normal variable declarations to short variable declarations?

@cuonglm
Copy link
Member

cuonglm commented Jan 9, 2020

Is it ok for compilers to automatically convert all local normal variable declarations to short variable declarations?

That's what my patch above does:

if len(vl) > 1 && len(el) == len(vl) && Curfn != nil {

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@go101
Copy link
Author

go101 commented Dec 5, 2022

It looks the results become consistent with tip.

@go101
Copy link
Author

go101 commented Dec 5, 2022

Another similar case is found:

package main

func p() {
  f := func(x int) { print(x) }
  g := func() int { f = nil; return 1 }
  defer f(g())
}

func q() {
  f := func(x int) { print(x) }
  g := func() int { f = nil; return 1 }
  f(g())
}

func main() {
  p()
  q()
}

[edit]: an modified example which includes go statement:

package main

func h() {
  c := make(chan int)
  f := func(x int) { print(x); <-c }
  g := func() int { f = func(int) { print(9); <-c }; return 1 }
  go f(g())
  c <- 1
}

func p() {
  f := func(x int) { print(x) }
  g := func() int { f = func(int) { print(9) }; return 1 }
  defer f(g())
}

func q() {
  f := func(x int) { print(x) }
  g := func() int { f = func(int) { print(9) }; return 1 }
  f(g())
}

func main() {
  h() // 1
  p() // 1
  q() // 9
}

@mdempsky
Copy link
Member

mdempsky commented Dec 5, 2022

@go101 Sorry, how does that last test case demonstrate a difference in semantics between normal and short variable declarations?

@go101
Copy link
Author

go101 commented Dec 5, 2022

It is not variable declaration related. It is related to this issue in the expression evaluation order consistency view.
Need to open a new issue for it?

@go101

This comment was marked as outdated.

@go101
Copy link
Author

go101 commented Mar 15, 2024

This issue can be closed now. Because the cases mentioned in #36449 (comment) now print consistent outputs now.

@go101 go101 closed this as completed Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

5 participants