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

Jump to bottom

cmd/compile: go build -race fails with label .inlret7 already defined #17449

Closed
adamfaulkner opened this issue Oct 15, 2016 · 7 comments
Closed

cmd/compile: go build -race fails with label .inlret7 already defined #17449

adamfaulkner opened this issue Oct 15, 2016 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@adamfaulkner
Copy link

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/mnt/repos/server/go:/home/adamf/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build433458907=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

cat /tmp/master/compiler_bug.go

package master

type PriorityList struct {
    elems []interface{}
}

// Implement sort.Sort interface to sort a PriorityList by ascending Priority.
func (x *PriorityList) Len() int { return len(x.elems) }

func (l *PriorityList) remove(i int) interface{} {
    elem := l.elems[i]
    l.elems = append(l.elems[:i], l.elems[i+1:]...)
    return elem
}

// Removes the next element from the list, in order of priority (e.g. higher priority first).
// Returns nil if list is empty.
func (l *PriorityList) Next() interface{} {
    // The last element from the list has the highest priority.
    return l.remove(l.Len() - 1)
}

var l *PriorityList

func Foo() {
    for elem := l.Next(); elem != nil; elem = l.Next() {
    }
}

go build -race /tmp/master/compiler_bug.go

/tmp/master/compiler_bug.go:26: label .inlret7 already defined at /tmp/master/compiler_bug.go:26

What did you expect to see?

I expected it to successfully build this program.

What did you see instead?

/tmp/master/compiler_bug.go:26: label .inlret7 already defined at /tmp/master/compiler_bug.go:26

Restructuring the code in various ways makes this go away. For instance, rewriting the for loop as

elem := l.Next()
for elem != nil {
    elem = l.Next()
}

will make the compiler error go away. Also, disabling the race detector or trying with go 1.6 will make this go away. Similarly, "inlining" the Len() call will make this go away.
@mikioh mikioh changed the title go build -race fails with label .inlret7 already defined cmd/compile: go build -race fails with label .inlret7 already defined Oct 16, 2016
@JayNakrani
Copy link
Contributor

JayNakrani commented Oct 16, 2016 •
edited

Looks like race instrumentation duplicates the labels created for function inlining.

Confirmed with following. Assuming pwd is /src/, compile with:

  1. Inlining enabled and race detection disabled: No issues, compiles just fine.

    $ ./../pkg/tool/linux_amd64/compile ~/tmp/race/race.go

  2. Inlining and race detection both enabled: Fails to compile. Notice ".i4" is repeated in dumped code.

    $ ./../pkg/tool/linux_amd64/compile -race ~/tmp/race/race.go

  3. Inlining disabled and race detection enabled: Compiles just fine.

    $ ./../pkg/tool/linux_amd64/compile -race -l ~/tmp/race/race.go

I also dumped the function before and after race instrumentation. Side by side diff of generated codes is at https://www.diffchecker.com/68vuYuaq. Notice that label (and corresponding GOTO statement) .i4 is repeated in right side. (In master, inline-labels are of format .iX, where X is some number.)

Looking into how to prevent this.

JayNakrani added a commit to JayNakrani/go that referenced this issue Oct 17, 2016
@JayNakrani
Potential solution to Issue golang#17449.
7b1b24d 
Change-Id: I1fb8759057f6ef1c97b55330a1bc833e20957cc5
@JayNakrani
Copy link
Contributor

JayNakrani commented Oct 17, 2016 •
edited

Okay, so code duplicated by race instrumentation is more than just inline-label.

Here is a human readable form of dumped code after race instrumentation. Notice that code between ORIGINAL CODE BEGINS and ORIGINAL CODE ENDS is repeated inside AS-init block (marked with REPEATED CODE BEGINS/ENDS).

This happens because in instrumentnode(), it copies parent's generated nodes to outn and then appends outn to child's Ninit. This copies over all the parent block's generated nodes so far which in this case includes inline-label, i4 (or .inlret7 in Go 1.7.1's case).

I have a solution at JayNakrani@7b1b24d. It passes all the tests and compiles this code successfully, but I am not sure if that's good enough.

(Side note: Another file that shows <output of Dump()> : <human readable pseudo code>.)

@JayNakrani
Copy link
Contributor

Adding some folks who last modified that part of racewalk.go

@ianlancetaylor @dr2chase : Can you confirm if the potential solution is good enough to be sent for review? Other than running tests and manual inspection of generated code, how can I tell if the code is working correctly?

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 17, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Oct 17, 2016
@quentinmit
Copy link
Contributor

/cc @randall77

@ianlancetaylor
Copy link
Contributor

There's no need for pre-review of a change review. We are used to reviewing in Gerrit, so it will be easier for us if you simply send the change following the guidelines at https://golang.org/doc/contribute.html .

Your patch should include a new test.

@JayNakrani
Copy link
Contributor

Sent it for review with test : https://go-review.googlesource.com/#/c/31317/

@gopherbot
Copy link

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

@dsnet dsnet mentioned this issue Oct 21, 2016
Closed
@golang golang locked and limited conversation to collaborators Dec 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@quentinmit @adamfaulkner @ianlancetaylor @JayNakrani @gopherbot