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
Closed

cmd/cgo: nested structure has too much alignment padding #28896

kdescoteaux opened this issue Nov 20, 2018 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@kdescoteaux
Copy link

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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

@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Nov 21, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Nov 21, 2018
@kdescoteaux
Copy link
Author

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

@kdescoteaux
Copy link
Author

Can this be considered for backport to 1.11 ?

@ianlancetaylor
Copy link
Contributor

@gopherbot Please open an issue to backport to 1.11.

@gopherbot
Copy link

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
Copy link

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
…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>
@golang golang locked and limited conversation to collaborators Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants