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: unexpected string to byte casting behavior in 1.5 #12226

Closed
groob opened this issue Aug 20, 2015 · 26 comments
Closed

cmd/compile: unexpected string to byte casting behavior in 1.5 #12226

groob opened this issue Aug 20, 2015 · 26 comments

Comments

@groob
Copy link
Contributor

groob commented Aug 20, 2015

var lines = []string{"foo", "bar", "baz"}

func main() {
    for _, line := range lines {
        if len(line) == 0 ||
            []byte(line)[0] == []byte("#")[0] {
            continue
        }
        fmt.Println(line)
    }
}

This snippet of code prints the strings as you would expect when compiled with 1.4, but produces no output when compiled with 1.5.
http://play.golang.org/p/hR7iVDC-Vu

Here's a slightly simpler example of the same bug:

        s := "foo"
        fmt.Println(len(s)==0 || []byte(s)[0]==[]byte("#")[0])

This results in false with 1.4, but true with 1.5.
by taruti in #golang-nuts on irc

@tsuna
Copy link
Contributor

tsuna commented Aug 20, 2015

Interestingly, the problem can't be reproduced with []byte{...} syntax:

    s := "foo"
    fmt.Println(len(s) == 0 || []byte(s)[0] == []byte{'#'}[0])

This correctly prints false with 1.5. I wonder if this is related to the compiler bug I just filed in #12225 as that problem also only occurs with byte slices created from strings.

@johto
Copy link
Contributor

johto commented Aug 20, 2015

Also, the len() call seems to be irrelevant. This reproduces as well:

fmt.Println([]byte("0")[0] == []byte("1")[0] || false)

@ulikunitz
Copy link
Contributor

Following program reproduces the bug for me on AMD64:

package main

import "fmt"

func main() {
    if []byte("foo")[0] == []byte("b")[0] {
        fmt.Println("BUG: foo and b appear to have the same first byte")
    } else {
        fmt.Println("No bug: foo doesn't start with b")
    }
}

It seems to be an issue with the register allocator, the compiler produces CMP BL, BL for the equality operator, which is always true.

@adg
Copy link
Contributor

adg commented Aug 20, 2015

Smells like a compiler optimizer bug to me.

@adg adg changed the title Unexpected string to byte casting behavior in 1.5 cmd/compiler: unexpected string to byte casting behavior in 1.5 Aug 20, 2015
@adg
Copy link
Contributor

adg commented Aug 20, 2015

cc @rsc @josharian

@bradfitz bradfitz added this to the Go1.5.1 milestone Aug 20, 2015
@ulikunitz
Copy link
Contributor

Correction: Statement is CMPB BL,BL, which will always be equal.

go tool compile -N -S produces the same statement, so I suppose it is not the optimizer.

@e-dard
Copy link

e-dard commented Aug 20, 2015

Hi, not sure if this is relevant or not; apologies if it's noise. Did a bit of git bisect...

Test program:

package main

import (
        "os"
)

func main() {
        if []byte("0")[0] == []byte("1")[0] {
                os.Exit(1)
        }
}

The first bad commit I can see for this program is b115c35.

It causes a panic:

# command-line-arguments
naddr [0x208d1b440]
.   INDEX u(100) l(8) tc(1) byte
.   .   CALLFUNC u(100) l(8) tc(1) ARRAY-[]byte
.   .   .   NAME-runtime.stringtoslicebyte u(1) a(true) l(2) x(0+0) class(PFUNC) tc(1) used(true) FUNC-func(*[32]byte, string) []byte
.   .   CALLFUNC-list
.   .   .   AS u(2) l(8) tc(1)
.   .   .   .   INDREG-SP a(true) l(8) x(0+0) tc(1) PTR64-*[32]byte
.   .   .   .   ADDR u(2) l(8) tc(1) PTR64-*[32]uint8
.   .   .   .   .   NAME-main.autotmp_0000 u(1) a(true) l(8) x(0+0) class(PAUTO) esc(N) tc(1) addrtaken used(true) ARRAY-[32]uint8

.   .   .   AS u(1) l(8) tc(1)
.   .   .   .   INDREG-SP a(true) l(8) x(8+0) tc(1) string
.   .   .   .   LITERAL-"0" u(1) a(true) l(8) tc(1) string
.   .   LITERAL-0 u(1) a(true) l(8) tc(1) int
../../main.go:8: internal compiler error: naddr: bad INDEX
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x1cfdb0]

goroutine 1 [running]:
cmd/compile/internal/gc.hcrash()
    /tmp/go/src/cmd/compile/internal/gc/subr.go:105 +0x50
cmd/compile/internal/gc.Fatal(0x452f90, 0x10, 0x208adf360, 0x2, 0x2)
    /tmp/go/src/cmd/compile/internal/gc/subr.go:198 +0x2b1
cmd/compile/internal/gc.Naddr(0x208d32fd0, 0x208d1b440)
    /tmp/go/src/cmd/compile/internal/gc/gsubr.go:313 +0x1a5b
cmd/compile/internal/amd64.gins(0x3036, 0x208d1b440, 0x208d1b7a0, 0xfffb)
    /tmp/go/src/cmd/compile/internal/amd64/gsubr.go:629 +0x127
cmd/compile/internal/gc.bgenx(0x208d1b830, 0x0, 0x1, 0x0, 0x208c517a0)
    /tmp/go/src/cmd/compile/internal/gc/cgen.go:2061 +0x1954
cmd/compile/internal/gc.Bgen(0x208d1b830, 0x208c51800, 0x0, 0x208c517a0)
    /tmp/go/src/cmd/compile/internal/gc/cgen.go:1739 +0x47
cmd/compile/internal/gc.gen(0x208d1b8c0)
    /tmp/go/src/cmd/compile/internal/gc/gen.go:805 +0xcd4
cmd/compile/internal/gc.Genlist(0x208d01aa0)
    /tmp/go/src/cmd/compile/internal/gc/gen.go:219 +0x30
cmd/compile/internal/gc.compile(0x208d1b050)
    /tmp/go/src/cmd/compile/internal/gc/pgen.go:469 +0xc03
cmd/compile/internal/gc.funccompile(0x208d1b050)
    /tmp/go/src/cmd/compile/internal/gc/dcl.go:1480 +0x1c9
cmd/compile/internal/gc.Main()
    /tmp/go/src/cmd/compile/internal/gc/lex.go:470 +0x1ece
cmd/compile/internal/amd64.Main()
    /tmp/go/src/cmd/compile/internal/amd64/galign.go:127 +0x58d
main.main()
    /tmp/go/src/cmd/compile/main.go:26 +0x189

At a44bece the behaviour changes to no panic but an exit code of 1.

@ulikunitz
Copy link
Contributor

This diff fixes the bug for me:

diff --git a/src/cmd/compile/internal/gc/cgen.go b/src/cmd/compile/internal/gc/cgen.go
index 4160ae9..b6a3e5b 100644
--- a/src/cmd/compile/internal/gc/cgen.go
+++ b/src/cmd/compile/internal/gc/cgen.go
@@ -2018,11 +2018,11 @@ func bgenx(n, res *Node, wantTrue bool, likely int, to *obj.Prog) {
        Regalloc(&n2, nr.Type, nil)
        Cgen(nr, &n2)
        nr = &n2
-       Regfree(&n2)

        Regalloc(&n1, nl.Type, nil)
        Cgen(&tmp, &n1)
        Regfree(&n1)
+       Regfree(&n2)
    } else {
        var n1 Node
        if !nl.Addable && Ctxt.Arch.Thechar == '8' {

@josharian
Copy link
Contributor

Want to send a CL? That diff makes sense.

@ulikunitz
Copy link
Contributor

I have sent the CL https://go-review.googlesource.com/#/c/13771/

(It is my first.)

@dgryski
Copy link
Contributor

dgryski commented Aug 20, 2015

Should that test be an actual test in the CL instead of in the commit message?

@ulikunitz
Copy link
Contributor

You are right. But before I can do that, I have to understand where and how to add the test. I will work on it.

@dgryski
Copy link
Contributor

dgryski commented Aug 20, 2015

@gopherbot
Copy link

CL https://golang.org/cl/13771 mentions this issue.

@ulikunitz
Copy link
Contributor

Thanks for that. The CL includes now test/fixedbugs/issue12226.go.

@mikioh mikioh changed the title cmd/compiler: unexpected string to byte casting behavior in 1.5 cmd/compile: unexpected string to byte casting behavior in 1.5 Aug 21, 2015
@dgryski
Copy link
Contributor

dgryski commented Aug 21, 2015

Probably worth running @dvyukov's Gosmith 1.4.2 vs 1.5. Previously I think it was only run gc vs gccgo.

@myml
Copy link

myml commented Aug 22, 2015

package main

import "fmt"

func main() {
    if []byte("foo")[0] == []byte("b")[0] == true {
        fmt.Println([]byte("foo")[0] == []byte("b")[0])
        fmt.Println("BUG: foo and b appear to have the same first byte")
    } else {
        fmt.Println("No bug: foo doesn't start with b")
    }
}

No bug: foo doesn't start with b

@ulikunitz
Copy link
Contributor

@myml: The original test program is definitely buggy on 1.5. Your test is different and it probably doesn't follow the same path in the code generation for the equal operator.

@myml
Copy link

myml commented Aug 22, 2015

@uli-go

if []byte("foo")[0] == []byte("b")[0] {} 

and

if []byte("foo")[0] == []byte("b")[0] == true {}  

on 1.5 get different results;

@ulikunitz
Copy link
Contributor

@myml: We agree that your program is different.

@sleaze
Copy link

sleaze commented Aug 22, 2015

Why you censor my comments?

@bradfitz
Copy link
Contributor

Off-topic or uncivil comments adding no technical value to the discussion may be deleted. Please keep the discussions focused. If you want to discuss something unrelated, use the golang-nuts list, but keep it civil there too.

@mengzhuo
Copy link
Contributor

Confirm on AMD64
I think this is an urgent bug to fix, how about release 1.5.0.1?

@rsc
Copy link
Contributor

rsc commented Aug 25, 2015

Thanks very much for the fix, @uli-go.

@rsc rsc closed this as completed in 6ec1809 Aug 25, 2015
@gopherbot
Copy link

CL https://golang.org/cl/14227 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 8, 2015
…erator

The issue 12226 has been caused by the allocation of the same register
for the equality check of two byte values. The code in cgen.go freed the
register for the second operand before the allocation of the register
for the first operand.

Fixes #12226

Change-Id: Ie4dc33a488bd48a17f8ae9b497fd63c1ae390555
Reviewed-on: https://go-review.googlesource.com/13771
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/14227
Reviewed-by: Ian Lance Taylor <iant@golang.org>
petermattis added a commit to cockroachdb/cockroach that referenced this issue Sep 30, 2015
Needed to pick up the fix for golang/go#12226
which affects other equality comparisons than the one mentioned in that
issue.
@sleaze
Copy link

sleaze commented Feb 23, 2016

@bradfitz Roger that, thank you for explaining!

@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests