Add comments and test for 64-bit field alignment #418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
January 2, 2020 15:32
Add comment about alignment requirements to all struct fields who's values are passed to 64-bit atomic operations. Update any struct's field ordering if one or more of those fields has alignment requirements to support 64-bit atomic operations.
Most `struct` that have field alignment requirements are now statically validated prior to testing. The only `struct`s not validated that have these requirements are ones defined in tests themselves where multiple `TestMain` functions would be needed to test them. Given the fields are already identified with comments specifying the alignment requirements and they are in the test themselves, this seems like an OK omission.
MrAlias
requested review from
iredelmeier,
jmacd,
lizthegrey,
paivagustavo,
rghetia and
tedsuo
as code owners
jmacd
approved these changes
Jan 4, 2020
lizthegrey
approved these changes
Jan 6, 2020
lizthegrey
added a commit
that referenced
this pull request
Jan 6, 2020
This reverts commit 91091b4.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All values that are passed to 64-bit operations in the
atomicpackage need to be arranged for 64-bit alignment:This requirement has led to opaque test failures in the past. The modification of a
structs field ordering would result in fields requiring this alignment to no longer have it. However, instead of a useful error message being presented, all the developer received was a runtime panic raised for anilpointer dereference.This PR is intended to proactively notify developers of the critical ordering of certain
structfields as well as add tests to check alignment and give useful error messages when incorrect modification is committed. To do this the code-base was cataloged and all (current) variables passed to 64-bit atomic operations were determined. Any of these variables that ended up as a field in astructwas then explicitly identified with a comment for that field, moved to the beginning of thestructto standardize on alignment, and included in a unit test. The tests added use a common set of testing utilities and validate (in the preprocessor) that the field offset is 64-bit aligned prior to any other testing being run.Important to note is what this PR is not. Ideally, the validation of variables passed to the 64-bit atomic operations would be dynamic instead of static. However, this would require a compiled Go program to parse a syntax tree (which it is a part of) and then evaluate the field offset of discovered
structs. As explained in the linked issue, this is not possible. Evaluation of constant values is possible, but the desired offset value (a variable value) is not. Therefore, these tests are static and not dynamic.This trade-off for static tests is one that is currently acceptable, but it is made with the understanding that it might not be in the future. At that future point it is expected these test will be modified or removed.
Resolves #341