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

unsafe: Sizeof documentation should be more explicit #40322

Open
eric-s-raymond opened this issue Jul 21, 2020 · 13 comments
Open

unsafe: Sizeof documentation should be more explicit #40322

eric-s-raymond opened this issue Jul 21, 2020 · 13 comments
Labels
Documentation help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@eric-s-raymond
Copy link

This is a documentation issue. Therefore I have not attempted to use the standard template.

In C, sizeof() applied to a type actually returns its stride length, e.g. including any trailing padding required so that the start address of each element in an array of the type has legal alignment. The difference can be significant for certain layouts of structs.

In Go it appears that this may not the case. Quoting: "the size in bytes of a hypothetical variable v as if v was declared via var v = x". This is unclear, because a compiler might choose to allocate storage for a series of unrelated single variables in such a way that structs have no trailing padding and yet each variable is properly self-aligned (e.g. in the case of, say, a byte-valued variable tucked in where the struct padding might be).

This is a request for clarification. The documentation should either (a) affirm that unsafe.Sizeof does not return a stride length, making it subtly unlike C sizeof(), (b) affirm that it does return a stride length, or (c) state that whether it does so or not is outside the scope of the Go specification.

@ianlancetaylor ianlancetaylor changed the title unsafe.Sizeof documentation should be more explicit. unsafe: Sizeof documentation should be more explicit Jul 21, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Go1.16 Jul 21, 2020
@ianlancetaylor
Copy link
Contributor

unsafe.Sizeof does indeed include trailing padding much as the C sizeof operator does.

I agree that the docs should be improved.

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 21, 2020
@nasirhm
Copy link

nasirhm commented Jul 21, 2020

I would like to work on it

@ianlancetaylor

@ianlancetaylor
Copy link
Contributor

Go ahead. Thanks.

@AndrewWPhillips
Copy link

AndrewWPhillips commented Jul 21, 2020

I don't think the documentation for unsafe.Sizeof() requires any changes, except maybe to reference where the Go spec mentions how fields of a struct are aligned and (if nec.) padded. (And yes, Go structs may have zero or more padding byte(s) after any field of a struct including the last one, exactly as in C.) In other words the doc. for Sizeof should merely say that it returns the number of bytes the compiler allocates for a variable (which it does); the spec should say how the memory is allocated (which it does).

@eric-s-raymond
Copy link
Author

Andrew, if that is the approach that is taken I would appreciate it if the Sizeof documentation included a direct link to where the specification describes trailing padding.

However, my point is that the phrase "the number of bytes the compiler allocates for a variable" is not fine-grained enough. It doesn't tell you under what circumstances the instance will have internal or trailing padding making it larger than is obvious. As a C programmer I can consider Go's specification of self-alignment and be pretty confident that I know the answer to this question when the argument of sizeof() is part of a larger array or struct aggregate, but that doesn't settle the singleton case.

@AndrewWPhillips
Copy link

Thanks for the follow-up Eric. You are right that a bit more clarity in the doc would be good, but I don't think the Go spec should include any details of how padding is done. This is deliberately unspecified so as not to limit future changes/optimizations (in fact Go [unlike C] does not even specify that the field order is preserved!) For example, see https://dave.cheney.net/2015/10/09/padding-is-hard for how padding (and hence Sizeof) changed between Go 1.4 and 1.5.

This is not to say that more control of the layout of structs in Go is not a good idea - see #10014.

Note that padding is also not specified in C - the C standard only says something like "there may be padding bytes added between fields or at the end of a struct but not at the beginning". Admittedly many C implementations document how the layout is done and implement #pragma pack, command line options, etc which allow you to control padding - but this is not part of the C standard (the last time I looked).

BTW I am still not sure what problem you are trying to avoid. Your code should not be depending on the size or layout of a structure in memory - eg. when writing structs to persistent storage they should not be written as unencoded binary data but encoded with something like GOB. Re-reading your comments it seems that you may be concerned that a single instance of a type has a different size than an element of the same type in an array - I think Go (and C) guarantee this but maybe it needs to be explicitly stated in the Go spec.

You may also be interested in a blog I wrote about this Alignment and Pragma Pack. This was for C
but some of the ideas can also be applied to Go. (Luckily some of thenasty problems that I described, like misaligned fields when linking, are not possible in Go.)

@eric-s-raymond
Copy link
Author

Andrew: No, I'm not trying to do pointer arithmetic. :-)

In reposurgeon I have a command called "sizeof" that displays the result of unsafe.Sizeof() applied to various core types. The purpose of the command is to provide feedback to reposurgeon developers trying to reduce working set by reordering members to squash out padding. Yes, this can actually matter on very large repositories!

In the documentation of my sizeof command I wanted to be able to tell my devs whether they should assume they are seeing stride lengths or not.

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Feb 6, 2021
@mdempsky
Copy link
Member

unsafe.Sizeof does indeed include trailing padding much as the C sizeof operator does.

With cmd/compile and gccgo, it does; but with go/types, it does not: https://play.golang.org/p/PaKQwyXwYJi

/cc @griesemer @findleyr

@griesemer
Copy link
Contributor

griesemer commented Apr 29, 2021

The go/types and types2 unsafe.Sizeof implementation doesn't match the compilers, but it is also configurable. If this is an issue, the respective types.Config.Sizes object can be provided.

@mdempsky
Copy link
Member

Ack on the default sizes implementation working as intended and possible to override. But if we update the documentation for unsafe.Sizeof to say it includes trailing padding (as this issue suggests above), that will make go/types's default implementation non-conforming.

So I think it's only correct to update the documentation if we also intend to update go/types's default implementation.

@mdempsky mdempsky added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Apr 29, 2021
@ianlancetaylor
Copy link
Contributor

We should update the spec and update go/types.

@dmitshur
Copy link
Contributor

@ianlancetaylor Should that definitely happen for Go 1.17? If so, let's mark this a release blocker, or otherwise move to Backlog.

@ianlancetaylor
Copy link
Contributor

Moving to Backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants