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

proposal: cmd/compile: make 64-bit fields be 64-bit aligned on 32-bit systems, add //go:packed directive on structs #36606

Open
danscales opened this issue Jan 16, 2020 · 48 comments

Comments

@danscales
Copy link

@danscales danscales commented Jan 16, 2020

Summary: We propose to change the default layout of structs on 32-bit systems such that 64-bit fields will be 64-bit aligned. The layout of structs on 64-bit systems will not change. For compatibility reasons (and finer control of struct layout), we also propose the addition of a //go:packed directive that applies to struct types. When the //go:packed directive is specified immediately before a struct type, then that struct will have a fully-packed layout, where fields are placed in order with no padding between them and hence no alignment based on types. The developer must explicitly add padding to enforce any desired alignment.

The main goal of this change is to avoid the bugs that frequently happen on 32-bit systems, where a developer wants to be able to do 64-bit operations on a 64-bit field (such as an atomic operation), but gets an alignment error because the field is not 64-bit aligned. With the current struct layout rules, a developer must often add explicit padding in order to make sure that such a 64-bit field is on a 64-bit boundary. As shown by repeated mentions in issue #599 (18 in 2019 alone), developers still often run into this problem. They may only run into it late in the development cycle as they are testing on 32-bit architectures, or when they execute an uncommon code path that requires the alignment.

As an example, the struct for ticks in runtime/runtime.go is declared as

var ticks struct {
	lock mutex
	pad  uint32 // ensure 8-byte alignment of val on 386
	val  uint64
}

so that the val field is properly aligned on 32-bit architectures. With the change in this proposal, it could be declared as:

var ticks struct {
	lock mutex
	val  uint64
}

and the val field would always be 64-bit aligned, even if other fields are added to struct.

We do not propose changing the alignment of 64-bit types which are arguments or return values, since that would change the Go calling ABI on 32-bit architectures. For consistency, we also don't propose to change the alignment of 64-bit types which are local variables. However, since we are changing the layout of some structs, we could be changing the ABI for functions that take a struct as an argument or return value. However, if assembly code is accessing struct fields, it should be using the symbolic constants (giving the offset of each field in a struct) that are available in go_asm.h (which is automatically generated for any package which contains an assembly file).

However, we do require a new directive for compatibility in the case of cgo and also interactions with the operating system. We propose the addition of a //go:packed directive that applies to struct types. When the //go:packed directive is specified immediately before a struct type, then that struct will have a fully-packed layout, where fields are placed in order with no padding between them and hence no alignment based on types. The developer must explicitly add padding fields to enforce any desired alignment within the structure.

We would then use the //go:packed directive for cgo-generated struct types (with explicit padding added as needed). Also, given the strict layout requirements for structures passed to system calls, we would use //go:packed for structs that are used for system calls (notably Flock_t and Stat_t, used by Linux system calls). We can enforce the use of //go:packed for structures that are passed to system calls (typically via pointers) with the use of a go vet check. I don't think we would particularly encourage the use of //go:packed in any other situations (as with most directives), but it might be useful in a few specific other cases.

This proposal is essentially a solution to issue #599. A related issue #19057 proposes a way to force the overall alignment of structures. That proposal does not propose changing the default alignment of 64-bit fields on 32-bit architectures (the source of the problems mentioned in issue #599), but would provide a mechanism to explicitly align 64-bit fields without using developer-computed padding. With that proposal, aligning a uint64 field (for example) in a struct to a 64-bit boundary on a 32-bit system would require replacing the uint64 type with a struct type that has the required 64-bit alignment (as I understand it).

Compatibility: We are not changing the alignment of arguments, return variables, or local variables. Since we would be changing the default layout of structs, we could affect some programs running on 32-bit systems that depend on the layout of structs. However, the layout of structs is not explicitly defined in the Go language spec, except for than minimum alignment, and we are maintaining the previous minimum alignments. So, I don't believe this change breaks the Go 1 compatibility promise. If assembly code is accessing struct fields, it should be using the symbolic constants (giving the offset of each field in a struct) that are available in go_asm.h. go_asm.h is automatically generated and available for each package that contains an assembler file, or can be explicitly generated for use elsewhere via go tool compile -asmhdr go_asm.h ....

@gopherbot gopherbot added this to the Proposal milestone Jan 16, 2020
@gopherbot gopherbot added the Proposal label Jan 16, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 16, 2020

If we are going to consider adding a go:packed magic comment that affects code generation, then I think we should also consider a magic comment that permits specifying the desired alignment of a type. That will permit people to align types to a wider boundary than would otherwise be used, which permits using vectorization writting in assembler or C. See #19057.

@josharian
Copy link
Contributor

