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: incorrect evaluation of conditional in if statement #12266

Closed
griesemer opened this issue Aug 21, 2015 · 11 comments
Closed

cmd/compile: incorrect evaluation of conditional in if statement #12266

griesemer opened this issue Aug 21, 2015 · 11 comments
Labels
FrozenDueToAge

Comments

@griesemer
Copy link
Contributor

https://play.golang.org/p/rEXm1QaLgo prints

false
true

instead of

false
false

This happens both in the playground (currently 1.4) and at tip (1.5) (go version devel +4ec018e Fri Aug 21 13:08:00 2015 -0700 darwin/amd64).

I suspect the condition code register gets clobbered or perhaps is not properly saved/restored across function calls.

Thanks to Daniel Martí (@mvdan) for reporting this.
@griesemer griesemer added this to the Go1.6Early milestone Aug 21, 2015
@mvdan mvdan mentioned this issue Aug 21, 2015
Merged
@griesemer
Copy link
Contributor Author

Here's the assembly code for g1 and g2:

"".g1 t=1 size=64 value=0 args=0x8 locals=0x0
    0x0000 00000 (b.go:11)  TEXT    "".g1(SB), $0-8
    0x0000 00000 (b.go:11)  NOP
    0x0000 00000 (b.go:11)  NOP
    0x0000 00000 (b.go:11)  FUNCDATA    $0, gclocals·5184031d3a32a42d85027f073f873668(SB)
    0x0000 00000 (b.go:11)  FUNCDATA    $1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
    0x0000 00000 (b.go:12)  MOVQ    $0, AX
    0x0002 00002 (b.go:12)  MOVQ    $0, CX
    0x0004 00004 (b.go:12)  CMPB    AL, $0
    0x0006 00006 (b.go:12)  JNE 42
    0x0008 00008 (b.go:12)  MOVQ    $1, AX
    0x000f 00015 (b.go:12)  CMPB    CL, $0
    0x0012 00018 (b.go:12)  JNE 33
    0x0014 00020 (b.go:12)  MOVQ    $0, CX
    0x0016 00022 (b.go:12)  MOVQ    CX, BX
    0x0019 00025 (b.go:12)  CMPB    BL, AL
    0x001b 00027 (b.go:12)  SETEQ   "".~r0+8(FP)
    0x0020 00032 (b.go:12)  RET
    0x0021 00033 (b.go:12)  MOVQ    $1, BX
    0x0028 00040 (b.go:12)  JMP 25
    0x002a 00042 (b.go:12)  MOVQ    $1, AX
    0x0031 00049 (b.go:12)  JMP 15
    0x0000 31 c0 31 c9 3c 00 75 22 48 c7 c0 01 00 00 00 80  1.1.<.u"H.......
    0x0010 f9 00 75 0d 31 c9 48 89 cb 38 c3 0f 94 44 24 08  ..u.1.H..8...D$.
    0x0020 c3 48 c7 c3 01 00 00 00 eb ef 48 c7 c0 01 00 00  .H........H.....
    0x0030 00 eb dc                                         ...
