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

cmd/cgo: 'packed' does not mean packed in some gcc #6149

Closed
gopherbot opened this issue Aug 14, 2013 · 17 comments

Comments

@gopherbot
Copy link

commented Aug 14, 2013

by Vova616:

What steps will reproduce the problem?
foo.c:
#include "_cgo_export.h"

void test() {
    Test(1,2,3);
}

foo.go:
package main

//void test();
import "C"
import "fmt"

//export Test
func Test(val C.int, val2 C.double, val3 C.double) {
    fmt.Println(float64(val), float64(val2), float64(val3))
}

func main() {
    C.test()
}


What is the expected output?
1 2 3

What do you see instead?
1 9.80967354e-315 5.304989477e-315

Which compiler are you using (5g, 6g, 8g, gccgo)?
8g 386

Which operating system are you using?
Windows 7 64bit

Which version are you using?  (run 'go version')
go version go1.1.2 windows/386
also
go version devel +1d0a0a267088 Wed Aug 14 22:18:49 2013 +0400 windows/386

Please provide any additional information below.

Changing (val C.int -> val C.double) fixes the problem.
"tapir" from github confirmed similar code was working fine on Arch linux
32bit but not on windows.
@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2013

Comment 1:

Labels changed: added priority-soon, cgo, removed priority-triage.

Status changed to Accepted.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Aug 15, 2013

Comment 2:

It works fine here on windows-386. I use
C:\>gcc --version
gcc (tdm-1) 4.5.2
Copyright (C) 2010 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Perhaps there is a confusion between Go and gcc about the size of C.int. I am not cgo
expert, but I would use "go -x -work ..." command to examine intermediate files to see
where the problem is. I would look for C Test function prototype generated by Go to
decide if it is right or wrong.
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

commented Aug 15, 2013

Comment 3 by Vova616:

I'm using gcc (GCC) 4.7.0, I'm not sure its just with int, it happens with
char,int,float, unsafe.Pointer and its happening only when combining with C.double I
think.
@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 15, 2013

Comment 4:

Try gcc -malign-double
@gopherbot

This comment has been minimized.

Copy link
Author

commented Aug 15, 2013

Comment 5 by Vova616:

nope -malign-double did not work, but -O changed val2 to 8.09325e-318 from
9.80967354e-315
@tapir

This comment has been minimized.

Copy link

commented Aug 16, 2013

Comment 6:

I can confirm that this bug occurs on my machine with: Windows 7 (64-bit) with Go 1.1.2
(32-bit) and GCC 4.7.2 (32-bit) whereas there is no problem with Arch 32-bit.
@tapir

This comment has been minimized.

Copy link

commented Aug 16, 2013

Comment 7:

So I'm feeling this is an issue about 32-bit Go in 64-bit Windows?
@tapir

This comment has been minimized.

Copy link

commented Aug 16, 2013

Comment 8:

I can confirm that this bug occurs on my machine with: Windows 7 (64-bit) with Go 1.1.2
(32-bit) and GCC 4.7.2 (32-bit) whereas there is no problem with Arch 32-bit.
So I'm feeling this is an issue about 32-bit Go in 64-bit Windows since it didn't happen
to Alex in Windows 32-bit?
@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 16, 2013

Comment 9:

Please describe how exactly do you build the program
@gopherbot

This comment has been minimized.

Copy link
Author

commented Aug 18, 2013

Comment 10 by Vova616:

package main
//#cgo CFLAGS: -malign-double
//void test();
import "C"
.....
is it ok?
@gopherbot

This comment has been minimized.

Copy link
Author

commented Aug 18, 2013

Comment 11 by dvyukov:

I mean the exact commands you execute to build the program.
@tapir

This comment has been minimized.

Copy link

commented Aug 19, 2013

Comment 12:

With github.com/go-gl/glfw3 we just build the package with "go build
github.com/go-gl/glfw3"
Same problem existed there but since then we've changed C.double to C.float as a
workaround.
@tapir

This comment has been minimized.

Copy link

commented Aug 22, 2013

Comment 13:

Here is a more glfw3 like presentation:
foolib.c
========
#include "_cgo_export.h"
void cFoobar() {
    goFoobar(1,2,3);
}
foolib.go
=========
package foolib
//#cgo CFLAGS: -malign-double
//void cFoobar();
import "C"
import "fmt"
//export goFoobar
func goFoobar(val C.int, val2 C.double, val3 C.double) {
    fmt.Println(float64(val), float64(val2), float64(val3))
}
func Foobar() {
    C.cFoobar()
}
runme.go
========
package main
import "foolib"
func main() {
    foolib.Foobar()
}
Command for running
===================
go run path\to\runme.go
Output
======
1 9.80967354e-315 5.304989477e-315
@dvyukov

This comment has been minimized.

Copy link
Member

commented Aug 22, 2013

Comment 14:

Can not reproduce it on Windows 64-bit with either 64-bit or 32-bit toolchain.
Please provide "go build -work -x" build log and contents of the $WORK directory.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Aug 22, 2013

Comment 15 by Vova616:

I have attached all the source/build/log files, what is your gcc version?

Attachments:

  1. go-build627162527.zip (426420 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2013

Comment 17:

Thank you for attaching that file full of intermediate objects.
Here is the C Test function:
void Test(int p0, double p1, double p2)
{
    struct {
        int p0;
        double p1;
        double p2;
    } __attribute__((packed)) a;
    a.p0 = p0;
    a.p1 = p1;
    a.p2 = p2;
    crosscall2(_cgoexp_aff9d600cead_Test, &a, 20);
}
and here is its disassembly:
0x00002060 <Test+0>:  sub    $0x3c,%esp
0x00002063 <Test+3>:  mov    0x40(%esp),%eax
0x00002067 <Test+7>:  mov    %eax,0x18(%esp)
0x0000206b <Test+11>: fldl   0x44(%esp)
0x0000206f <Test+15>: fstpl  0x20(%esp)
0x00002073 <Test+19>: fldl   0x4c(%esp)
0x00002077 <Test+23>: fstpl  0x28(%esp)
0x0000207b <Test+27>: movl   $0x14,0x8(%esp)
0x00002083 <Test+35>: lea    0x18(%esp),%eax
0x00002087 <Test+39>: mov    %eax,0x4(%esp)
0x0000208b <Test+43>: movl   $0x0,(%esp)
0x00002092 <Test+50>: call   0x2097 <Test+55>
0x00002097 <Test+55>: add    $0x3c,%esp
0x0000209a <Test+58>: ret    
0x0000209b <Test+59>: ret    
The "__attribute__((packed))" is supposed to mean there is no padding
between struct field elements, no matter what, and yet the code is
storing the 4-byte int at 0x18(%esp) but the first double at 0x20(%esp).
The 4-byte gap at 0x1c(%esp) is not supposed to be there, yet it is.
This is why Go cannot read the arguments correctly.
The best solution would be to get a better, less buggy copy of gcc.
But that might be difficult, since we don't know which versions have
the problem or even if it is an admitted problem.
Another solution might be to change cgo to stop using packed structs.
It could instead declare a struct with the arguments as scratch space
and a buffer of the right size and then memmove from the scratch space
into the buffer.
Still Go1.2Maybe, but not sure it will happen.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2013

Comment 19:

Looks like another instance of issue #5603, which is fixed on tip.  We should have put
the patch into 1.1.2, but we missed it.  Sorry about that.

Status changed to Duplicate.

Merged into issue #5603.

@rsc rsc added this to the Go1.2 milestone Apr 14, 2015
@rsc rsc removed the go1.2maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.