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: compiler does not see that string escapes via function literal #14409

Closed
szank opened this issue Feb 19, 2016 · 4 comments
Closed

Comments

@szank
Copy link

szank commented Feb 19, 2016

Hi, I am attaching a test case which fails on OSX El Capitan after upgrading go to version 1.6
The first call to the badFunc illustrates that the code works when the string returned by the badFunc is equal or longer than 32 bytes. For shorter strings it fails.

When I shuffle the code around, the returned bytes are bit different.
When I call the NewError([something shorter than 32 bytes]) it returns expected results.

package main

import (
    "reflect"
    "testing"
)

type ErrorType struct {
    Description string
}

func TestGoCompileError(t *testing.T) {
    var theError = badFunc("AAAAAAAAAAAAAAAAAAAAAA")
    if reflect.DeepEqual([]byte(theError.Description), []byte("AAAAAAAAAAAAAAAAAAAAAADDDDDDDDDDD")) == false { // 22 + 11 chars = 33 chars in the output
        t.Fatal("Test that shouldn't have failed have failed")
    }

    theError = badFunc("AAAAAAAAAAAAAAAAAAAAA")
    if reflect.DeepEqual([]byte(theError.Description), []byte("AAAAAAAAAAAAAAAAAAAAADDDDDDDDDDD")) == false { // 21 chars + 11 chars = 32 chars in the output, fails for char count <= 32
        t.Fatalf("Got %v bytes, expected %v", []byte(theError.Description), []byte("AAAAAAAAAAAAAAAAAAAAADDDDDDDDDDD"))
    }
}

func badFunc(input string) *ErrorType {
    var testSlice = []string{}
    testSlice = append(testSlice, input)
    var outputString = ""
    outputString = testSlice[0] + "DDDDDDDDDDD" // 11 chars

    return NewError(outputString)
}

func NewError(description ...string) *ErrorType {
    return &ErrorType{
        Description: func() string {
            if len(description) > 0 {
                return description[0]
            }

            return "Another error"
        }(),
    }
}

Results:

$ go test 
--- FAIL: TestGoCompileError (0.00s)
    main_test.go:20: Got [32 0 0 0 0 0 0 0 43 42 4 0 0 0 0 0 192 128 10 32 200 0 0 0 232 141 4 32 200 0 0 0] bytes, expected [65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 65 68 68 68 68 68 68 68 68 68 68 68]
FAIL
exit status 1
@ianlancetaylor ianlancetaylor changed the title Go 1.6 regression: Memory corruption when running a test code cmd/compile: compiler does not see that string escapes via function literal Feb 19, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.6.1 milestone Feb 19, 2016
@randall77
Copy link
Contributor

Looks like an escape analysis bug to me also.

Simpler repro:

func f(s string) string {
    t := s + "XXXX"
    return z(t)
}
func z(a ...string) string {
    return func() string { return a[0] }()
}

In f, the compiler decides to pass a temporary stack buffer to concatstring2. It shouldn't, as the resulting string is returned via a tortuous path through z.

@gopherbot
Copy link

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

@adg adg modified the milestones: Go1.6.1, Go1.6.2 Apr 7, 2016
@rsc rsc added the Release-OK label Apr 7, 2016
@gopherbot
Copy link

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

@adg
Copy link
Contributor

adg commented Apr 14, 2016

As noted on the cherry pick CL for 1.6.2, this doesn't build as it depends on 2d56dee, a much more substantial piece of work. For this to make it into 1.6.2 it needs a specially authored patch.

@adg adg reopened this Apr 14, 2016
gopherbot pushed a commit that referenced this issue Apr 19, 2016
Missed a case for closure calls (OCALLFUNC && indirect) in
esc.go:esccall.

Cleanup to runtime code for windows to more thoroughly hide
a technical escape.  Also made code pickier about failing
to late non-optional kernel32.dll.

Revised for 1.6.2

Fixes #14409.

Change-Id: Ie75486a2c8626c4583224e02e4872c2875f7bca5
Reviewed-on: https://go-review.googlesource.com/22050
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@adg adg closed this as completed Apr 19, 2016
@golang golang locked and limited conversation to collaborators Apr 19, 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

7 participants