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: internal compiler error: width not calculated: map[string]string #44895

Closed
andig opened this issue Mar 9, 2021 · 7 comments
Closed
Assignees
Milestone

Comments

@andig
Copy link
Contributor

@andig andig commented Mar 9, 2021

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

$ go version
go version devel +48ddf70 Tue Mar 9 20:35:41 2021 +0000 darwin/arm64

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
❯ gotip env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/andig/Library/Caches/go-build"
GOENV="/Users/andig/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/andig/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/andig/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/andig/sdk/gotip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/andig/sdk/gotip/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="devel +48ddf70 Tue Mar 9 20:35:41 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/andig/htdocs/evcc/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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sv/rs_453y57xj86xsbz3kw1mbc0000gn/T/go-build3964674039=/tmp/go-build -gno-record-gcc-switches -fno-common"

In #43931 I've learned that I can use gotip to experiment with generic code and that (#43931 (comment))

Non-generic Go code is expected to fully work, so please file issues if you find anything that works without -G=3 but breaks with -G=3

I'm using

gotip run -gcflags=all=-G=3 main.go

on this piece of code:

func (v *Volvo) status() (interface{}, error) {
	var res volvoStatus

	req, err := v.request(fmt.Sprintf("%s/vehicles/%s/status", volvoAPI, v.vin))
	if err == nil {
		err = v.DoJSON(req, &res)
	}

	return res, err
}

What did you expect to see?

No error

What did you see instead?

vehicle/volvo.go:172:17: internal compiler error: width not calculated: map[string]string

The entire code is in https://github.com/andig/evcc/tree/go2/registry

@ianlancetaylor ianlancetaylor changed the title gotip: internal compiler error: width not calculated: map[string]string cmd/compile: internal compiler error: width not calculated: map[string]string Mar 10, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 10, 2021

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 10, 2021

Loading

@danscales
Copy link

@danscales danscales commented Mar 10, 2021

Yes, thanks for the simple example! I guess in rare cases we still need to do CheckSize in g.typ(), even though it shouldn't really be necessary - I think I should be able to figure out when exactly when/why this is needed.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

There's no real harm to extra calls to CheckSize, other than marginally extra CPU usage if we don't end up needing the type (e.g., dead code elimination or something). So feel free to liberally add them as needed to fix issues, as long as we add regress tests for them too.

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Mar 10, 2021

Yes, thanks for the simple example! I guess in rare cases we still need to do CheckSize in g.typ(), even though it shouldn't really be necessary - I think I should be able to figure out when exactly when/why this is needed.

I'm leaning to the same fix as @mdempsky suggested, like what I did in https://go-review.googlesource.com/c/go/+/299689. It's safer to just include the check, because now we don't compile function immediately but pushing to queue and compile later instead. So there's no way to guarantee that the type size is calculated. Previously, it's ok to compute size during SSA generation, now it doesn't.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

Previously, it's ok to compute size during SSA generation, now it doesn't.

While we didn't explicitly detect size calculations during non-concurrent SSA construction before, we also don't have any code that made us more aggressive about preemptively checking type sizes when concurrency was enabled. So any late size calculations that used to work, would have failed with concurrent compilations. We just don't have as much test coverage for concurrent compilation, and always using the compilation queue made those mistakes more visible.

If we wanted to restore the old, lax behavior, then I think in gc/compile.go:compileFunctions we could leave types.CalcSizeDisabled and base.Ctxt.InParallel as false when base.Flag.LowerC is 1. In that case, while there are multiple goroutines compiling everything, all of the work is serialized on a 1-element semaphore.

However, I'd lean towards keeping the failures noisy to help rooting them out and fixing them, rather than leaving them as latent issues for users that use concurrent compilation.

Loading

andig added a commit to evcc-io/evcc that referenced this issue Mar 10, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2021

Change https://golang.org/cl/300989 mentions this issue: cmd/compile: call types.CheckSize() in g.typ()

Loading

@gopherbot gopherbot closed this in 71a6c13 Mar 12, 2021
andig added a commit to evcc-io/evcc that referenced this issue Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants