Skip to content

documentation: Go internal abi specification document alignment confusion #53223

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

Closed
hawkinsw opened this issue Jun 3, 2022 · 5 comments
Closed
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@hawkinsw
Copy link
Contributor

hawkinsw commented Jun 3, 2022

Hello everyone!

I hope that this is the right place to raise this issue:

I have read through the "Go internal ABI specification" (src/cmd/compile/internal-abi.md) and have studied the excellent, thorough discussion of the definition of the alignment of sequences (of which a struct is a special case) and the alignment of built-in types.

I am curious, however, how that squares with the following statement

On ARM, 386, and 32-bit MIPS, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically. The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.

from https://pkg.go.dev/sync/atomic#pkg-note-BUG.

Nothing in the "Go internal ABI specification" seems to indicate that there is a special case for these architectures. I was just wondering if this is is because the representation made by https://pkg.go.dev/sync/atomic#pkg-note-BUG concerns the "public" ABI and would supersede the definitions/algorithms in the internal ABI in this case?

Or, is it simply something that is just out of date with a (admittedly internal) piece of documentation. If it is, I would be glad to submit a patch for the "Go internal ABI specification" document to bring it up to date.

That document is such a helpful, interesting read for so many reasons and one of the main reasons why I really enjoy working with this language!

Thanks for everything!
Will

cc @danscales

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Jun 3, 2022
@dmitshur
Copy link
Member

dmitshur commented Jun 3, 2022

CC @golang/runtime.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 3, 2022
@dmitshur dmitshur added this to the Backlog milestone Jun 3, 2022
@ianlancetaylor
Copy link
Contributor

The abi-internal.md document says that on 32-bit architectures the alignment of uint64 and int64 is 4. The sync package docs mention that on "ARM, 386, and 32-bit MIPS," all of which are 32-bit systems, the programmer is responsible for aligning 8-byte values at a alignment of 8. There is no inconsistency.

In general you will get better and faster answers to questions on a forum rather than the issue tracker. See https://go.dev/wiki/Questions.

Closing this issue because there doesn't seem to be a bug here.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Jun 4, 2022
@hawkinsw
Copy link
Contributor Author

hawkinsw commented Jun 4, 2022

Thank you, @ianlancetaylor, for the response! You were kind enough to chat with me before my presentation at GopherCon 2017 -- an awesome experience.

The abi-internal.md document says that on 32-bit architectures the alignment of uint64 and int64 is 4. The sync package docs mention that on "ARM, 386, and 32-bit MIPS," all of which are 32-bit systems, the programmer is responsible for aligning 8-byte values at a alignment of 8. There is no inconsistency.

I completely, 100% agree with what you just said. What I cannot grok is how the second sentence is compatible with the first and the abi-internal.md document. In particular,

The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.

Despite the lack of a clear definition of "allocated struct, array, or slice" in the language reference, I now better understand that means the storage of the variable exists on the heap and I can verify in runtime.mallocgc the blocks allocated are properly aligned. But, what about the "first word in a variable"? According to the ARM documentation, a word is 4 bytes (and I can find no go-specific definition of word in either the Language specification or the memory model) so I would assume that a (u)int64 would contain 2 words. So, if I have a variable of type (u)int64 on a 32-bit machine, to me (the uninformed reader) the sync package document implies the first four bytes of that variable would be 8-byte aligned. And yet, as you rightly point out, the abi-internal.md document does not guarantee that at all!

I am so sorry to reopen this issue and extend the documentation, but I clearly don't understand something!

Thank you for your time!
Will

@hawkinsw
Copy link
Contributor Author

hawkinsw commented Jun 4, 2022

@ianlancetaylor @bradfitz I now understand!! After digging through the commit log for the doc.go file in src/sync/atomic/, I found the explanatory statement detailing why there is no conflict. From fea7c43, @aclements says

Local variables can also be relied on the [sic] be 64-bit aligned, since they will be escaped to the heap if used with any atomic operations.

Would you entertain a PR where that information is added to the "BUGS" section of the src/sync/atomic/doc.go? I believe it would help tremendously! As usual, thank you all for your help!

Will

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410102 mentions this issue: sync/atomic: clarify that 8-byte alignment of variables is due to escape

gopherbot pushed a commit that referenced this issue Jun 7, 2022
For #53223.

Change-Id: I79e9b920488581a4d850e4051ee0dd600b5bbcb1
Reviewed-on: https://go-review.googlesource.com/c/go/+/410102
Reviewed-by: Michael Pratt <[email protected]>
Reviewed-by: Austin Clements <[email protected]>
@golang golang locked and limited conversation to collaborators Jun 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants