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

gccgo: type switch broken on AIX #39276

Closed
Helflym opened this issue May 27, 2020 · 13 comments
Closed

gccgo: type switch broken on AIX #39276

Helflym opened this issue May 27, 2020 · 13 comments
Milestone

Comments

@Helflym
Copy link
Contributor

@Helflym Helflym commented May 27, 2020

I'm trying to build and test gccgo provided by gcc-10. But I've discovered that type-switch aren't working anymore on AIX, in some cases.
This occurs because of a type duplication between the Go program and the libgo (shared not with --static-libgo). On Linux, AFAIK the linker and the loader are able to merge symbols being the same across .o/.so and thus will merge type symbols if there are identical. On AIX, this merge doesn't occur. Thus, this can result of having two identical type symbols.

For example, I'm using this testcase:

package main

import (
        "fmt"
)

// Local fmt.Stringer interface
type Stringer interface {
        String() string
}

// Animal has a Name and an Age to represent an animal.
type Animal struct {
        Name string
        Age  uint
}

// String makes Animal satisfy the Stringer interface.
func (a Animal) String() string {
        return fmt.Sprintf("%v (%d)", a.Name, a.Age)
}

func main() {
        a := Animal{
                Name: "Gopher",
                Age:  2,
        }

        printType(a)
}

func printType(a interface{}) {
        switch t := a.(type) {
        case fmt.Stringer:
                fmt.Printf("fmt.Stringer %T\n", t)
        case Stringer:
                fmt.Printf("main.Stringer %T\n", t)
        default:
                fmt.Printf("default %T\n", t)
        }
}

With Go Toolchain and gccgo-9, the result was "fmt.Stringer main.Animal". With gccgo-10, the result is "main.Stringer main.Animal". That's because the type symbol of Animal.String() result isn't the same of the one expected by fmt.Stringer interface.

When using gdb and stopping at https://github.com/golang/gofrontend/blob/master/libgo/go/runtime/iface.go#L487, we have the following values ($r9 being toMethod.typ and $r10 fromMethod.mtyp, or the opposite but it doesn't really matter there)

(gdb) x /4gx $r9 
0x8001000a0108c68 <type..func.8.9.8string.9>:   0x0000000000000008      0x0000000000000008
0x8001000a0108c78 <type..func.8.9.8string.9+16>:        0x3699a68800080833      0x0000000000000000
(gdb) x /4gx $r10
0x110000d50 <type..func.8.9.8string.9>: 0x0000000000000008      0x0000000000000008
0x110000d60 <type..func.8.9.8string.9+16>:      0x3699a68800080833      0x0000000000000000

As you can see, these two type symbols are identical. But as there are two different pointers, the current code won't accept them.

This problem is triggered by https://go-review.googlesource.com/c/gofrontend/+/179598 and https://go-review.googlesource.com/c/gofrontend/+/182978. In fact, only the part about eqtype is problematic, because even if gccgo is generating unique type descriptors, on AIX the duplication can still occur.
I know that I'm discovering this really late. But would it be possible to keep eqtype function for AIX ? I've tried locally and it's working. The final solution would be to have this symbol merging happening also on AIX. But AFAIK it's not possible for now.

Another solution which we were using during gcc-7 and gcc-8, is to add -lgo before Go object files is also working (AIX linker is able to do the merge in this case). But the patch we have for that is too intrusive to be merged inside gcc tree (there are forcing -lgo in gcc.c), so I'd rather sticking to the first solution.

/cc @ianlancetaylor @cherrymui

@gopherbot gopherbot added this to the Gccgo milestone May 27, 2020
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 27, 2020

Thanks for the report. This is unfortunate. I guess we could restore the eqtype function on AIX.

Is there some linker magic to make it merge the symbols?

I agree that forcing -lgo doesn't seem nice. In theory there could be Go shared objects other than libgo itself, so forcing -lgo isn't a complete solution.

@Helflym
Copy link
Contributor Author

@Helflym Helflym commented May 28, 2020

