test: go run run.go -linkshared fails

[mwhudson]

Please answer these questions before submitting your issue. Thanks!

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

go version devel +50edddb Tue Aug 2 21:31:58 2016 +0000 linux/386

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

Ubuntu 16.04, not that I expect it matters.

1. What did you do?

GOHOSTARCH=386 GOARCH=386 ./make.bash && \
    go install -buildmode=shared std && \
    cd ../test && \
    go run run.go -linkshared

1. What did you expect to see?

everything passing

1. What did you see instead?

exit status 2
# command-line-arguments
/tmp/318403551/tmp__.go:99: syntax error: missing operand

FAIL    64bit.go    2.105s
# go run run.go -- const3.go
exit status 1
type info didn't propagate in const: got 2 4 16 256 65536
panic: fail

goroutine 1 [running]:
panic(0xf73ffec0, 0xd6474160)
    /opt/opensource/go/src/runtime/panic.go:500 +0x59a
main.main()
    /opt/opensource/go/test/const3.go:29 +0x48b
exit status 2

FAIL    const3.go   1.695s

(and 'goprint' hangs and has to be killed)

This is a regression from 1.6 but too late to fix for release. I'll patch it in Ubuntu if I have to :-)

[randall77]

@crawshaw

Simple repro

package main

import "fmt"

type T int

func (t T) String() string { return "foo" }

const x T = 5

func main() {
    fmt.Printf("%v\n", x)
}

Build with

$ GOHOSTARCH=386 GOARCH=386 ./make.bash
$ go install -buildmode=shared std
$ go build -linkshared test.go

Works fine without the linkshared.
Same problem on amd64.

I suspect the problem is in the handleMethods function in fmt/print.go. It looks like it is returning false even though T is a Stringer.

[mwhudson]

Oh, that's pretty bad :(

[randall77]

Yes, type switches no worky. I'm going to mark as 1.7.

[crawshaw]

I'll take a look tomorrow. There was a similar problem early in the 1.7 cycle where the type is defined in one module and used from another, though I don't know why it would be showing up only for GOARCH=386 now.

[mwhudson]

It's not just showing up on 386, it happens on amd64 too.

[crawshaw]

(Dumping info on this bug as I find it.)

To confirm this is a problem with interface types used across modules:

package main

import "fmt"

type T int

func (t T) String() string { return "foo" }

type stringer interface {
        String() string
}

func main() {
        x := T(5)
        var y interface{} = x
        _, ok1 := y.(fmt.Stringer)
        _, ok2 := y.(stringer)
        println(ok1, ok2)
}

A standard build prints true, true
A build with -linkshared prints `false, true``

[crawshaw]

The -linkshared program above has two different *_type values for func() string, one from each module.

The runtime requires there be a single *_type for each type, and for multi-module programs the initialization in runtime.typelinksinit should de-duplicate the *_type instances. In this case it's not because these types have different package names: one is "main" and the other is "fmt", so runtime.typesEqual is returning false.

Package names are used on these types so that package p2 cannot implement an unexported interface method from package p1. That reasoning does not apply to exported method names, and so the smallest fix to this is to ignore package names on exported function types in runtime.typesEqual. I'm not sure if this is a general fix, still pondering.

Fortunately all of this code is guarded behind firstmoduledata.next != nil, so it should be safe to add this to 1.7 without worrying that it effects any of the more tested build modes.

[crawshaw]

Actually, the bug is not in typesEqual. It correctly identifies the equality and builds the module typeMap properly.

It's something to do with the typemap map[typeOff]*_type as called via typeOff via additab. Under some conditions, md.typemap[off] returns the *_type, sometimes it returns nil. When it returns nil, I can range over the map, find the key and the value is correct!

That is, the following (absurd) patch fixes the example programs above:

diff --git a/src/runtime/type.go b/src/runtime/type.go
index 5ef11a4..7a3ab10 100644
--- a/src/runtime/type.go
+++ b/src/runtime/type.go
@@ -224,8 +224,10 @@ func (t *_type) typeOff(off typeOff) *_type {
                }
                return (*_type)(res)
        }
-       if t := md.typemap[off]; t != nil {
-               return t
+       for moff, mres := range md.typemap {
+               if moff == off {
+                       return mres
+               }
        }
        res := md.types + uintptr(off)
        if res > md.etypes {

[crawshaw]

I don't see any obvious codegen bugs in the reflect.typeOff method.

@randall77 is there any reason why a mapaccess from additab could be a problem? I can replace the data structure with a sorted slice and binary search to fix this, but it would be nice to know why this is broken.

[josharian]

Stupid questions: data race? If you use comma ok during map access, what does ok say?

[crawshaw]

The map is built in typelinksinit and not written to again, so I believe a data race is out. With the patch:

diff --git a/src/runtime/type.go b/src/runtime/type.go
index 5ef11a4..0696efa 100644
--- a/src/runtime/type.go
+++ b/src/runtime/type.go
@@ -224,8 +224,13 @@ func (t *_type) typeOff(off typeOff) *_type {
                }
                return (*_type)(res)
        }
-       if t := md.typemap[off]; t != nil {
-               return t
+       for moff, mres := range md.typemap {
+               if moff == off {
+                       if t, ok := md.typemap[off]; t == nil {
+                               println("md.typemap[", off, "] in map, but t=", t, ", ok=", ok)
+                       }
+                       return mres
+               }
        }
        res := md.types + uintptr(off)
        if res > md.etypes {

then Keith's test program above prints

md.typemap[ 1400352 ] in map, but t= 0x0 , ok= false
md.typemap[ 1400352 ] in map, but t= 0x0 , ok= false
md.typemap[ 1400352 ] in map, but t= 0x0 , ok= false
foo

[randall77]

I don't see any reason why mapaccess would fail there. Any chance there is more than one copy of the runtime package around? That would screw with hash functions.

[randall77]

...or maybe you build the map before the init function in alg.go? That would make it fail.

[crawshaw]

@randall77 that's it, typelinksinit runs before alg.go init. Thanks. I'll send a CL in a bit.

[crawshaw]

I think the fix to this (https://golang.org/cl/25469) calls for RC6.

/cc @ianlancetaylor @adg @quentinmit

[quentinmit]

@crawshaw Sadly, I agree. I suggest that we wait until Monday to cut RC6, in the hopes that any more latent bugs will shake out before then.

[mwhudson]

Thanks so much for fixing this, and sorry for finding it so late!

[broady]

Is this a regression from 1.6?

[mwhudson]

Yes it is (or was, I guess, now it's fixed)