This PR (HEAD: f243e24) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/159258 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Message from Ian Lance Taylor:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

This PR (HEAD: 1c1ebe3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/159258 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Message from Philipp Stephani:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Ian Lance Taylor:

Patch Set 2:

R=go1.13

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Emmanuel Odeke:

Patch Set 2: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 2:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=eec656d3

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 2:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/eec656d3/freebsd-amd64-12_0_2a75ce0f.log

Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 2: TryBot-Result-1

9 of 19 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/eec656d3/freebsd-amd64-12_0_2a75ce0f.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/eec656d3/openbsd-amd64-64_1ee84de1.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/eec656d3/linux-amd64-race_268ee945.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/eec656d3/linux-amd64_5135e2c2.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/eec656d3/windows-amd64-2016_ce1ce9df.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/eec656d3/linux-386_e165151d.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/eec656d3/windows-386-2008_2f3e9b7d.log
Failed on nacl-amd64p32: https://storage.googleapis.com/go-build-log/eec656d3/nacl-amd64p32_e8c12650.log
Failed on nacl-386: https://storage.googleapis.com/go-build-log/eec656d3/nacl-386_c0c714a6.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

This PR (HEAD: 71dac4a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/159258 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

This PR (HEAD: d8a401b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/159258 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Message from Philipp Stephani:

Patch Set 4:

I've moved the test to the misc/cgo directory, I hope they now pass.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Tobias Klauser:

Patch Set 4: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=50991e3d

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 4:

Build is still in progress...
This change failed on linux-386:
See https://storage.googleapis.com/go-build-log/50991e3d/linux-386_9d65ed80.log

Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 4: TryBot-Result-1

2 of 19 TryBots failed:
Failed on linux-386: https://storage.googleapis.com/go-build-log/50991e3d/linux-386_9d65ed80.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/50991e3d/windows-386-2008_3eaa6aa1.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Ian Lance Taylor:

Patch Set 4:

Trybot failures look real.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Philipp Stephani:

Patch Set 4:

Patch Set 4:

Trybot failures look real.

Yes, agreed. This is because C and Go disagree about the alignment of (u)int64 on 32-bit x86.
I'm wondering why goTypes has a fixed alignment of 8; IIUC the actual alignment on 32-bit systems is 4. Should I fix this in goTypes by using unsafe.Alignof(int64(0)) etc.?

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Ian Lance Taylor:

Patch Set 4:

Trybot failures look real.

Yes, agreed. This is because C and Go disagree about the alignment of (u)int64 on 32-bit x86.
I'm wondering why goTypes has a fixed alignment of 8; IIUC the actual alignment on 32-bit systems is 4. Should I fix this in goTypes by using unsafe.Alignof(int64(0)) etc.?

How are you deciding what go/types uses for the alignment (I didn't look at your code)? When I run this:

fmt.Println(unsafe.Alignof(int64(0)))
sizes := types.SizesFor("gc", "386")
fmt.Println(sizes.Alignof(types.Universe.Lookup("int64").Type()))

it prints 4 as I expect.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Philipp Stephani:

Patch Set 4:

Patch Set 4:

Trybot failures look real.

Yes, agreed. This is because C and Go disagree about the alignment of (u)int64 on 32-bit x86.
I'm wondering why goTypes has a fixed alignment of 8; IIUC the actual alignment on 32-bit systems is 4. Should I fix this in goTypes by using unsafe.Alignof(int64(0)) etc.?

How are you deciding what go/types uses for the alignment (I didn't look at your code)? When I run this:

fmt.Println(unsafe.Alignof(int64(0)))
sizes := types.SizesFor("gc", "386")
fmt.Println(sizes.Alignof(types.Universe.Lookup("int64").Type()))

it prints 4 as I expect.

What I mean are the alignment settings in the goTypes map in https://go.googlesource.com/go/+/refs/tags/go1.12.1/src/cmd/cgo/out.go#1375. They set the alignment to 8 unconditionally for the 64-bit types. I'm not quite sure what the Size and Align members of these map values refer to; should they match the actual size and alignment on the target architecture?

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Ian Lance Taylor:

Patch Set 4:

Trybot failures look real.

Yes, agreed. This is because C and Go disagree about the alignment of (u)int64 on 32-bit x86.
I'm wondering why goTypes has a fixed alignment of 8; IIUC the actual alignment on 32-bit systems is 4. Should I fix this in goTypes by using unsafe.Alignof(int64(0)) etc.?

How are you deciding what go/types uses for the alignment (I didn't look at your code)? When I run this:

fmt.Println(unsafe.Alignof(int64(0)))
sizes := types.SizesFor("gc", "386")
fmt.Println(sizes.Alignof(types.Universe.Lookup("int64").Type()))

it prints 4 as I expect.

What I mean are the alignment settings in the goTypes map in https://go.googlesource.com/go/+/refs/tags/go1.12.1/src/cmd/cgo/out.go#1375. They set the alignment to 8 unconditionally for the 64-bit types. I'm not quite sure what the Size and Align members of these map values refer to; should they match the actual size and alignment on the target architecture?

The only place where that table is used, (*Package).cgoType, adjusts the alignment so that it does not exceed the target pointer size. That is supposed to make the result match what the compiler does.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

This PR (HEAD: 546a2cc) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/159258 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Message from Philipp Stephani:

Patch Set 4:

Patch Set 4:

Trybot failures look real.

Yes, agreed. This is because C and Go disagree about the alignment of (u)int64 on 32-bit x86.
I'm wondering why goTypes has a fixed alignment of 8; IIUC the actual alignment on 32-bit systems is 4. Should I fix this in goTypes by using unsafe.Alignof(int64(0)) etc.?

How are you deciding what go/types uses for the alignment (I didn't look at your code)? When I run this:

fmt.Println(unsafe.Alignof(int64(0)))
sizes := types.SizesFor("gc", "386")
fmt.Println(sizes.Alignof(types.Universe.Lookup("int64").Type()))

it prints 4 as I expect.

What I mean are the alignment settings in the goTypes map in https://go.googlesource.com/go/+/refs/tags/go1.12.1/src/cmd/cgo/out.go#1375. They set the alignment to 8 unconditionally for the 64-bit types. I'm not quite sure what the Size and Align members of these map values refer to; should they match the actual size and alignment on the target architecture?

The only place where that table is used, (*Package).cgoType, adjusts the alignment so that it does not exceed the target pointer size. That is supposed to make the result match what the compiler does.

OK, I've pushed a change making a similar adjustment. (Though I guess in the long run it'd be better to use the go/types package to reduce duplication.)

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Ian Lance Taylor:

Patch Set 5: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 5:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=977bd5d7

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Gobot Gobot:

Patch Set 5: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

Message from Ian Lance Taylor:

Patch Set 5: Code-Review+2

Thanks.

RELNOTES=yes

Please don’t reply on this GitHub thread. Visit golang.org/cl/159258.
After addressing review feedback, remember to publish your drafts!

This PR is being closed because golang.org/cl/159258 has been merged.