Not that I'm aware of...
By the way, is there any other kind of symbols which can be common between object files and shared objects and which should be merged ? I'm only aware of type descriptors for now.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 28, 2020

There are some symbols that are nice to be merged, like compiler-generated wrapper functions, but the merging is not required for correctness, as we don't compare addresses of them. Type descriptors are probably the only one.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 4, 2020

Change https://golang.org/cl/235697 mentions this issue: runtime: revert eqtype for AIX

jpf91 pushed a commit to D-Programming-GDC/gcc that referenced this issue Aug 11, 2020
AIX linker is not able to merge identical type descriptors in a single
symbol if there are coming from different object or shared object files.
This results into several pointers referencing the same type
descriptors.
Thus, eqtype is needed to ensure that these different symbols will be
considered as the same type descriptor.

Fixes golang/go#39276

gcc/go/ChangeLog:

	* go-c.h (struct go_create_gogo_args): Add need_eqtype field.
	* go-lang.c (go_langhook_init): Set need_eqtype.

Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/235697
@Helflym
Copy link
Contributor Author

@Helflym Helflym commented Sep 15, 2020

I've discovered that there is another place where runtime.eqtype needs to be called. This happens when a type switch is checking for a "basic" types like string or int. Once again, for AIX even for "basic" types the type pointer is different. The statement to change is at https://github.com/golang/gofrontend/blob/master/go/statements.cc#L4669. But as there is no Gogo, I don't know how to check for Gogo->need_eqtype()

It can be reproduced with

package main

func do(i interface{}) bool {
	switch i.(type) {
	case string:
		return true
	case int:
		return true
	}
	return false
}

func main() {
	src := []byte("ab")
	do(src)

}

The current gimple representation for case string: is

        GOTMP.0 = i;
        GOTMP.1 = GOTMP.0.__type_descriptor;
        if (GOTMP.1 != &string..d) goto <D.1902>; else goto <D.1903>;
        <D.1902>:
        {
          goto <D.1619>;
        }
        <D.1903>:
        {
          {
            $ret0 = 1;
            D.1904 = $ret0;
            return D.1904;
          }
        }
        goto <D.1620>;
        <D.1619>:

It should be something like

        GOTMP.0 = i;
        GOTMP.1 = runtime.eqtype (&string..d, GOTMP.0.__type_descriptor);
        if (GOTMP.1 != &string..d) goto <D.1902>; else goto <D.1903>;
        <D.1902>:
        {
          goto <D.1619>;
        }
        <D.1903>:
        {
          {
            $ret0 = 1;
            D.1904 = $ret0;
            return D.1904;
          }
        }
        goto <D.1620>;
        <D.1619>:

Ps: The issue is already closed but that the exact same bug, thus I comment there. Tell me if I need to open another issue.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 15, 2020

You can pass in the Gogo* from the only caller in Type_switch_statement::do_lower. You'll have to pass it through Type_case_clauses::lower.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 16, 2020

Change https://golang.org/cl/255202 mentions this issue: compiler: call runtime.eqtype for non-interface type switch on aix

@Helflym
Copy link
Contributor Author

@Helflym Helflym commented Sep 16, 2020

I've checked quickly, but it doesn't seem to have anywhere else in the compiler that need to call runtime.eqtype.

However, reflect and internal/reflectlite also have the same problem. In implements, the methods' type are checked (https://github.com/golang/gofrontend/blob/master/libgo/go/internal/reflectlite/type.go#L542 and https://github.com/golang/gofrontend/blob/master/libgo/go/internal/reflectlite/type.go#L559) and the last assertion toType(vm.mtyp).common() == toType(tm.typ).common() will also fail depending on where the type pointers are taken from.

As a workaround, I've changed this assertion to use the hash parameter toType(vm.mtyp).common().hash == toType(tm.typ).common().hash, it's not 100% accurate but I guess it works in almost all cases. Still, that's a workaround any idea if it could be fixed easily ?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 17, 2020

You should be able to compare toType(vm.mtyp).String() == toType(tm.typ).String(), as runtime.eqtype does. But we should have an AIX and a non-AIX version.

