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/gc: Slice access of const causes strange string errors (Bug introduced in go1.3, doesn't happen in go <= 1.2) #8325

Closed
gopherbot opened this issue Jul 3, 2014 · 11 comments
Closed

cmd/gc: Slice access of const causes strange string errors (Bug introduced in go1.3, doesn't happen in go <= 1.2) #8325

gopherbot opened this issue Jul 3, 2014 · 11 comments
Labels
FrozenDueToAge
Milestone
Go1.3.1

Comments

@gopherbot
Copy link

by psnim2000:

What does 'go version' print?
go version go1.3 darwin/amd64 (first discovered in go version go1.3 linux/amd64)

What steps reproduce the problem?
If possible, include a link to a program on play.golang.org.
http://play.golang.org/p/RwqaHkITff
 
What happened?
See attached program. 
If you run this program on go1.3, you will get something like.
Hello, playground PLCM7w9WX0F4Fs9Yz9.0699t A5

If you run this program on go1.2, you will get something like
Hello, playground PLCM2IE95QPT6ZLUAX218417FLX3449NEJ5D

What should have happened instead?
The code shouldn't return any characters after [A-Z0-9]
If you notice the output under go1.3, the value outside of [A-Z0-9] shouldn't be
possible. I'm unsure if the output value is even valid UTF-8.

Please provide any additional information below.
This error was uncovered after upgrading a service to go1.3. We tested the function with
go1 and go1.2, and the problem doesn't occur.
@ianlancetaylor
Copy link
Contributor

Comment 1:

This looks like a compiler bug.

Labels changed: added repo-main, release-go1.3.1.

@remyoudompheng
Copy link
Contributor

Comment 2:

The generated code is wrong for b % 36.
The theoretical equivalent expression is: b - 36*(b/36) where b/36 is represented by 
uint16(b) * 57 >> 11 (2048/57 = 35.9298...)
The generated code is:
    0x007f 00127 (z.go:7)   MOVBQZX (SI),BP    => BP = b
        0x0083 00131 (z.go:7)   MOVQ    BP,DI      => DI = b too.
    0x0086 00134 (z.go:8)   CMPQ    CX,R9
    0x0089 00137 (z.go:8)   JCC $1,325
    0x008f 00143 (z.go:8)   LEAQ    (R10)(CX*1),BX
    0x0093 00147 (z.go:8)   MOVQ    $57,R8
    0x009a 00154 (z.go:8)   MOVQ    BP,AX
    0x009d 00157 (z.go:8)   MULB    R8B,       => AH:AL = uint16(b)*57
    0x00a0 00160 (z.go:8)   MOVB    AH,DL      => b/36 is DL>>3
                                                   => we are missing a MOVB DL, BPB here
    0x00a2 00162 (z.go:8)   SHRB    $3,BPB     => BP>>3 is not what we want.
    0x00a6 00166 (z.go:8)   MOVQ    $36,R8
    0x00ad 00173 (z.go:8)   IMULQ   R8,BP
    0x00b1 00177 (z.go:8)   MOVQ    DI,R8
    0x00b4 00180 (z.go:8)   SUBQ    BP,R8      => R8 = DI - 36*BP
    0x00b7 00183 (z.go:8)   MOVBQZX R8B,BP
    0x00bb 00187 (z.go:8)   LEAQ    go.string."0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"+16(SB)(BP*1),R8
The wrong code results in wrong index (> 36) taken in the constant string.

Status changed to Started.

@remyoudompheng
Copy link
Contributor

Comment 3:

I could bisect the issue up to:
changeset:   19052:bcc8475a7ada
user:        Daniel Morsing <daniel.morsing@gmail.com>
date:        Tue Feb 11 20:25:40 2014 +0000
summary:     cmd/6g, cmd/8g, cmd/5g: make the undefined instruction have no successors
Not sure how it causes the bug.

@remyoudompheng
Copy link
Contributor

Comment 4:

https://golang.org/cl/104560044

@gopherbot
Copy link
Author

Comment 5:

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

@gopherbot
Copy link
Author

Comment 6:

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

@rsc
Copy link
Contributor

rsc commented Aug 11, 2014

Comment 7:

This issue was closed by revision 5b63ce4.

Status changed to Fixed.

@rsc
Copy link
Contributor

rsc commented Aug 11, 2014

Comment 8:

No workaround, small fix. Approved for Go 1.3.1.

@rsc
Copy link
Contributor

rsc commented Aug 11, 2014

Comment 9:

Also need CL 129720043 for 386 fix.

@adg
Copy link
Contributor

adg commented Aug 12, 2014

Comment 10:

This issue was closed by revision 8246129e698d.

@adg
Copy link
Contributor

adg commented Aug 12, 2014

Comment 11:

Applied to release-branch.go1.3.

@rsc rsc added this to the Go1.3.1 milestone Apr 14, 2015
adg added a commit that referenced this issue May 11, 2015
@adg
[release-branch.go1.3] cmd/6g, cmd/8g: fix, test byte-sized magic mul…
3fa4a78 
…tiply

««« CL 124950043 / 8e5ec6948793
cmd/6g, cmd/8g: fix, test byte-sized magic multiply

Credit to Rémy for finding and writing test case.

Fixes #8325.

LGTM=r
R=golang-codereviews, r
CC=dave, golang-codereviews, iant, remyoudompheng
https://golang.org/cl/124950043
»»»

TBR=rsc
CC=golang-codereviews
https://golang.org/cl/126000043
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@rsc @wheatman
cmd/6g, cmd/8g: fix, test byte-sized magic multiply
148f7a2 
Credit to Rémy for finding and writing test case.

Fixes golang#8325.

LGTM=r
R=golang-codereviews, r
CC=dave, golang-codereviews, iant, remyoudompheng
https://golang.org/cl/124950043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@rsc @wheatman
cmd/6g, cmd/8g: fix, test byte-sized magic multiply
6a71545 
Credit to Rémy for finding and writing test case.

Fixes golang#8325.

LGTM=r
R=golang-codereviews, r
CC=dave, golang-codereviews, iant, remyoudompheng
https://golang.org/cl/124950043
This issue was closed.
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
Go1.3.1
Development

No branches or pull requests
5 participants
@rsc @remyoudompheng @ianlancetaylor @adg @gopherbot