@josharian josharian commented Jan 17, 2020

We expect that any assembler functions that depend on the layout of a struct argument or return value should be using unsafe.Offsetof to determine field offsets within a struct.

Does the assembler accept unsafe.Offsetof for FP offsets? I don’t recall ever seeing that in assembly code.

The good news is that vet checks these offsets, so breakage should be caught as a matter of course. (It’ll mean teaching go/types about the annotations, but that would need to happen anyway.)

Or do I misunderstand?

@josharian
Copy link
Contributor

@josharian josharian commented Jan 17, 2020

During the discussion of changing the Go ABI, several people expressed the opinion that if we're going to break a bunch of assembly code, we'd rather do that only once. I share that opinion.

Making this change and then an ABI change would mean breaking Go assembly twice.

I'd hate to gate this proposal on figuring out the ABI changes. What if instead, as a prerequisite for this change, we teach the Go assembler some way to express things like "first argument" or "offset of field F within struct S". Then we could update the assembly once to use these higher level constructs, and if we change the ABI later, if we've done our job well, no further assembly changes would be necessary. It'd also lower the cost of ABI experimentation.

@danscales
Copy link
Author

@danscales danscales commented Jan 17, 2020

We expect that any assembler functions that depend on the layout of a struct argument or return value should be using unsafe.Offsetof to determine field offsets within a struct.

Does the assembler accept unsafe.Offsetof for FP offsets? I don’t recall ever seeing that in assembly code.

I was thinking that you could have a Go init routine that stores the relevant values of unsafe.OffsetOf in some global variables that are accessed by the assembler routine. Also, Ian pointed out to me that in many situations you can use 'go tool compile -asmhdr go_asm.h ...' on the Go file that defines the type to get the relevant offsets written out to a go_asm.h file. I can clarify this in the proposal.

The good news is that vet checks these offsets, so breakage should be caught as a matter of course. (It’ll mean teaching go/types about the annotations, but that would need to happen anyway.)

Or do I misunderstand?

I don't really understand what you mean by 'vet checks these offsets' in this case. What vet rule is this? Or are you talking about the suggested new vet checks that I mention in the proposal?

Making this change and then an ABI change would mean breaking Go assembly twice.

As I understand it, the goal with introducing a new ABI would be to use it in Go code mainly, and not break existing assembly code (which would stay with the original Go ABI, referenced as ABI0 in
Austin's changes).

I do agree that this proposal could break some existing assembly code. I'm guessing that it might be quite minimal (but that may definitely be wrong), and certainly possibly breaking some assembler code is relevant to deciding about the benefits/drawbacks of this proposal.

@josharian
Copy link
Contributor

@josharian josharian commented Jan 17, 2020

I can clarify this in the proposal.

I think that that would be helpful.

I don't really understand what you mean by 'vet checks these offsets' in this case.

I was thinking of https://github.com/golang/tools/blob/master/go/analysis/passes/asmdecl/asmdecl.go

@beoran
Copy link

@beoran beoran commented Jan 20, 2020

I hope that whatever change there is to the ABI or to struct layouts, that it does not make interfacing with the OS or with C code any harder than it is now. At best the ABI and struct layout stays as it is and any special directives only affect that when used, not by default.

The only reasons I consider worth while of to change the Go ABI would be to make C calls faster by getting rid of the segmented stack, for the rest I'm not sure what the benefits would be.

@rsc rsc added this to Incoming in Proposals Jan 22, 2020
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 22, 2020

The other day I proposed adding a zero-width type (like unsafe.Packed) that could be embedded instead of a magic comment. (We do something similar with sync/cond.go:type noCopy struct{}.)

@rsc
Copy link
Contributor

@rsc rsc commented Jan 22, 2020

Note that it is critical that this proposal include a vet check to ensure that all places where we hand data structures to C/syscall/asm/DLL get marked as "packed". That gives us the benefit, after going through the work of marking all those places (helped by vet), of letting the compiler rearrange fields in unmarked structs, such as to fill in alignment-induced gaps with upcoming fields. (We've talked about doing this for a long time, but we held back because we didn't want to break compatibility with C/etc. If we're not compatible anyway, then we should make sure to get a benefit. Today, we rearrange fields to fill in gaps today by hand, which is not great.)

The exact annotation for how to mark "this struct is packed" is less important than the semantic change. We should probably decide "do we want to do this at all" first.

The alignment note that Ian suggested is another semantic change. That suggestion is to mark a specific type as having a specific alignment (anywhere a value of that type is represented in memory). That may or may not be separable from this issue, but if we think we want to do it at some point, it would be good to choose a marking scheme for packed/not that matches an eventual marking scheme for per-type overall alignment.

@rsc rsc moved this from Incoming to Active in Proposals Jan 22, 2020
@beoran
Copy link

@beoran beoran commented Jan 23, 2020

I think that perhaps, it's already too late to make this change, as there are hundreds if not thousands of Go projects out there that use C/syscall/asm/DLL with the assumption that the struct layout in Go is as it is today, without automatic rearrangements. While what we have now isn't all too great, it does work in practice, and I know that if I am careful, then I can manually lay out the Go struct in such a way that the kernel/C code/DLL can use it.

As an aside, while it is true that in this kind of low level situations, instead of a struct I could use a []byte in conjunction with "encoding/binary", this is not a very satisfactory solution due to the overhead of the function calls and the lack of convenience.

While letting the compiler automatically rearrange fields in a struct will save on some memory space, I don't see that as a benefit that is worth breaking millions of lines of working code over. Making such a change now truly will lead to an incompatible Go v2, which risks the same problem as python 3 had.

Rather than making such changes, I would specify and document how one can manually lay out a struct efficiently, and then make a tool that can rearrange structs to be more compact according to this documentation. Furthermore, I would be in favor of changes which make laying out a struct manually more easy or more precise.

Furthermore, a "go vet" check alone is not enough to fix this, we would also need a "go fix" for this so this can be fixed automatically. However, I suspect that either will be difficult to implement because exported structs can be used freely everywhere, so it is hard to see if a struct is involved in a situation where the compiler should not rearrange it.

I do agree that this issue is not separate from other alignment and packing issues. If changes are to be made, they should all be made together, to make the transition somewhat easier.

@aclements
Copy link
Member

@aclements aclements commented Jan 24, 2020

During the discussion of changing the Go ABI, several people expressed the opinion that if we're going to break a bunch of assembly code, we'd rather do that only once. I share that opinion.

Dan's change doesn't affect argument layouts, so no FP offsets would change in assembly code. The prototype implementation actually separates the concerns of struct layout from argument frame layout in order to keep this working. Since we have the ABI0/ABIInternal split, we could in principle use struct-style layout for Go calls while still keeping the existing alignment rules for assembly calls (though I don't know if that would be worthwhile).

The one place that it does potentially affect assembly code is if the assembly hard-codes offsets into Go structures. Our hope is that any assembly that is using struct offsets is just including go_asm.h and using the symbolic values already created by the compiler. (You don't even have to use "go tool compile -asmhdr go_asm.h" like Dan mentioned above; the go tool does this automatically.)

@aclements
Copy link
Member

@aclements aclements commented Jan 24, 2020

A few points that have been made in in-person discussions about this proposal:

  • Some architectures will trap on unaligned access. We can either say that's up to the user to avoid, or have the compiler emit the necessary code to avoid the unaligned access. GCC, for example, will load unaligned fields one byte at a time if it has to.

  • @dr2chase pointed out one reason you may need both a "packed" annotation and an "alignment" annotation on the same struct. If "packed" has the effect of making all fields alignment 1, then the whole struct has alignment 1 and you have no control over what happens if that struct is embedded in another type. In particular, you may be laying out a type carefully so that some fields are aligned, but that alignment may be broken when the type is put in a larger type. However, this can be handled if you can specify both that the struct is packed and explicitly specify its overall alignment.

  • @mknyszek pointed out that the garbage collector requires that pointers be word-aligned. It would be unfortunate to say "you can't have pointers in a packed struct", so I think pointers need to be word-aligned even in packed structs (which also affects overall struct alignment). We can either make this happen silently, or we can say it's an error if a pointer in a packed struct doesn't already fall on a word boundary.

@aclements
Copy link
Member

@aclements aclements commented Jan 24, 2020

I think that perhaps, it's already too late to make this change, as there are hundreds if not thousands of Go projects out there that use C/syscall/asm/DLL with the assumption that the struct layout in Go is as it is today, without automatic rearrangements.

I personally think this is a significant overestimate, but of course, I don't have any data to back up that claim either. If we implement a vet check, we could at least automatically scan a large corpus and get a better sense.

While letting the compiler automatically rearrange fields in a struct will save on some memory space, I don't see that as a benefit that is worth breaking millions of lines of working code over. ... Rather than making such changes, I would specify and document how one can manually lay out a struct efficiently ...

Note that we're not currently proposing field rearrangement. This proposal isn't about saving memory at all; it's about addressing a persistent source of run-time crashes in configurations that are often poorly tested.

Furthermore, a "go vet" check alone is not enough to fix this, we would also need a "go fix" for this so this can be fixed automatically.

Yes, we've actually been talking about this. It probably wouldn't be a "go fix" just because "go fix" is built to make as few assumptions about the code as possible, but it could be a new flag to "go vet", since "go vet" can do the sorts of type and flow analysis that you want for this. We've been thinking about doing this for some other vet checks as well that have clear automatic fixes.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 24, 2020

@beoran

I think that perhaps, it's already too late to make this change, as there are hundreds if not thousands of Go projects out there that use C/syscall/asm/DLL with the assumption that the struct layout in Go is as it is today, without automatic rearrangements. While what we have now isn't all too great, it does work in practice, and I know that if I am careful, then I can manually lay out the Go struct in such a way that the kernel/C code/DLL can use it.

Two things.

  1. It does not actually work in practice all the time. Most of the time, yes. But not all the time. Cgo silently skips over fields that are aligned in ways it cannot express to Go, which is a persistent low-rate source of problems. Same for the lack of 64-bit alignment when using 64-bit atomics on 32-bit systems. The point of the annotation is to allow fixing the latter, and it also fixes the former.

  2. All the existing code can be grandfathered in. The go.mod file lets us identify which version of Go code expects, and if we do any rearrangement, we would almost certainly disable it for code written against older versions of Go.

@beoran
Copy link

@beoran beoran commented Jan 24, 2020

Ok, if I limit myself to just this proposal, as opposed to other related ABI changes, then I have to admit that a //go:packed annotation, or similar functionality by itself, is useful to make it easier to interact with C/DLL/kernel. I didn't know about the limitation of cgo, so if this annotation could fix that, then that would be great.

I'm more worried about the other part of this proposal to change the alignment, as this breaks the ABI of go. If this change is disabled for old code though the go statement in the go.mod file, then that would be sufficient to keep old code working. Of course the "old" ABI must then be maintained "forever", or at least, say 10 years, and must be interoperable with the new ABI.

And I am glad to hear that automatic fixing using go vet is being considered.

With the safeguards of go module versioning and go vet in place, and if properly planned, it might be possible to make ABI changes to go without breaking existing code. Still, I'd prefer that, like generics, the issue of changing the go ABI be well thought out, without precipitation, so we can get it right, and hopefully only have to do it once every 10 years or less.

@aclements
Copy link
Member

@aclements aclements commented Jan 25, 2020

@mknyszek pointed out to me that I completely misunderstood one of the concerns with the ABI: while Dan's proposing to maintain the current behavior for aligning the starts of arguments (which means argument frame and struct layout may differ, unlike right now), if you actually pass a struct by value to an assembly function, then it may be affected by an alignment change in that struct.

Does this actually happen in the wild? I'm not sure. It would require 32-bit assembly that takes a struct argument whose layout actually changes. vet would be able to detect it even without a new analysis, so we could try running it on a large corpus to get a sense.

@mknyszek proposed that we could create an ABI wrapper to relayout such structs in the arguments/results to assembly functions. This is kind of a pain, but is certainly doable. If we go down this path, maybe we also make ABIInternal Go functions continue to take a struct-style argument frame.

Still, I'd prefer that, like generics, the issue of changing the go ABI be well thought out

We have been thinking very carefully about how to change the ABI (it's not as flashy as generics :). We already have the system in place (modulo a few rough edges) to maintain strict calling convention compatibility with existing assembly code, even if we change the calling convention in Go code. This is what I was referring to with an "ABI wrapper" above.

@beoran
Copy link

@beoran beoran commented Jan 25, 2020

@aclements Great, I fully agree with the idea at keeping the current ABI as ABI0. Good to see that there is a plan. However, I am missing something in there. It only mentioned assembly.

However, there is also Cgo, DLL calls and direct OS calls to keep in mind, like I do with my Linux input library https://gitlab.com/beoran/galago/blob/master/os/linux/input_linux.go. I would like thought to be given also to these use cases and preferably mention them also in that document.

Edit: I reread the document and cgo is mentioned, but I feel more though should be given to it and the use cases I mentioned above.

@danscales
Copy link
Author

@danscales danscales commented Jan 26, 2020

@beoran: just to get back to the immediate proposal (which only changes the alignment of 64-bit fields on 32-bit systems), it would definitely be helpful if you can send pointers to any of your code (or any other code that you know of) that will be immediately impacted. That is, if you have assembly language code that hard-codes offsets of fields in structs. Or if you have structs (other than cgo, which we plan to handle automatically) that must match existing syscall/dll/C structures, and will no longer match because of the alignment change. Similar to the Flock_t and Stat_t structs we mentioned above, any other such structures would potentially need to use //go:packed and explicit padding to keep the exact alignment desired for syscalls/dlls or other C API (but that is not using cgo).

@danscales
Copy link
Author

@danscales danscales commented Jan 26, 2020

@mknyszek proposed that we could create an ABI wrapper to relayout such structs in the arguments/results to assembly functions. This is kind of a pain, but is certainly doable. If we go down this path, maybe we also make ABIInternal Go functions continue to take a struct-style argument frame.

@aclements: I'm not sure if I'm understanding, but are you saying we could convert an incoming struct from the new layout back to the Go 1.13 layout via the wrapper, so that the existing assembly doesn't notice the field offset change? I suppose that is possible, but remember that the assembly routine might be taking a pointer to the struct, or access the struct via a chain of multiple pointers. So, it will certainly never be possible to ensure that an assembly routine that hard-codes field offsets will remain compatible with the struct alignment changes.

@beoran
Copy link

@beoran beoran commented Jan 27, 2020

@danscales As I mentioned before, the code of my "galago" Linux input system does not use CGo, but direct Linux kernel calls to read joysticks, etc. The code is currently written such that, like GCC does for the kernel, 64 bits fields are aligned on 32 bits boundaries on 32 bits systems. Go structs currenly seems to agree mostly with C structs as GCC produces them, which is convenient.

So I think my code is going to be affected, though I will have to test it first. See here for the code "galago" packages:

https://gitlab.com/beoran/galago/tree/master/os/linux

@danscales
Copy link
Author

@danscales danscales commented Jan 28, 2020

@beoran In your galago code, I see very little use of uint64/int64/float64, and even less for 32-bit systems. In that case, the only 64-bit type in use that I see is Time_t, which is int64 even for 32-bit system. 'Long' however looks to be int32 on a 32-bit system. So, there could possibly be a change in alignment only because of the use of Time_t, but the only use of Time_t is in Timeval, where it is the first field. So, the offset of the fields in this struct would not change on 32-bit systems, though the overall required alignment of the structure would increase from 32-bit to 64-bit alignment. I don't think this should affect any interaction with the OS, but let me know if you foresee some issue.

We will keep your case in mind, and please do send any other pointers to code that might require code changes with this proposal.

@beoran
Copy link

@beoran beoran commented Jan 28, 2020

@danscales Yes, I can see that in my specific case of direct Linux kernel calls there may not be a problem because the struct layout is such that it should be OK. Perhaps the Linux developers did put the 64 bits members of the struct first on purpose, but I'm not sure that this is true for all direct Linux system calls. There are probably thousands of structs involved in hundreds of system calls, and I'm unsure if all are so well-behaved, so that is certainly something we need to keep in mind.

@hajimehoshi 's Ebiten is also likely to be affected on Windows, because Ebiten's code does make direct DLL calls without using cgo using syscall, to avoid the performance overhead of CGO. Some of the structs in that code are filled with 32 bits integers. For example: https://github.com/hajimehoshi/ebiten/blob/master/internal/glfw/glfw_windows.go.

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Jan 28, 2020

Not sure yet another "great idea" is welcome, but perhaps we can specify field ordering and type alignment separately. Before I go too far with this, I ran into a problem, this might be a common problem, so I'll lead with that.


IF type alignment is specified separately, there will be a problem with conversions from pointer-to-less-aligned-thing to pointer-to-more-aligned-thing. Pointer conversion currently is unchecked -- if the compiler allows it, it happens. If we allow such pointers (and we might want them for atomics, if we like static checking) then we need an answer for this. I think the least-surprise option would be to enhance the semantics of a plain pointer cast to include an alignment check where this would be needed. The advantage there is that old and new code would interoperate; the disadvantage is that an operation that previously could not panic, now can panic.

Returning to the presentation of the great idea...

The two magic comments would be

//go:align(<somevalue>)
//go:orderedfields

Here, <somevalue> specifies the byte alignment required for a type, and is one of

  • a power of two (not sure how large these should be allowed; can be higher or lower than default, subject to the compiler whining about lack of a load instruction.)
  • ptr to specify the native pointer alignment
  • the name of a language (C, Java, Fortran, ???) to specify the native alignment of the corresponding type in the other programming language, with an error if we believe the other language has no such corresponding type (Fortran has complex, Java does not).
  • Possibly "*" preceding an alignment to indicate a pointer to an aligned quantity; I am not convinced this is necessary, because a pointer to an aligned type can inherit the alignment. If this is allowed, there can be multiple alignment attributes for both the pointer itself and the referent.

Thus

type cacheLine       [64]uint64 //go:align(64)
type atomicComplex64 complex64  //go:align(8)
type floatPtrPun     float64    //go:align(ptr)

type cacheLinePtr *cacheLine // implicitly, a pointer to something that is aligned
type alignedPtrToAligned8Uint8 *[8]uint8 // go:align(8,*8)

and

type INPUT_keymap_entry struct { //go:orderedfields
	Flags uint8; 
	Len uint8;
	Index uint16; 
	Keycode uint32;
	Scancode [32]uint8;
};

and if you are not a trusting soul or wish to be explicit about your expectations

type INPUT_keymap_entry struct { //go:orderedfields
	Flags uint8;         //go:align(1)
	Len uint8;           //go:align(1)
	Index uint16;        //go:align(2)
	Keycode uint32;      //go:align(4)
	Scancode [32]uint8;  //go:align(1)
};

I expect the following uses to be common:

  • simple type, with alignment, gets that alignment wherever it is allocated, and is part of minimum alignment of any containing struct. Within an otherwise undecorated struct, field may be reordered to satisfy whims of compiler writers.
  • structures for interchange are ordered, if there's no ambiguity about field alignments (this is up to convention and expectations of code authors, vet might help define "ambiguity") then they don't need alignment annotations. But useless alignment annotations are fine.
  • Some types have non-negotiable alignment restrictions and it is a compile-time error to break those.

There is a problem with conversions from pointer-to-less-aligned-thing to pointer-to-more-aligned-thing. Pointer conversion currently is unchecked -- if the compiler allows it, it happens. If we allow such pointers (and we might want them for atomics, if we like static checking) then we need an answer for this. I think the least-surprise option would be to enhance the semantics of a plain pointer cast to include an alignment check where this would be needed. The advantage there is that old and new code would interoperate; the disadvantage is that an operation that previously could not panic, now can panic.

We could provide a tool that would pre-annotate structs/types for easy compatibility into the future, and the annotations are backwards compatible in the sense that old code, annotated, then runs in an old system, would behave as it always did. However, old code, annotated, might not run in a new system because of referent alignment issues in pointer conversions.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 28, 2020

Can you give an example where Go would permit a conversion from pointer to a less-aligned thing to a pointer to a more-aligned thing without using unsafe?

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Jan 28, 2020

Statically, it would be unsafe. Dynamically, we could check. Maybe we call it unsafe for now, wait to see if anyone ever needs to do it. It wouldn't surprise me if "old" code mechanically ported forward would trip over this (in the static case).

Example, using the alignment annotation:

type T struct {
   x int64 // current default alignment for int64 is 32 bits, right?
}
type ptrAtomicInt64 *int //go:align(*8) // this is a pointer to an 8-byte-aligned 64-bit int.
...
v := new(T) // GC allocates on 8-byte boundary, so v.x is 8-byte aligned
pax := ptrAtomicInt64(&v.x) // statically dubious, a dynamic check would succeed
@beoran
Copy link

@beoran beoran commented Jan 28, 2020

What happens when a pointer to a 32 bit aligned struct is stored in an interface`, which presumably is 64 bits aligned internally on 32 bits architecture?

type Foo struct {
        Field int32
}
var foo = &Foo { 7 }
var ifa interface{} = foo
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 9, 2020

Change https://golang.org/cl/237317 mentions this issue: design/36606-64-bit-field-alignment: add proposal

gopherbot pushed a commit to golang/proposal that referenced this issue Jun 12, 2020
This change adds a new design proposal to the repository describing
changes to Go alignment behavior and addition of Go directives, as a way
to deal with the problems associated with issue golang/go#599.

Updates golang/go#36606

Change-Id: I2b09424a71d2ffed7136205756f6e101aafd13bb
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/237317
Reviewed-by: Austin Clements <austin@google.com>
@danscales
Copy link
Author

@danscales danscales commented Jun 12, 2020

I've finally submitted a full proposal in the design repot at https://go-review.googlesource.com/c/proposal/+/237317/3/design/36606-64-bit-field-alignment.md
(sorry for the delay).

I welcome any comments here about that proposal. With respect to controlling alignment in structs (especially for the new packed structs), we haven't yet reached consensus on the best syntax: using a new //go:align directive vs. adding new special types runtime.AlignN (as described in issue #19057. I have listed the pluses and minuses for each of the approaches in the proposal.

For this proposal, I also mention that we could sidestep the above issue about how to express alignment by removing explicit control of alignment from the proposal, and instead have a separate rule to establish the overall alignment of packed structs.

I'm happy to hear comments on any of the alternatives. We're not planning to proceed until we get good agreement on the design.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 10, 2020

Change https://golang.org/cl/254057 mentions this issue: runtime: align 12-byte objects to 8 bytes on 32-bit systems

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 11, 2020

I don't see any direct mention in https://github.com/golang/proposal/blob/master/design/36606-64-bit-field-alignment.md about how the compiler should handle pointers to fields within packed structs.

For example:

func f(p *int) { *p = 42 }

//go:packed
type T struct { b byte; i int }

func g() {
    var x T
    f(&x.i)
}

Does this code compile? If it does, is it guaranteed to run successfully? Or can it panic or otherwise fail unsafely?

My reading of:

The compiler will give a compile-time error if the fields of a packed struct are aligned incorrectly for garbage collection or hardware-specific needs. In particular, it will be an error if a pointer field is not aligned to a 4-byte boundary. It may also be an error, depending on the hardware, if 16-bit fields are not aligned to 2-byte boundaries or 32-bit fields are not aligned to 4-byte boundaries.

is that the code may not compile on all target platforms; but that on platforms where it does compile, it's expected to work. I.e., that f needs to be compiled to handle p pointing to a packed int field too.

As a small tweak, I'd suggest instead reserving the right for the compiler to give a compile-time error for any //go:packed structure, with pointers and 16/32-bit fields listed as examples of reasons it might reject them. E.g., if there's a 64-bit type that requires 64-bit alignment, the compiler should be allowed to reject that too.

@danscales
Copy link
Author

@danscales danscales commented Sep 12, 2020

@mdempsky Your reading of that paragraph is correct. For platforms where struct T compiles, f must be able to deal with a pointer to a packed int that may not be naturally aligned. The idea is that the compiler can reject any packed struct that will be noticeably inefficient (or cause a segv, etc.) for any particular architecture. For simplicity, I'm assuming the compiler would also reject T if it would have to generate extra code to deference these unaligned pointers on the platform, even if that code would be nearly as efficient. So, there should be no change to f in order to deal with these pointers. I can try to make this somewhat clearer.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 1, 2020

Looks like this needs to be removed from "Hold".

@rsc rsc moved this from Hold to Active in Proposals Oct 1, 2020
@rsc rsc removed the Proposal-Hold label Oct 1, 2020
gopherbot pushed a commit that referenced this issue Oct 1, 2020
Currently on 32-bit systems 8-byte fields in a struct have an alignment
of 4 bytes, which means that atomic instructions may fault. This issue
is tracked in #36606.

Our current workaround is to allocate memory and put any such atomically
accessed fields at the beginning of the object. This workaround fails
because the tiny allocator might not align the object right. This case
specifically only happens with 12-byte objects because a type's size is
rounded up to its alignment. So if e.g. we have a type like:

type obj struct {
    a uint64
    b byte
}

then its size will be 12 bytes, because "a" will require a 4 byte
alignment. This argument may be extended to all objects of size 9-15
bytes.

So, make this workaround work by specifically aligning such objects to 8
bytes on 32-bit systems. This change leaves a TODO to remove the code
once #36606 gets resolved. It also adds a test which will presumably no
longer be necessary (the compiler should enforce the right alignment)
when it gets resolved as well.

Fixes #37262.

Change-Id: I3a34e5b014b3c37ed2e5e75e62d71d8640aa42bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/254057
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Michael Knyszek <mknyszek@google.com>
@rsc
Copy link
Contributor

@rsc rsc commented Oct 7, 2020

Adding this back to the proposal minutes after a long time on hold.
Now that the full design doc is in, are there any objections to moving forward with this?

@danscales
Copy link
Author

@danscales danscales commented Oct 7, 2020

@rsc , it may be a bit obscured in the proposal , but we never did come to a consensus on what syntax we should use to insure alignment of various fields. Each choice had a number of pros and cons. See 2nd paragraph of the comment above. Do you have a suggestion on how to proceed?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 7, 2020

As far as syntax, we continue to add //go: comments, including the new //go:build and //go:embed comments that were recently accepted as proposals. With those and //go:packed, it seems like //go:align is the obvious choice for alignment rather than introduce unusual syntax for this one directive.

I would support any of these as consensus outcomes:

  1. Do //go:packed, where alignment of struct is max alignment of any field. No //go:align at all.
  2. Choice (1), adding //go:align for struct type declarations.
  3. Choice (2), adding //go:align for fields.

It seems like the most conservative route would be to start with (1), which I thought was the proposal on the table, and go from there, since doing so does not cut off (2) and (3) later.

(One way to paint ourselves into a corner would be to say that //go:packed means the overall struct alignment is always 1, but I think that's clearly a mistake as a default, so let's not do that.)

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 7, 2020

I'd rather we un-hold proposal #19057. Rationale:

  1. These two proposals appear to address the same set of issues (atomics on 32-bit CPUs, fancy vector instructions). Except #19057 addresses it just through alignment control (which only affects Go code on an opt-in basis), whereas this proposal adds alignment control and default changes to alignment and structure packing (which have consequences for all Go code).

    As the use cases are somewhat niche (atomics are discouraged in favor of higher-level synchronization primitives; 32-bit CPUs are increasingly less common; vector instructions are a specialized use case), I'd rather we focus first on just the alignment control aspect. We can always add the other two changes later if alignment control proves insufficient.

  2. I strongly dislike using //go: directives to affect types. Except for //go:notinheap (which is an internal implementation detail used only by the runtime and cgo), every other directive either affects a function declaration (//go:noinline, //go:norace, etc) or is just package global (//go:linkname). The //go:embed proposal extends that to variables, which are still a declared object. These are all directives affecting a statement.

    Types though are described by expressions, and type declarations merely bind names to types. I'm concerned about the consequences if a magic comment on type T1 T2 means it's no longer safe to convert between *T1 and *T2.

    The alignment and packing directives are also visible through unsafe.Alignof/unsafe.Sizeof/unsafe.Offsetof, which can be used in static assert statements. The justification for using comments for other changes to the Go language have been that they don't change the semantics of the language, and a compiler could ignore them. That's not true with this proposal.

    Directives are also a very course-grained kludge. E.g., we don't have a way to apply directives to particular fields. In a struct with a bunch of ints where only some of them are accessed atomically, #19057 allows selectively aligning the fields that actually need stricter alignment.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2020

@mdempsky, it's pretty late in the process to suggest we do something completely different.
Dan has put together a good proposal with help from many people.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 14, 2020

@rsc This is a proposal that changes the Go programming language semantics, and at least in the past, I've been a reviewer of Go programming language spec changes. But no one pinged me about the change, internally or externally. As far as I can tell, I didn't learn about this proposal until Sep 11, when I asked my first clarifying question above. (I've spent quite some time now trying to reconstruct how I even found the issue on Sep 11, and I'm still unsure. My current best hypothesis is I just happened to spot it in the proposals repo while looking for an unrelated proposal.)

When I did ask that, the proposal was in "Hold" status. I commented my technical concerns within a week of it being returned back to "Active" when you specifically asked for concerns: "are there any objections to moving forward with this?" Why did you ask that question if you're going to dismiss when people answer it?

Finally, as far as I can tell, there's no clear, explicit consensus in favor of this approach over #19057. E.g., @ianlancetaylor expressed strong opposition to type annotations in #19057 (comment), which I'd assume extend to this proposal. For lack of a more formal voting process (which I think Go's proposal process would benefit from), I'll note that #19057 currently has 17 "like" reactions, while this proposal currently has none.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2020

In terms of the list in #36606 (comment), I thought the design doc when I took it off hold was proposing choice (1): add //go:packed, let the alignment of a struct be the max alignment of any field in the struct, don't have any override for alignment of fields, don't have any override for alignment of overall structs, and do start aligning 64-bit fields on 64-bit boundaries even on 32-bit systems.

To start aligning 64-bit fields on 64-bit boundaries even on 32-bit systems - which is clearly the right default - we need some way to override that decision, to say "pack these together" for cgo and other low-level uses. #19057 does not give that control. It only addresses the things that choice (1) explicitly omits for now. That is, choice (1) of this proposal and #19057 are completely orthogonal.

Should we proceed with choice (1), meaning: //go:packed by itself (no //go:align) and aligning 64-bit fields properly on 32-bit systems

?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 14, 2020

Was previously on hold for design doc, and I was late taking it off hold for that.
But now placing back on hold per discussion with @danscales.
The focus of the compiler/runtime team right now is on figuring out how to implement generics, so they don't have bandwidth for this.

@rsc rsc moved this from Active to Hold in Proposals Oct 14, 2020
@aclements
Copy link
Member

@aclements aclements commented Oct 15, 2020

We discussed this a bit in a brainstorming session today because @mknyszek has been running into lots of atomic alignment issues with his runtime metrics changes.

This exploration all started (now long ago) when @danscales was running into alignment issues and I asked him to explore what it would take to change the default alignment of 64-bit fields on 32-bit. While I absolutely agree that that seems like the ideal thing to do, we learned through this process that it has a lot of unfortunate consequences and complexity. It's not at all clear to me that the benefits of the default alignment change pay for this complexity, which suggests to me that may in fact be the wrong path to take.

The alternative, #19057 goes down the other path by providing a simple tool to fix alignment issues in the very rare cases where it's necessary, without trying to change any defaults. And it's quite simple in comparison ("provide types with zero size and various fixed alignments"). The only complexity with this seems to be that the language doesn't specify struct field order, so it doesn't technically have to align the following field, but there are various ways to solve that that are all perfectly fine.

Since this is causing a lot of pain for @mknyszek , we floated the idea of experimenting with these magic alignment types just in the runtime to get a feel for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.