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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: -ssa=0 generates incorrect code for append(s,s) #17039

Closed
alandonovan opened this issue Sep 9, 2016 · 10 comments
Closed

cmd/compile: -ssa=0 generates incorrect code for append(s,s) #17039

alandonovan opened this issue Sep 9, 2016 · 10 comments
Labels
FrozenDueToAge

Comments

@alandonovan
Copy link
Contributor

alandonovan commented Sep 9, 2016 •
edited

gc 1.6.2 generates incorrect linux/amd64 code for the following program: the final print statement prints the wrong answer.

% cat ~/a.go
package main

type S []S

func main() {
        var s S
        println(s == nil) // "true"
        s = append(s, s) // append a nil value to s
        println(s[0] == nil) // "false" (!)
}

From the assembly, it appears that the register r8, which holds the old value of s.ptr and is used in the s[0]==nil check, gets set to the the new value of s.ptr during the growslice operation.

By default, the tip gc generates completely different code that does not exhibit the problem, but with -ssa=0 the same problem reoccurs (with r9). See comments in listing below.

"".main t=1 size=335 args=0x0 locals=0x68
...
    0x0021 00033 (a.go:7)   MOVQ    $0, AX
    0x0023 00035 (a.go:7)   MOVQ    AX, "".s+80(SP)
    0x0028 00040 (a.go:7)   MOVQ    AX, "".s+88(SP)
    0x002d 00045 (a.go:8)   MOVQ    AX, "".s+72(SP)
    0x0032 00050 (a.go:8)   CMPQ    AX, $0
    0x0036 00054 (a.go:8)   SETEQ   "".autotmp_0+71(SP)
    0x003b 00059 (a.go:8)   PCDATA  $0, $1
    0x003b 00059 (a.go:8)   CALL    runtime.printlock(SB)
    0x0040 00064 (a.go:8)   MOVBQZX "".autotmp_0+71(SP), BX
    0x0045 00069 (a.go:8)   MOVB    BL, (SP)
    0x0048 00072 (a.go:8)   PCDATA  $0, $1
    0x0048 00072 (a.go:8)   CALL    runtime.printbool(SB)
    0x004d 00077 (a.go:8)   PCDATA  $0, $1
    0x004d 00077 (a.go:8)   CALL    runtime.printnl(SB)
    0x0052 00082 (a.go:8)   PCDATA  $0, $1
    0x0052 00082 (a.go:8)   CALL    runtime.printunlock(SB)
    0x0057 00087 (a.go:8)   MOVQ    "".s+72(SP), CX
    0x005c 00092 (a.go:8)   MOVQ    "".s+88(SP), DI
    0x0061 00097 (a.go:9)   MOVQ    "".s+80(SP), BX
    0x0066 00102 (a.go:9)   MOVQ    CX, R9   ;;; load old s.ptr into R9
    0x0069 00105 (a.go:9)   MOVQ    BX, AX
    0x006c 00108 (a.go:9)   INCQ    BX
    0x006f 00111 (a.go:9)   CMPQ    BX, DI
    0x0072 00114 (a.go:9)   JHI $1, 260   ;;; uh oh, time to grow the slice
    0x0078 00120 (a.go:9)   MOVQ    BX, SI
    0x007b 00123 (a.go:9)   MOVQ    CX, BX
    0x007e 00126 (a.go:9)   MOVQ    AX, R8
    0x0081 00129 (a.go:9)   IMULQ   $24, R8
    0x0085 00133 (a.go:9)   ADDQ    R8, BX
    0x0088 00136 (a.go:9)   MOVQ    SI, "".s+80(SP)
    0x008d 00141 (a.go:9)   MOVQ    SI, 8(BX)
    0x0091 00145 (a.go:9)   MOVQ    DI, "".s+88(SP)
    0x0096 00150 (a.go:9)   MOVQ    DI, 16(BX)
    0x009a 00154 (a.go:9)   MOVQ    R9, "".s+72(SP)
    0x009f 00159 (a.go:9)   CMPB    runtime.writeBarrier(SB), $0
    0x00a6 00166 (a.go:9)   JNE $0, 234
    0x00a8 00168 (a.go:9)   MOVQ    R9, (BX)    ;;; check s[0]==nil using new not old s.ptr
    0x00ab 00171 (a.go:10)  CMPQ    SI, $0
    0x00af 00175 (a.go:10)  JLS $1, 227
    0x00b1 00177 (a.go:10)  MOVQ    (R9), R8
    0x00b4 00180 (a.go:10)  CMPQ    R8, $0
    0x00b8 00184 (a.go:10)  SETEQ   "".autotmp_1+71(SP)
    0x00bd 00189 (a.go:10)  PCDATA  $0, $0
    0x00bd 00189 (a.go:10)  CALL    runtime.printlock(SB)
    0x00c2 00194 (a.go:10)  MOVBQZX "".autotmp_1+71(SP), BX
    0x00c7 00199 (a.go:10)  MOVB    BL, (SP)
    0x00ca 00202 (a.go:10)  PCDATA  $0, $0
    0x00ca 00202 (a.go:10)  CALL    runtime.printbool(SB)
        ...
    0x00e2 00226 (a.go:12)  RET
    ...        
    0x0104 00260 (a.go:9)   LEAQ    type."".S(SB), R8
    0x010b 00267 (a.go:9)   MOVQ    R8, (SP)
    0x010f 00271 (a.go:9)   MOVQ    CX, 8(SP)
    0x0114 00276 (a.go:9)   MOVQ    AX, 16(SP)
    0x0119 00281 (a.go:9)   MOVQ    DI, 24(SP)
    0x011e 00286 (a.go:9)   MOVQ    BX, 32(SP)
    0x0123 00291 (a.go:9)   PCDATA  $0, $0
    0x0123 00291 (a.go:9)   CALL    runtime.growslice(SB)
    0x0128 00296 (a.go:9)   MOVQ    40(SP), R9   ;;; update R9 with new, larger s.ptr
    0x012d 00301 (a.go:9)   MOVQ    48(SP), SI
    0x0132 00306 (a.go:9)   MOVQ    56(SP), DI
    0x0137 00311 (a.go:9)   MOVQ    SI, AX
    0x013a 00314 (a.go:9)   INCQ    SI
    0x013d 00317 (a.go:9)   MOVQ    R9, CX
    0x0140 00320 (a.go:9)   JMP 123
@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

If we delete the old backend, this problem is solved.

Considering that SSA is the default for amd64 in Go 1.7 and we don't do point releases for fixes two versions back (for Go 1.6), there's nothing to do here unless we also generate bad code for non-amd64 architectures.

Want to add a test/fixedbugs/nnnnn.go file with this case and run it against the trybots and see? You'll need to make your git commit's parent be before tip's SSA starting being enabled by default for various architectures. I'd add the test on the release-go1.7 branch and trybot that.

@alandonovan
Copy link
Contributor Author

Agreed that deleting the old backend is a solution and that go1.6 is a dead letter; my concern was about current non-x86.

I'll add the suggested test.

@alandonovan
Copy link
Contributor Author

At 1.7, the test fails on a bunch of platforms: https://go-review.googlesource.com/c/28873/

@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

Well, fails on 386, arm, and amd64p32. So probably all. ppc64 and such aren't tested by the trybots.

@bradfitz bradfitz added this to the Go1.7.2 milestone Sep 9, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

Flagging this potentially for 1.7.2. Not sure whether it bites people in practice.

/cc @griesemer @mdempsky @josharian

@bradfitz bradfitz changed the title gc: -ssa=0 generates incorrect code for append(s,s) cmd/compile: -ssa=0 generates incorrect code for append(s,s) Sep 9, 2016
@randall77
Copy link
Contributor

This doesn't seem like something that would occur in practice. Can you actually do anything with such a recursive type? My brain hurts trying to type check it.
Would be fine to put a fix in 1.7.2 but I don't think it should trigger a 1.7.2.

@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

Would be fine to put a fix in 1.7.2 but I don't think it should trigger a 1.7.2.

Agree.

@alandonovan
Copy link
Contributor Author

I agree this example is just a toy, but if the cause is a bug in the register allocator, the problem might be more general.

@randall77
Copy link
Contributor

I think the bug is triggered when the slice you're appending to and the thing you're appending are in the same register. That doesn't ever happen in practice - they always have different types.

@josharian
Copy link
Contributor

The problem only occurs with this peculiar, rare form of statement, so it won't get fixed in a point release, and the old backend is now gone. Closing.

@broady broady removed this from the Go1.7.2 milestone Oct 17, 2016
@golang golang locked and limited conversation to collaborators Oct 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@bradfitz @broady @josharian @randall77 @alandonovan @gopherbot