"".g2 t=1 size=64 value=0 args=0x8 locals=0x0
    0x0000 00000 (b.go:15)  TEXT    "".g2(SB), $0-8
    0x0000 00000 (b.go:15)  NOP
    0x0000 00000 (b.go:15)  NOP
    0x0000 00000 (b.go:15)  FUNCDATA    $0, gclocals·5184031d3a32a42d85027f073f873668(SB)
    0x0000 00000 (b.go:15)  FUNCDATA    $1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
    0x0000 00000 (b.go:16)  MOVQ    $0, AX
    0x0002 00002 (b.go:16)  MOVQ    $0, DX
    0x0004 00004 (b.go:16)  CMPB    AL, $0
    0x0006 00006 (b.go:16)  JNE 53
    0x0008 00008 (b.go:16)  MOVQ    $1, AX
    0x000f 00015 (b.go:16)  MOVQ    AX, BX
    0x0012 00018 (b.go:16)  MOVQ    BX, CX
    0x0015 00021 (b.go:16)  CMPB    DL, $0
    0x0018 00024 (b.go:16)  JNE 44
    0x001a 00026 (b.go:16)  MOVQ    $0, AX
    0x001c 00028 (b.go:16)  CMPB    CL, CL
    0x001e 00030 (b.go:16)  JNE 38
    0x0020 00032 (b.go:17)  MOVB    $1, "".~r0+8(FP)
    0x0025 00037 (b.go:17)  RET
    0x0026 00038 (b.go:19)  MOVB    $0, "".~r0+8(FP)
    0x002b 00043 (b.go:19)  RET
    0x002c 00044 (b.go:16)  MOVQ    $1, BX
    0x0033 00051 (b.go:16)  JMP 28
    0x0035 00053 (b.go:16)  MOVQ    $1, BX
    0x003c 00060 (b.go:16)  JMP 18
    0x0000 31 c0 31 d2 3c 00 75 2d 48 c7 c0 01 00 00 00 48  1.1.<.u-H......H
    0x0010 89 c3 48 89 d9 80 fa 00 75 12 31 c0 38 c9 75 06  ..H.....u.1.8.u.
    0x0020 c6 44 24 08 01 c3 c6 44 24 08 00 c3 48 c7 c3 01  .D$....D$...H...
    0x0030 00 00 00 eb e7 48 c7 c3 01 00 00 00 eb d4        .....H........

As an aside, it appears that the function calls g1 and g2 are inlined (as one would hope for) but there's no constant folding after inlining (otherwise the bug would disappear). Presumably this will change with the upcoming SSA-based backend.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.5.1, Go1.6Early Aug 22, 2015
@ianlancetaylor
Copy link
Contributor

This is most likely the same as #12226 .

@davecheney
Copy link
Contributor

Does it get it wrong on all arches, or just Intel?

On Sat, 22 Aug 2015 11:11 Ian Lance Taylor notifications@github.com wrote:

This is most likely the same as #12226
#12226 .

—
Reply to this email directly or view it on GitHub
#12266 (comment).

@mvdan
Copy link
Member

mvdan commented Aug 22, 2015

Didn't think it could be #12226 because of its title. Actually reading the comments on the issue, it does look like it's the same one.

@mvdan
Copy link
Member

mvdan commented Aug 22, 2015

Slight difference though - that issue appears to not be present in 1.4.2, while ours does happen in play.golang.org which @griesemer says is running that version.

@davecheney
Copy link
Contributor

Thanks playground was updated to 1.5 overnight, perhaps there was a
confusion over which version was used to execute the test program.

On Sat, 22 Aug 2015 11:18 Daniel Martí notifications@github.com wrote:

Slight difference though - that issue appears to not be present in 1.4.2,
while ours does happen in play.golang.org which @griesemer
https://github.com/griesemer says is running that version.

—
Reply to this email directly or view it on GitHub
#12266 (comment).

@mvdan
Copy link
Member

mvdan commented Aug 22, 2015

Oh, in that case I agree that it's most probably a duplicate.

@ulikunitz
Copy link
Contributor

With https://golang.org/cl/13771 (fix for #12226) for 1.5 the output is correct:

 false
 false

Looking on the assembler the issue is again CMPB CL, CL, the comparison of the same register, which is always equal, so the conditional jump JNE will never be taken.

go 1.4.2 produces the correct output and doesn't have the issue.

The issue is a duplicate of #12226.

The playground should display the version as requested in #12218.

@ulikunitz
Copy link
Contributor

Dave Cheney asked about architectures affected. The bug is in the architecture-independent code generation part of cmd/compile. So multiple architectures may be affected, I could verify the bug and the fix for 386 and AMD64.

@ulikunitz
Copy link
Contributor

Based on the assembler output of GOARCH=arm go tool compile -S I can confirm that the bug affects ARM as well and is fixed by CL 13771.

@rsc
Copy link
Contributor

rsc commented Aug 25, 2015

Dup of #12226, fixed by CL 13771.

@rsc rsc closed this as completed Aug 25, 2015
@adg adg removed this from the Go1.5.1 milestone Sep 8, 2015
@golang golang locked and limited conversation to collaborators Sep 8, 2016
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
8 participants
@davecheney @rsc @ianlancetaylor @mvdan @adg @ulikunitz @griesemer @gopherbot