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.

Sign up for GitHub

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

Jump to bottom

sync: need a way to guarantee that WaitGroup.state1 fields are always 4-bytes aligned #27577

Closed
go101 opened this issue Sep 9, 2018 · 6 comments
Closed

sync: need a way to guarantee that WaitGroup.state1 fields are always 4-bytes aligned #27577

go101 opened this issue Sep 9, 2018 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@go101
Copy link

go101 commented Sep 9, 2018 •
edited

I have noticed and am glad that the suggestion mentioned in #19149 is adopted. But I feel the current new implementation is still not perfect.

The current implementation assumes that the WaitGroup.state1 field is always 4-bytes aligned, but I can't find any such guarantee is written down in any official Go documentation.

So I suggest that we should add a method like the following for *WaitGroup. The method will never be called, it just gives compilers a hint that the WaitGroup.state1 field needs to be 4-bytes aligned.

func (wg *WaitGroup) _() {
	_ = atomic.LoadUint32(&wg.state1[0])
}
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 9, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 9, 2018
@ianlancetaylor
Copy link
Contributor

I don't mind adding a compiler hint but I would prefer it to be one that the compiler might actually recognize. I think it's unlikely that the compiler will ever recognize your suggestion.

Note that it's more or less OK for the standard library to assume behavior that is not guaranteed, because if the behavior changes, the standard library can change at the same time. This is not true for packages outside the standard library.

@randall77
Copy link
Contributor

WaitGroup.state1 is a [3]uint32. It will be 4-byte aligned on all platforms we support.
It's not guaranteed as we don't talk about alignment in the spec. But we align all Go data types to their natural alignment up to a platform-specific maximum alignment, which is at least 4 on all platforms. It is very likely to remain at least 4 because pointers need to be aligned (to avoid shearing pointers).

@go101
Copy link
Author

go101 commented Sep 9, 2018

I understand the facts. I just want to make it perfect in theory. :)
Consider there is a third-party compiler which is neither gc or gccgo, ...

@CAFxX
Copy link
Contributor

CAFxX commented Sep 18, 2018

Can't this be solved simply by adding a test that will make all.bash fail if the field doesn't have the expected alignment?

@ianlancetaylor
Copy link
Contributor

@CAFxX A sync.WaitGroup can appear in user code that all.bash will never see, but it still has to be aligned correctly.

@guevara guevara mentioned this issue Mar 26, 2019
Open
@bcmills bcmills mentioned this issue Jan 28, 2022
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@go101
Copy link
Author

go101 commented Oct 21, 2022

Closed, for WaitGroup aligns fields by using atomic.Uint64 /

@go101 go101 closed this as completed Oct 21, 2022
@golang golang locked and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@CAFxX @ianlancetaylor @randall77 @gopherbot @go101