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: assert in "implements_interface", types.cc:9145 with inlinable function #33013

Closed
thanm opened this issue Jul 9, 2019 · 5 comments
Closed
Assignees
Milestone

Comments

@thanm
Copy link
Member

@thanm thanm commented Jul 9, 2019

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

Using gccgo tip

$ go version
go version go1.12.2 gccgo (GCC) 10.0.0 20190624 (experimental) linux/amd64

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

linux/amd64

What did you do?

Build the following three packages: first "a", then "b", and finally "c".

Package "a":

package a

import "context"

type Service uint64
type ServiceDesc struct {
	X int
	uc
}

type uc interface {
	f() context.Context
}

var q int

func RS(svcd *ServiceDesc, server interface{}, qq uint8) *Service {
	defer func() { q += int(qq) }()
	return nil
}

Package "b":


import (
	"a"
	"context"
)

type BI interface {
	Something(s int64) int64
	Another(pxp context.Context) int32
}

func BRS(sd *a.ServiceDesc, server BI, xyz int) *a.Service {
	return a.RS(sd, server, 7)
}

Package "c":


import (
	"a"
	"b"
)

var GA a.Service

func C() {
	b.BRS(nil, nil, 22)
}

What did you expect to see?

Clean compile

What did you see instead?

Assert as follows:

go1: internal compiler error: in implements_interface, at go/gofrontend/types.cc:9145
0xbc4cfc Interface_type::implements_interface(Type const*, std::__cxx11::basic_string, std::allocator >*) const
	../../gcc-trunk/gcc/go/gofrontend/types.cc:9145
0xbaa243 Type::are_assignable(Type const*, Type const*, std::__cxx11::basic_string, std::allocator >*)
	../../gcc-trunk/gcc/go/gofrontend/types.cc:737
0xbaa6e1 Type::are_convertible(Type const*, Type const*, std::__cxx11::basic_string, std::allocator >*)
	../../gcc-trunk/gcc/go/gofrontend/types.cc:813
0xab2a5e Type_conversion_expression::do_check_types(Gogo*)
	../../gcc-trunk/gcc/go/gofrontend/expressions.cc:3921
0xb370bb Expression::check_types(Gogo*)
	../../gcc-trunk/gcc/go/gofrontend/expressions.h:957
0xb24059 Check_types_traverse::expression(Expression**)
	../../gcc-trunk/gcc/go/gofrontend/gogo.cc:3795
0xaaab16 Expression::traverse(Expression**, Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/expressions.cc:45
0xadff7f Expression_list::traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/expressions.cc:18499
0xac6a3d Call_expression::do_traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/expressions.cc:10647
0xaaab51 Expression::traverse(Expression**, Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/expressions.cc:51
0xb8f033 Statement::traverse_expression(Traverse*, Expression**)
	../../gcc-trunk/gcc/go/gofrontend/statements.cc:86
0xb91133 Assignment_statement::do_traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/statements.cc:883
0xb8ef79 Statement::traverse(Block*, unsigned long*, Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/statements.cc:56
0xb2d9ea Block::traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/gogo.cc:6854
0xba2a20 Block_statement::do_traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/statements.h:970
0xb8ef79 Statement::traverse(Block*, unsigned long*, Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/statements.cc:56
0xb2d9ea Block::traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/gogo.cc:6854
0xb29608 Function::traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/gogo.cc:5760
0xb341f8 Bindings::traverse(Traverse*, bool)
	../../gcc-trunk/gcc/go/gofrontend/gogo.cc:9130
0xb22115 Gogo::traverse(Traverse*)
	../../gcc-trunk/gcc/go/gofrontend/gogo.cc:2841
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.

What appears to be happening is as follows:

  1. Compiler imports "a", and in the process reads in type information for the "context.Context" type. Here are excerpts from the dump of a's export data:
types 30 3 ...
type 1 "Service" 
type 2 "ServiceDesc" 
type 3 ".a.uc" 
type 4 "context.Context" 
...
type 22 struct { X ; ? ; }
type 27 interface { }
type 28 interface { .a.f () ; }
type 29 interface { Deadline () (deadline , ok ); Done () ; Err () ; Value (key ) ; }

Note that there is a chain of type references from one of the exported types ("ServiceDesc") to the empty interface type corresponding to "type 27", which gets created eagerly (since it is needed by an exported type).

  1. Compiler imports "b", which contains an inlineable function "BRS". Here are excerpts from b's export data:
types 33 2 ...
type 1 "BI" 
type 2 ".a.uc" 
type 3 "a.Service" 
type 4 "a.ServiceDesc" 
type 5 "context.Context" 
type 29 interface { }
type 30 interface { .a.f () ; }
type 31 interface { Another (pxp ) ; Something (s ) ; }
type 32 interface { Deadline () (deadline , ok ); Done () ; Err () ; Value (key ) ; }
func BRS (sd  , server  , xyz ) ($ret0 ) 
// /ssd2/go1/src/b/b.go:13
{ //14
$ret0 = RS(sd, $convert(, server), $convert(, 7 )) //14
return //14
} //0

Key thing to note is that we have an exported type 'BI', which refers to context.Context. Since context.Context was encountered previously, we discover this fact at the point where we import the named type, and so we reuse the previously established Type object. See import.cc line 1091.

However we continue on to read the methods (import.cc line 1103, meaning that we create a new empty interface corresponding to "type 29" (which again gets created eagerly).

The key difference here is that there is no reference to this type from an exported type in the package (we broke the link by using the previous definition of conext.Context).

  1. Compiler calls Gogo::finalize_methods. This visits all of the various methods reachable from package bindings, but it skips "type 29" (as described above) since there is no way to reach it.

  2. Compiler calls Gogo::lower_parse_tree(). This has the side effect of bringing in the body of the inlineable function "BRS" from package "b". We now have a live reference to type 29.

  3. Compiler calls check_types() -- now we encounter type 29, and assert since it wasn't exposed to finalize_methods().

Possible approaches to fixing this.

One possibility would be to change Import::read_named_type so that if it encounters a previously defined type it doesn't read it at all -- just returns the previously defined type. I prototyped this, but I don't think it will work in this specific case: there is still a dangling reference to 'type 29' via the prototype for the "a.RS" declaration, so the phantom type still gets created on import. We'd need some way to skip it as well.

Another hacky solution would be to set an interface finalized on creation if its parse_methods are empty.

@thanm thanm added this to the Gccgo milestone Jul 9, 2019
@thanm thanm self-assigned this Jul 9, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 9, 2019

What would happen if we changed Import::read_type (not Import_function_body::read_type) so that it calls finalize_methods_for_type after calling type_for_index, just as Import_function_body::read_type does?

@thanm
Copy link
Member Author

@thanm thanm commented Jul 9, 2019

I am OK with that I suppose. Input::read_type gets called quite early, but then again finalize_methods happens pretty early as well. I will give it a try.

@thanm
Copy link
Member Author

@thanm thanm commented Jul 10, 2019

It looks like an eager call to finalize_methods_for_type from Import::read_type has some problems -- what seems to happen is that we read a named type NT (but not its underlying type), create a forward declaration type for NT, then read another type T2 whose methods refer to NT. When we try to finalize methods for T2, we get an assert, e.g.

#1  Forward_declaration_type::warn () at ../../gcc-trunk/gcc/go/gofrontend/types.cc:12278
#2  Forward_declaration_type::real_type () at ../../gcc-trunk/gcc/go/gofrontend/types.cc:12327
#3  Forward_declaration_type::do_hash_for_method () at ../../gcc-trunk/gcc/go/gofrontend/types.h:3760
#4  Type::hash_for_method () at ../../gcc-trunk/gcc/go/gofrontend/types.cc:943
...

I'm going to propose an alternative, which is to change the importer to make a second pass through the types array after exported types are read in and call finalize_methods_for_type then.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 10, 2019

Change https://golang.org/cl/185517 mentions this issue: test: new testcase for gccgo compiler bug

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 10, 2019

Change https://golang.org/cl/185518 mentions this issue: compiler: finalize methods when importing types

gopherbot pushed a commit that referenced this issue Jul 10, 2019
Updates #33013

Change-Id: I3db062b37860bb0c6c99a553408b47cf0313531e
Reviewed-on: https://go-review.googlesource.com/c/go/+/185517
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
kraj pushed a commit to kraj/gcc that referenced this issue Jul 10, 2019
    
    This patch changes the compiler to be more aggressive about finalizing
    methods on imported types, to avoid problems with interface types that
    are imported but remain unreachable until a later stage in the compilation.
    
    The normal pattern prior to this change was that the import process would
    leave imported interface types alone, and rely on Gogo::finalize_methods
    to locate and finalize all interface types at a later point. This way
    of doing things was not working in all cases due to the fact that we can
    import an interface type that is only reachable from the body of an
    inlinable function, meaning that we can't "find" the type during
    the methods finalize phase.
    
    The importer's Import::read_types() now makes a pass over all imported
    types to finalize methods on any newly imported type, which takes care
    of the issue.
    
    New test case for this problem in CL 185517.
    
    Fixes golang/go#33013
    
    Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/185518


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@273364 138bc75d-0d04-0410-961f-82ee72b054a4
@golang golang locked and limited conversation to collaborators Jul 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.