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

Created on 13 Jun 2019 · 7Comments · Source: golang/go

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 fff4f599fe1c21e411a99de5c9b3777d06ce0ce6

cc @randall77

FrozenDueToAge NeedsInvestigation release-blocker

Source

cuonglm

Most helpful comment

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.

agnivade on 13 Jun 2019

👍5

All 7 comments

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.

agnivade on 13 Jun 2019

👍5

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

martisch on 13 Jun 2019

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

cuonglm on 13 Jun 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 on 13 Jun 2019

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

cherrymui on 13 Jun 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.

randall77 on 13 Jun 2019

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