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/asm: "compile: loop" compiler bug? #49716

Closed
toxyl opened this issue Nov 22, 2021 · 7 comments
Closed

cmd/asm: "compile: loop" compiler bug? #49716

toxyl opened this issue Nov 22, 2021 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@toxyl
Copy link

toxyl commented Nov 22, 2021

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

$ go version
go version go1.17.2 linux/amd64

I have tested all 1.16 and 1.17 versions, the bug does not occur in any 1.16.x version and occurs in every 1.17.x version.

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/redacted/.cache/go-build"
GOENV="/redacted/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/redacted/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/redacted/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.17"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.17/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/redacted/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2541760486=/tmp/go-build -gno-record-gcc-switches"

What did you do?

To test a feature I wanted to run my program with the race detector but it would fail with the error "compile: loop" which seems to originate in https://github.com/golang/go/blob/master/src/cmd/internal/obj/x86/asm6.go#L2179

However, the error occurred in a part of the program that was unaffected by the feature I wanted to test. Rather in a slightly modified version of go-syslog (https://pkg.go.dev/gopkg.in/mcuadros/go-syslog.v2). Since I can't share the original code I've build a test case that can reliably reproduce the error:

package main

import (
	"time"
)

type Priority struct {
	P int
	// remove any of the following fields to make the compile succeed
	F Facility
	S Severity
}

type Facility struct {
	// remove Value field to make the compile succeed
	Value int
}

type Severity struct{}

type header struct {
	priority Priority
	// remove any of the following fields to make the compile succeed
	version   int
	timestamp time.Time
	hostname  string
	appName   string
	procId    string
}

type Parser struct {
	buff           []byte
	cursor         int
	l              int
	header         header
	structuredData map[string]interface{}
}

func NewParser(buff []byte) *Parser {
	return &Parser{}
}

func (p *Parser) initDefaultValue(key string, defaultValue interface{}) {
	if _, ok := p.structuredData[key]; !ok {
		// remove this line to make the compile succeed
		p.structuredData[key] = defaultValue
	}
}

func (p *Parser) parseHeader() (header, error) {
	// remove this call to make the compile succeed
	p.parsePriority()
	return header{}, nil
}

func newPriority(p int) Priority {
	return Priority{P: p}
}

func ParsePriority(buff []byte, cursor *int, l int) (Priority, error) {
	pri := newPriority(0)

	i := 1

	// hardcode value instead of using var to make compile succeed
	priDigit := 0

	for i < l {
		if i >= 5 {
			return pri, nil
		}

		c := buff[i]

		if c == '>' {
			return newPriority(priDigit), nil
		}

		i++
	}

	return pri, nil
}

func (p *Parser) parsePriority() (Priority, error) {
	return ParsePriority(p.buff, &p.cursor, p.l)
}

func (p *Parser) Parse() error {
	// remove this block to make the compile succeed
	_, err := p.parseHeader()
	if err != nil {
		return err
	}

	p.initDefaultValue("a", "anything")
	p.initDefaultValue("b", "you")
	p.initDefaultValue("c", "can")
	p.initDefaultValue("d", "imagine")
	p.initDefaultValue("e", "is")
	p.initDefaultValue("f", "fine")
	p.initDefaultValue("g", "as")
	p.initDefaultValue("h", "input")
	p.initDefaultValue("i", "but")
	p.initDefaultValue("j", "too")
	p.initDefaultValue("k", "many")
	p.initDefaultValue("l", "initDefaultValue")
	p.initDefaultValue("m", "calls")
	p.initDefaultValue("n", "break")
	p.initDefaultValue("o", "it")
	p.initDefaultValue("p", ",")
	p.initDefaultValue("q", "take")
	p.initDefaultValue("r", "away")
	p.initDefaultValue("s", "the")
	p.initDefaultValue("t", "last")
	p.initDefaultValue("u", "initDefaultValue")
	p.initDefaultValue("v", "and")
	p.initDefaultValue("x", "magically")
	p.initDefaultValue("y", "it")
	p.initDefaultValue("z", "starts")
	p.initDefaultValue("1", "working")
	p.initDefaultValue("2", "again")
	p.initDefaultValue("3", ".")
	p.initDefaultValue("4", "but")
	p.initDefaultValue("5", "there")
	p.initDefaultValue("6", "are")
	p.initDefaultValue("7", "more")
	p.initDefaultValue("8", "places")
	p.initDefaultValue("9", "you")
	p.initDefaultValue("10", "can")
	p.initDefaultValue("11", "remove")
	p.initDefaultValue("12", "to")
	p.initDefaultValue("13", "make")
	p.initDefaultValue("14", "it")
	p.initDefaultValue("15", "work")
	p.initDefaultValue("16", "...")
	p.initDefaultValue("16", "*sigh*")
	p.initDefaultValue("17", 0)
	p.initDefaultValue("18", 0)
	p.initDefaultValue("19", 0)
	p.initDefaultValue("20", 0)
	p.initDefaultValue("21", 0)

	return nil
}

func main() {
	_ = NewParser([]byte(""))
}

To make things clear, I'm trying to run the above with the race detector:

go run -race test-case.go

Without race detector the program runs fine.

Note: There are a bunch of comments in the code above. Each of those is a change that makes the bug disappear but the careful observer might notice that several of these should have no impact on how the program operates.

What did you expect to see?

The program starting with the race detector.

What did you see instead?

compile: loop
@zhouguangyuan0718
Copy link
Contributor

zhouguangyuan0718 commented Nov 22, 2021

The function span6 in assembler of golang will exit with "span must be looping" when n > 20.

if n > 20 {

But I find that the fucntion span in inferno-os (which go convert from) will exit with "span must be looping" when n > 50.
https://bitbucket.org/inferno-os/inferno-os/src/4947f0601e5c1a549565a10a7d8c8624f98d0790/utils/6l/span.c#lines-66

Maybe we should also modify it to n > 50 in golang?

@ALTree ALTree changed the title "compile: loop" compiler bug? cmd/asm: "compile: loop" compiler bug? Nov 22, 2021
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 22, 2021
@ALTree
Copy link
Member

ALTree commented Nov 22, 2021

cc @cherrymui

@gopherbot
Copy link

Change https://golang.org/cl/366094 mentions this issue: cmd/internal/obj/x86: modify the threhold of assert loop for span6

@cherrymui
Copy link
Member

Yeah, 20 is rather too small. We could increase it to, say, 500 or 1000.

@manjari25
Copy link

manjari25 commented Feb 1, 2022

@gopherbot please consider this for backport to 1.17, it's a regression.

While upgrading to 1.17 from 1.16, several of our targets fail to build due to this error. We have verified that this fix works for us by patching it. We do not see any other workaround.

We would like to throw a voice out there that this solves a real regression for us that was introduced in 1.17 and we'd like to be able to use the latest release again :)

cc: @cherrymui Thanks so much for the fix!

@gopherbot
Copy link

Backport issue(s) opened: #50942 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

Change https://golang.org/cl/383214 mentions this issue: [release-branch.go1.17] cmd/internal/obj/x86: modify the threshold of assert loop for span6

gopherbot pushed a commit that referenced this issue Feb 7, 2022
… assert loop for span6

Updates #49716.
Fixes #50942.

Change-Id: I7ed73f874c2ee1ee3f31c9c4428ed484167ca803
Reviewed-on: https://go-review.googlesource.com/c/go/+/366094
Reviewed-by: Cherry Mui <cherryyz@google.com>
Trust: Heschi Kreinick <heschi@google.com>
(cherry picked from commit 14f2b2a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/383214
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alex Rakoczy <alex@golang.org>
@golang golang locked and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants