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,runtime: go tip breaks github.com/ethereum/go-ethereum/rlp #32595

Closed
cuonglm opened this issue Jun 13, 2019 · 7 comments
Closed

cmd/compile,runtime: go tip breaks github.com/ethereum/go-ethereum/rlp #32595

cuonglm opened this issue Jun 13, 2019 · 7 comments
Assignees
Milestone

Comments

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Jun 13, 2019

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

$ go version
go version devel +b388d6868f Thu Jun 13 03:58:18 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No.

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

go env Output
$ go env
GO111MODULE="off"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cuonglm/.cache/go-build"
GOENV="/home/cuonglm/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cuonglm/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/cuonglm/sources/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/cuonglm/sources/go/pkg/tool/linux_amd64"
GCCGO="/usr/local/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build174470444=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ cd $GOPATH/src/github.com/ethereum/go-ethereum/rlp
$ go test -count=1

What did you expect to see?

Test passed.

What did you see instead?

--- FAIL: TestDecodeWithByteReader (0.00s)
    decode_test.go:582: test 40: value mismatch
        got  [0]uint8{}
        want [0]uint8{}
        decoding into *[0]uint8
        input "80"
--- FAIL: TestDecodeWithNonByteReader (0.00s)
    decode_test.go:582: test 40: value mismatch
        got  [0]uint8{}
        want [0]uint8{}
        decoding into *[0]uint8
        input "80"
--- FAIL: TestDecodeStreamReset (0.00s)
    decode_test.go:582: test 40: value mismatch
        got  [0]uint8{}
        want [0]uint8{}
        decoding into *[0]uint8
        input "80"
FAIL
exit status 1
FAIL	github.com/ethereum/go-ethereum/rlp	0.005s

The test still works with go 1.12.6 and earlier.

I can't produce with minimal example, but cherry pick point to fff4f59

cc @randall77

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Jun 13, 2019

Found a reproducer. But very oddly, it fails only inside a test. Not in a main function.

func TestMy(t *testing.T) {
	deref := reflect.ValueOf(new([0]byte)).Elem().Interface()
	t.Log(reflect.DeepEqual(deref, [0]byte{}))
}

This gives false at tip, but true with 1.12.

But running this inside a func main() always gives true with both.

@martisch

This comment has been minimized.

Copy link
Member

@martisch martisch commented Jun 13, 2019

As I see a new([0]byte) there and an interface. Is this a case of pointers to zero sized types may or may not be equal? https://golang.org/ref/spec#Comparison_operators

One thing I remember that changed in relation to zero sized allocations in go1.13 was: https://go-review.googlesource.com/c/go/+/155840

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

@cuonglm cuonglm commented Jun 13, 2019

@martisch the weird thing is that it's only occurs inside a test.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jun 13, 2019

This is strange. It is a real bug, though.

With the stack-allocated defer CL, there are at runtime two reflect.rtype instances representing [0]uint8. The test makes two instances of a [0]uint8 in an interface, each with a type field pointing to a different reflect.rtype. Thus they don't compare equal, even though they should. We should never make two reflect.rtype instances representing the same type.

The second [0]uint8 type I think comes from when we build a defer struct for a stack-allocated defer. For a defer with no args (which I think is the one at github.com/ethereum/go-ethereum/rlp/typecache.go:80), one of the fields of that struct is a zero-sized array.

Somehow a real [0]uint8 type in the program is getting replaced with the [0]uint8 type that was used to build the defer struct. That shouldn't happen. We construct non-user-visible types in the compiler in lots of places (0-sized and otherwise), and I've never seen this before.

Next up, try to figure out why we're promoting one of these internal-only types to a real type. Particularly, why this happens with defer structs and not with other similar types (map buckets, map iteration structs, temp bufs for string ops).

@randall77 randall77 self-assigned this Jun 13, 2019
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jun 13, 2019

I guess we need to mark the compiler-created type NoAlg? (as map buckets do) So it won't get pulled into typelink.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Jun 13, 2019

Seems like a Noalg issue, there's one [0]uint8 with an algorithm table and one without.
I can fix the bug by not setting Noalg on the [0]uint8 created for the defer struct.
Seems related to #17752.
From that CL, it looks like the fix for #17752 doesn't quite work here.

I think what is going on is that we have a regular [0]byte type and a noalg [0]byte type. But there is also a *[0]byte type, and it only has one Elem field. In this case, the pointer in the *[0]byte Elem field points to the noalg [0]byte, not the regular one. Thus it doesn't compare equal with the regular one.

Not marking the byte array in the defer struct as noalg is probably the simplest fix, and I will do that for 1.13. (It isn't a big deal, as the alg pointer for [N]byte things will be shared.) We may need a deeper fix, though. One thing that does work is to change types.NewPtr to make a noalg pointer type when the base is noalg.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 14, 2019

Change https://golang.org/cl/182357 mentions this issue: cmd/compile: don't mark argument array as noptr

@gopherbot gopherbot closed this in 343b7fa Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.