gopherbot pushed a commit to golang/gofrontend that referenced this issue Sep 22, 2020
All type switch clauses must call runtime.eqtype if the linker isn't
able to merge type descriptors pointers. Previously, only interface-type
clauses were doing it.

Updates golang/go#39276

Change-Id: Ic06d95081e837122c409a2aba377d1afae676342
Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/255202
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Cherry Zhang <cherryyz@google.com>
pull bot pushed a commit to Qwerty0x64/gcc that referenced this issue Sep 22, 2020
All type switch clauses must call runtime.eqtype if the linker isn't
able to merge type descriptors pointers. Previously, only interface-type
clauses were doing it.

Updates golang/go#39276

Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/255202
@Helflym
Copy link
Contributor Author

@Helflym Helflym commented Oct 2, 2020

Using String() seems to work. However, I still have some issues with it.

First, how the equality operation is made between strings ? Because if I remember correctly, some constant strings can be associated to a Read-Only symbol (or at least an address in RO sections). Thus, checking that two pointers are equal would be faster that checking if the whole strings are equal. If it's indeed how it's performed, we would have the same problem than with types, these symbols won't be merged.

Moreover, I was able to fix type equalities in reflect and reflectlite. But what about stuff like https://github.com/golang/gofrontend/blob/master/libgo/go/internal/unsafeheader/unsafeheader_test.go#L61 ? Equality checks made on reflect.Type will fail, if it's targeting a type from the libgo and a type from somewhere else. I don't know if such things happens a lot in practice outside libgo tests. Adding a special case in the compiler for equality operations between reflect.Type for AIX might be a solution but I don't if it's possible or even the right thing to do.

Edit: Actually, the previous example is failing because the two iface are also two different pointers. Thus, https://github.com/golang/gofrontend/blob/master/libgo/go/runtime/alg.go#L315 will be false. Maybe the problem is similar with iface/eface/etc...

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 2, 2020

I don't think there is a problem with strings. For strings we never assume that if the pointers are not equal that the strings are not equal. The library assumes that types are canonicalized (there is only instance of a given type) but that is not true for strings (there can be multiple instances of a particular string).

In the reflect package, we used to have a canonicalization map that ensured that when the program sees a reflect.Type, it always sees a canonical version. That ensures that program comparisons of reflect.Type values do the right thing. That canonicalization map was removed in https://golang.org/cl/179598. Perhaps we need to bring it back for AIX.

(This of course assumes that there is only a single version of the canonicalization map in the program, which for AIX may mean that there is only a single version of the reflect package in the program. I don't know how AIX handles that kind of thing when shared libraries are involved.)

@Helflym
Copy link
Contributor Author

@Helflym Helflym commented Oct 5, 2020

No, it won't happen. Only the reflect tests might be a bit problematic as gotests is duplicating it. I'll check that anywya.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 7, 2020

Change https://golang.org/cl/260158 mentions this issue: reflect: ensure uniqueness of type descriptors on AIX.

pull bot pushed a commit to Qwerty0x64/gcc that referenced this issue Oct 14, 2020
On AIX, duplication of type descriptors can occur if one is
declared in the libgo and one in the Go program being compiled.
The AIX linker isn't able to merge them together as Linux one does.
One solution is to always load libgo first but that needs a huge mechanism in
gcc core. Thus, this patch ensures that the duplication isn't visible
for the end user.

In reflect and internal/reflectlite, the comparison of rtypes is made on their
name and not only on their addresses.

In reflect, toType() function is using a canonicalization map to force rtypes
having the same rtype.String() to return the same Type. This can't be made in
internal/reflectlite as it needs sync package. But, for now, it doesn't matter
as internal/reflectlite is not widely used.

Fixes golang/go#39276

Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/260158
rodgert added a commit to rodgert/gcc that referenced this issue Oct 16, 2020
All type switch clauses must call runtime.eqtype if the linker isn't
able to merge type descriptors pointers. Previously, only interface-type
clauses were doing it.

Updates golang/go#39276

Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/255202
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
5 participants
You can’t perform that action at this time.