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: expected struct { F uintptr; L *lua.Lua_State } value to have type struct { F uintptr; L *lua.Lua_State } #62498

Closed
raochq opened this issue Sep 7, 2023 · 16 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@raochq
Copy link

raochq commented Sep 7, 2023

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

$ go version
go version go1.21.0 windows/amd64

Does this issue reproduce with the latest release?

This issue only occurs in go 1.21
It work well before go1.21;
It work well in go1.21 with '-gcflags "all=-l"';

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

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\arao\AppData\Local\go-build
set GOENV=C:\Users\arao\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=D:\NN4\server\server\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=D:\NN4\server\server\
set GOPRIVATE=
set GOPROXY=https://goproxy.cn,direct
set GOROOT=C:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.21.0
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=D:\NN4\server\server\go.work
set CGO_CFLAGS=-ID:\NN4\server\server\local\include -ID:\NN4\server\server\local\include\luajit-2.1
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-LD:\NN4\server\server\local\lib\windows_amd64 -ltolua -lpbc -lluajit-5.1 -lmingwex
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\arao\AppData\Local\Temp\go-build2469763229=/tmp/go-build -gno-record-gcc-switches

What did you do?

go install battlesvr

What did you expect to see?

build success

What did you see instead?

# nikki/golua
src\nikki\golua/luastack.go:19:12: internal compiler error: expected struct { F uintptr; L *lua.Lua_State } value to have type struct { F uintptr; L *lua.Lua_State }

Please file a bug report including a short program that triggers the error.
https://go.dev/issue/new

code

////luastack.go
package golua

import (
	"nikki/golua/lua"
	"nikki/golua/tolua"

	"fmt"
	"runtime"
	"unsafe"
)

type LuaStack struct {
	l *lua.Lua_State
}

func NewLuaStack() *LuaStack {
	L := lua.LuaL_newstate()
	lua.LuaL_openlibs(L)
	tolua.Open(L) // The problem occurs in this line
	stack := &LuaStack{
		l: L,
	}
	runtime.SetFinalizer(stack, (*LuaStack).Close)
	return stack
} 
//// tolua.go
package tolua

/*
#include <tolua/tolua.h>
#include <stdlib.h>
*/
import "C"

import (
	"nikki/golua/lua"
	"unsafe"
)
// The problem occurs in this func
func Open(L *lua.Lua_State) {
	C.tolua_open((*C.lua_State)(L))
}
////lua.go
package lua

/*
#include <lua.h>
#include <stdlib.h>

LUA_API const char *(Lua_pushfstring) (lua_State *L, const char *s) { return lua_pushfstring(L, s); }
*/
import "C"

import (
	"fmt"
	"unsafe"
)
type Lua_State C.lua_State
func LuaL_newstate() *Lua_State { return (*Lua_State)(C.luaL_newstate()) }
@egonelbre
Copy link
Contributor

Similar issue can happen when you are typedef-in the same struct from multiple places via headers.

Does the problem persist when you move the code into a single file?

In other words, you should have only one single definition for *lua.Lua_State.

@raochq
Copy link
Author

raochq commented Sep 7, 2023

This problem is no longer present when I write function Open not in one line.

func Open(L *lua.Lua_State) {
	l := (*C.lua_State)(L)
	C.tolua_open(l)
}

@egonelbre
Copy link
Contributor

So, yes, *C.lua_State and *lua.Lua_state are distinct types for Go.

See https://pkg.go.dev/cmd/cgo#hdr-Go_references_to_C

Cgo translates C types into equivalent unexported Go types. Because the translations are unexported, a Go package should not expose C types in its exported API: a C type used in one Go package is different from the same C type used in another.

There's a separate issue for having a nicer solution for this problem: #13467

@egonelbre
Copy link
Contributor

Although, I don't think the compiler should ICE due to that.

@cherrymui cherrymui changed the title internal compiler error: expected struct { F uintptr; L *lua.Lua_State } value to have type struct { F uintptr; L *lua.Lua_State } cmd/compile: internal compiler error: expected struct { F uintptr; L *lua.Lua_State } value to have type struct { F uintptr; L *lua.Lua_State } Sep 7, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 7, 2023
@cherrymui cherrymui added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Sep 7, 2023
@cherrymui cherrymui added this to the Go1.22 milestone Sep 7, 2023
@cherrymui
Copy link
Member

