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: nested structure has too much alignment padding #28896

Closed
kdescoteaux opened this issue Nov 20, 2018 · 8 comments

Comments

@kdescoteaux
Copy link

commented Nov 20, 2018

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

$ go version
go version go1.11.2 windows/amd64
AND
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
set GOARCH=amd64
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows

AND

set GOARCH="amd64"
set GOHOSTARCH="amd64"
set GOHOSTOS="darwin"
set GOOS="darwin"

What did you do?

The attached file is a minimal reproducible test case for a complicated set of nested includes in our codebase. A "packed" structure that ends 'unaligned' is nested into an "unpacked" structure with a subsequent element requiring 8 byte alignment.

What did you expect to see?

I expected that only four bytes of padding would be inserted and be visible via fmt.Printf("%+v") resulting in a data structure that matched our external C code.

What did you see instead?

An extra "invisible" 8 bytes of padding are inserted breaking interoperability

main.txt

@kdescoteaux

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

Program output
`Unaligned struct requiring padding
sizes: inner = 0x10 outer = 0x20
offsets: elem2 0x8 elem3 0x18
outer dump {inner:{elem1: elem2:4294967295} _:[0 0 0 0] elem3:18446744073709551615}
outer hex dump:
00000000 00 00 00 00 00 00 00 00 ff ff ff ff 00 00 00 00 |................|
00000010 00 00 00 00 00 00 00 00 ff ff ff ff ff ff ff ff |................|

Aligned struct requiring padding
sizes: inner = 0x10 outer = 0x18
offsets: elem2 0x8 elem3 0x10
outer dump {inner:{elem1: elem2:4294967295} elem3:18446744073709551615}
outer hex dump:
00000000 00 00 00 00 00 00 00 00 ff ff ff ff 00 00 00 00 |................|
00000010 ff ff ff ff ff ff ff ff |........|
`

@ianlancetaylor ianlancetaylor changed the title CGO nested structure has too much alignment padding cmd/cgo: nested structure has too much alignment padding Nov 20, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

There is no way to represent a packed struct in Go, so the bug is that cgo is generating an incorrect struct rather than an error.

Slightly smaller test case. The two lines should have the same numbers. On amd64 this program currently prints

Go 24 16
C 16 16
package main

import (
	"fmt"
	"unsafe"
)

/*
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

#pragma pack(push, _header, 1)

// Should have size of 8 + 4 = 0xc
typedef struct {
	void * elem1;
	uint32_t elem2;
} InnerNotAligned;

// Should have size of 8 + 8 = 0x10
typedef struct {
	void * elem1;
	uint64_t elem2;
} InnerAligned;

#pragma pack(pop, _header)

// Should have size of 0xc + 4 (pad) + 8 = 0x18
typedef struct {
	InnerNotAligned inner;
	// requires ONLY 4 pad bytes to align the next element
	uint64_t elem3;
} OuterNotAligned;

// Should have size of 0x10 + 8 = 0x18
typedef struct {
	InnerAligned inner;
	// requires no padding
	uint64_t elem3;
} OuterAligned;

void printSizes() {
	printf("C %zu %zu\n", offsetof(OuterNotAligned, elem3), offsetof(OuterAligned, elem3));
}
*/
import "C"

func main() {
	var notAligned C.OuterNotAligned
	var aligned C.OuterAligned
	fmt.Printf("Go %d %d\n", unsafe.Offsetof(notAligned.elem3), unsafe.Offsetof(aligned.elem3))
	C.printSizes()
}
@gopherbot

This comment has been minimized.

Copy link

commented Nov 20, 2018

Change https://golang.org/cl/150602 mentions this issue: cmd/cgo: use field alignment when setting field offset

@kdescoteaux

This comment has been minimized.

Copy link
Author

commented Nov 21, 2018

I can't follow the gopherbot included link, but this is the issue referred to: https://go-review.googlesource.com/c/go/+/150602

@kdescoteaux

This comment has been minimized.

Copy link
Author

commented Nov 21, 2018

Can this be considered for backport to 1.11 ?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

@gopherbot Please open an issue to backport to 1.11.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 21, 2018

Backport issue(s) opened: #28916 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot closed this in fbdaa96 Nov 29, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Nov 29, 2018

Change https://golang.org/cl/151778 mentions this issue: [release-branch.go1.11] cmd/cgo: use field alignment when setting field offset

gopherbot pushed a commit that referenced this issue Dec 3, 2018
[release-branch.go1.11] cmd/cgo: use field alignment when setting fie…
…ld offset

The old code ignored the field alignment, and only looked at the field
offset: if the field offset required padding, cgo added padding. But
while that approach works for Go (at least with the gc toolchain) it
doesn't work for C code using packed structs. With a packed struct the
added padding may leave the struct at a misaligned position, and the
inserted alignment, which cgo is not considering, may introduce
additional, unexpected, padding. Padding that ignores alignment is not
a good idea when the struct is not packed, and Go structs are never
packed. So don't ignore alignment.

Updates #28896
Fixes #28916

Change-Id: Ie50ea15fa6dc35557497097be9fecfecb11efd8a
Reviewed-on: https://go-review.googlesource.com/c/150602
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit fbdaa96)
Reviewed-on: https://go-review.googlesource.com/c/151778
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.