code review 122850043: cmd/cgo: fix recursive type mapping

cmd/cgo: fix recursive type mapping

Instead of immediately completing pointer type mappings, add them to
a queue to allow them to be completed later.  This fixes issues	caused
by Type() returning arbitrary in-progress type mappings.

Fixes issue 8368.
Fixes issue 8441.

[mdempsky, 2014-08-05 00:40:11]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[mdempsky, 2014-08-05 19:47:23]

While playing around with fixes for issue 5253, I noticed a crash in "go tool
cgo -godefs" caused by godefsFields() expecting pointer types to be completed. 
I've fixed this in patch set 8 by deferring the godefsFields() calls until after
all pointer types are completed, but I'm not sure how to write a test case for
this since there don't seem to be any existing tests using "cgo -godefs"; the
closest is misc/cgo/testcdefs, which only tests -cdefs.

I've also double checked gcc.go for other code that might expect StarExpr.X to
be resolved, and didn't see anything.

That said, the type rewriting in godefsFields seems somewhat arbitrary; it just
rewrites "unsafe.Pointer" to "*byte" and "*unsafe.Pointer" to "**byte".  An even
easier fix would be to remove that type mangling in godefsFields, though I'm not
familiar enough with -godefs/-cdefs to know if there would be other adverse
consequences to that change.

[iant, 2014-08-05 20:52:03]

On 2014/08/05 19:47:23, mdempsky wrote:
> While playing around with fixes for issue 5253, I noticed a crash in "go tool
> cgo -godefs" caused by godefsFields() expecting pointer types to be completed.

> I've fixed this in patch set 8 by deferring the godefsFields() calls until
after
> all pointer types are completed, but I'm not sure how to write a test case for
> this since there don't seem to be any existing tests using "cgo -godefs"; the
> closest is misc/cgo/testcdefs, which only tests -cdefs.

I have a test for -godefs over in http://codereview.appspot.com/106260044 .  I
just noticed that Mikioh gave me a LGTM, I should probably submit that.

[iant, 2014-08-06 00:25:47]

https://codereview.appspot.com/122850043/diff/140001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/122850043/diff/140001/src/cmd/cgo/gcc.go#newco...
src/cmd/cgo/gcc.go:555: conv.Finish(pos)
The name Finish implies that the method will be called once after all type
conversions have been seen.  But the code calls it once per type name.  A better
name might be FinishType or something along those lines.

[mdempsky, 2014-08-06 00:42:41]

https://codereview.appspot.com/122850043/diff/140001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/122850043/diff/140001/src/cmd/cgo/gcc.go#newco...
src/cmd/cgo/gcc.go:555: conv.Finish(pos)
On 2014/08/06 00:25:47, iant wrote:
> The name Finish implies that the method will be called once after all type
> conversions have been seen.  But the code calls it once per type name.  A
better
> name might be FinishType or something along those lines.

Done.  I was thinking Flush or FlushTypes might be appropriate too, but went
with FinishType per your suggestion.  Happy to rename it again if you like
either of those names though.

[iant, 2014-08-06 01:13:38]

LGTM

[iant, 2014-08-06 01:17:04]

*** Submitted as https://code.google.com/p/go/source/detail?r=0015a2541545 ***

cmd/cgo: fix recursive type mapping

Instead of immediately completing pointer type mappings, add them to
a queue to allow them to be completed later.  This fixes issues	caused
by Type() returning arbitrary in-progress type mappings.

Fixes issue 8368.
Fixes issue 8441.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://codereview.appspot.com/122850043

Committer: Ian Lance Taylor <iant@golang.org>

[gobot, 2014-08-06 03:05:38]

This CL appears to have broken the linux-arm-cheney-imx6 builder.
See http://build.golang.org/log/553d41ec979e05c0c7fcb329b305cc0773916063

[dave_cheney.net, 2014-08-06 03:12:28]

I believe this is an unrelated failure.
On 6 Aug 2014 13:05, <gobot@golang.org> wrote:

> This CL appears to have broken the linux-arm-cheney-imx6 builder.
> See http://build.golang.org/log/553d41ec979e05c0c7fcb329b305cc0773916063
>
> https://codereview.appspot.com/122850043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>