cc @golang/compiler

@mdempsky mdempsky self-assigned this Sep 8, 2023
@mdempsky
Copy link
Contributor

mdempsky commented Sep 8, 2023

I'll look into this tomorrow, but if you can simplify the test case to work with only standard C (or at least POSIX) types/includes that will simplify reproducing it for me. Thanks.

@raochq
Copy link
Author

raochq commented Sep 8, 2023

This is the simplify code。 Hope this helps troubleshooting the issue.
https://github.com/raochq/cgotest.git

@cuonglm
Copy link
Member

cuonglm commented Sep 8, 2023

The problem seems to be a more agressive inlining of closures in go1.21

The closure generated for C.tolua_open in function:

func Open(L *lua.Lua_State) {
	C.tolua_open((*C.lua_State)(L))
}

can now be inlined, there's no generated Go type for (*C.lua_State)(L) in package tolua anymore.

It ends up making two closures with different capture vars: C.luaL_openlibs and C.tolua_open, having the same link string struct { F uintptr; L *github.com/raochq/cgotest/golua/lua.Lua_State }, but are not identical, because the closure pointer F have different symbol: lua..F and tolua..F.

@mdempsky
Copy link
Contributor

mdempsky commented Sep 8, 2023

@raochq Thank you, I can reproduce the failure.

@mdempsky
Copy link
Contributor

mdempsky commented Sep 9, 2023

Here's a standalone repro of the issue: https://go.dev/play/p/5eDYV-mzqg9

There's nothing wrong with this code, and it compiled in Go 1.20. So I think this is a regression that should be backported.

@gopherbot Please backport to Go 1.21. This is a compilation failure of valid code that used to work in Go 1.20.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #62544 (for 1.20), #62545 (for 1.21).

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

@mdempsky
Copy link
Contributor

mdempsky commented Sep 9, 2023

@cuonglm

but are not identical, because the closure pointer F have different symbol: lua..F and tolua..F.

Thanks, I concur. That's an issue: exported struct fields are supposed to always use ir.LocalPkg for their Sym.

@mdempsky
Copy link
Contributor

mdempsky commented Sep 9, 2023

Oh wait, the field name is actually .F, which isn't exported.

But Type.LinkString is rewriting this to just F, which is a problem, because LinkString is supposed to match type identity.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/527135 mentions this issue: cmd/compile/internal/typecheck: fix closure field naming

@mdempsky
Copy link
Contributor

mdempsky commented Sep 9, 2023

This problem is no longer present when I write function Open not in one line.

This is because the problem relates to closures capturing local variables with uppercase names. (There aren't closures in your code as written, but cgo introduces them as part of its rewriting.)

I've uploaded a fix, and I hope to have it included in the next Go minor release. In the mean time, disabling inlining like you discovered should work. Alternatively, you could rename your variables to start with a lowercase letter.

Sorry for the inconvenience. Thank you for the report and for preparing the standalone reproduction.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 12, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/534916 mentions this issue: [release-branch.go1.21] cmd/compile/internal/typecheck: fix closure field naming

gopherbot pushed a commit that referenced this issue Oct 13, 2023
…ield naming

When creating the struct type to hold variables captured by a function
literal, we currently reuse the captured variable names as fields.

However, there's no particular reason to do this: these struct types
aren't visible to users, and it adds extra complexity in making sure
fields belong to the correct packages.

Further, it turns out we were getting that subtly wrong. If two
function literals from different packages capture variables with
identical names starting with an uppercase letter (and in the same
order and with corresponding identical types) end up in the same
function (e.g., due to inlining), then we could end up creating
closure struct types that are "different" (i.e., not types.Identical)
yet end up with equal LinkString representations (which violates
LinkString's contract).

The easy fix is to just always use simple, exported, generated field
names in the struct. This should allow further struct reuse across
packages too, and shrink binary sizes slightly.

For #62498.
Fixes #62545.

Change-Id: I9c973f5087bf228649a8f74f7dc1522d84a26b51
Reviewed-on: https://go-review.googlesource.com/c/go/+/527135
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit e3ce312)
Reviewed-on: https://go-review.googlesource.com/c/go/+/534916
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants