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: reject C structs with Go-incompatible field alignment #7560

Closed
gopherbot opened this issue Mar 17, 2014 · 14 comments
Closed

cmd/cgo: reject C structs with Go-incompatible field alignment #7560

gopherbot opened this issue Mar 17, 2014 · 14 comments
Milestone

Comments

@gopherbot
Copy link
Contributor

by dturnbull:

go version go1.2.1 darwin/amd64
Apple LLVM version 5.0 (clang-500.2.79)

Using this program:

package main
/*
#include "stdlib.h"
#include "stdint.h"
#include "complex.h"
typedef struct {
    uint32_t unalign;
    float complex osc;
} mixer;
mixer* Mixer() {
    mixer *o = malloc(sizeof(mixer));
    o->osc = (3.0 + 5.0*I);
    return o;
}
*/
import "C"
import "fmt"
func main() {
    o := C.Mixer()
    fmt.Println(o)
}

I get incorrect output:
&{0 (5+0i)}

Removing the unalign element or changing to a double complex gives the expected output:
&{(3+5i)}
@ianlancetaylor
Copy link
Contributor

Comment 1:

Labels changed: added repo-main, release-go1.3.

@minux
Copy link
Member

minux commented Mar 27, 2014

Comment 2:

that particular C struct mixer can't be represented in Go at all.
complex float in C has 4-byte alignment (because it is treated
as struct { float real; float imag; }; however in Go, complex64
has 8-byte alignment.
If C has bigger alignment than Go, we can always adding padding,
but in this case, that won't help.
Either we support some kind of __attribute__((pack)) in Go (highly
unlikely to happen), or change float64 alignment to 4-byte (but
probably this is not allowed)
Of course, another option is to forbid this kind of struct in cmd/cgo.
What do you think? Ian?

Status changed to Accepted.

@ianlancetaylor
Copy link
Contributor

Comment 3:

I think you're right.  We should make cgo give an error for this case.  We don't want to
give an error for an unrepresentable struct in the C code, but we do want to give an
error for an attempt to use that struct in the Go code.

Labels changed: added release-go1.3maybe, removed release-go1.3.

@gopherbot
Copy link
Contributor Author

Comment 4 by dturnbull:

Will all complex64 elements in C structs be inaccessible from Go? Or just elements that
don't align?

@ianlancetaylor
Copy link
Contributor

Comment 5:

I'm not aware of any problem with complex64 fields at all.
There is a problem with struct fields that are not aligned the same way that Go aligns
them.  There are a number of ways this can happen in C, you just encountered one of them.

@gopherbot
Copy link
Contributor Author

Comment 6 by dturnbull:

I may not understand why go aligns the way it does, but I get what is happening. What
concerns me is how this will be handled in future versions of Go. If Go will forbid
complex64 in C structs by way of compilation error then my application will stop
working. If I understand better what this error will be then I can start writing
compatible code today.
I've attached a more complete example so you can see exactly what I'm doing. The
workaround I'm using is to put all the complex64 numbers at the start of the C struct.
This Mixer example is only one of dozens that is implemented in this pattern.
You may wonder why I use this unusual pattern. The process() function needs to run in
real-time on a 700MHz ARM. Using C lets me run this on an OS thread to bypass the range
checking and GC in Go. Moving to complex128 is a 20% performance hit on ARM (but
insignificant on amd64).

Attachments:

  1. Go-Issue7560.zip (5233 bytes)

@minux
Copy link
Member

minux commented Mar 28, 2014

Comment 7:

you can use this gcc-ism to force 8-byte alignment for your complex float in C
to be compatible with Go:
__attribute__((align(8)) complex float x;
(you can also use anonymous union to achieve the same effect if you don't want
to use gcc attributes, but you will have to make sure to choose another type that
has a alignment of 8-byte.)
for example, define your mixer struct this way:
typedef struct {
    uint32_t unalign;
    __attribute__((align(8))) complex float osc;
} mixer;
This will waste 4 byte of memory, so if you control the struct layout, you'd better
just align complex float to 8-byte boundary manually;
I will add something along this line to the cgo article on go-wiki for this workaround.

@ianlancetaylor
Copy link
Contributor

Comment 8:

Go is not going to forbid complex64 in structs.  As I said before, complex64 is not the
problem.
I think that Go should forbid referring directly to C structs with fields whose
alignment can not be represented in Go.  There are many ways to handle this case, the
most obvious being, don't use a struct.  Or, for the particular special case of a
misaligned complex64 field, use [2]float32.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2014

Comment 9:

Labels changed: added release-go1.3, removed release-go1.3maybe.

@gopherbot
Copy link
Contributor Author

Comment 10:

CL https://golang.org/cl/96300045 mentions this issue.

@rsc
Copy link
Contributor

rsc commented May 13, 2014

Comment 11:

This issue was closed by revision 2d1a951.

Status changed to Fixed.

@ianlancetaylor
Copy link
Contributor

Comment 12:

Issue #8110 has been merged into this issue.

@gopherbot
Copy link
Contributor Author

Comment 13 by fuzxxl:

This change looks like it could introduce unpredictable platform dependencies into cgo
code: It looks like this change only hides the struct field when it cannot be
represented on the machine you're compiling on right now. It might be entirely possible
that other platforms have different alignment constraints so that the field is exposed
on one platform as it is aligned but on a different platform it is omitted because that
platform has different alignment constraints causing the field to be misaligned.
Perhaps a more predictable solution has to be devised.

@ianlancetaylor
Copy link
Contributor

Comment 14:

Suggestions welcome, but please either on the mailing list or in a different issue, not
here.
Platform dependencies in cgo are in general unavoidable.

@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 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
Development

No branches or pull requests

4 participants