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

Fix fieldAlignment check for AtomicLong on Z #20323

Merged
merged 1 commit into from
Oct 21, 2024
Merged

Fix fieldAlignment check for AtomicLong on Z #20323

merged 1 commit into from
Oct 21, 2024

Conversation

matthewhall2
Copy link
Contributor

@matthewhall2 matthewhall2 commented Oct 9, 2024 •
edited
Loading

"value" field for in AtomicLong is of type "long" and therefore should be aligned to 8-byte boundary.

checkFieldAlignmentForAtomicLong was incorrectly checking that the offset of the field is a multiple of 4 instead of a multiple of 8.

This PR fixes that.

addresses #20235

@matthewhall2 matthewhall2 force-pushed the atomiclongoffset branch 2 times, most recently from 59b0ff4 to 8d91013 Compare October 11, 2024 14:42
@matthewhall2 matthewhall2 marked this pull request as ready for review October 16, 2024 17:53
@r30shah
Copy link
Contributor

r30shah commented Oct 17, 2024

@matthewhall2 as this one is fixing/ closing that issue, please use appropriate commit message. See https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md#example-commits

r30shah
r30shah requested changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change itself looks good to me. Please update the commit message and body with some useful information.

@matthewhall2 matthewhall2 force-pushed the atomiclongoffset branch 2 times, most recently from 7aa7c9c to a56a16a Compare October 17, 2024 18:03
@matthewhall2 matthewhall2 requested a review from r30shah October 17, 2024 18:04
@matthewhall2
Copy link
Contributor Author

Change itself looks good to me. Please update the commit message and body with some useful information.

done

@matthewhall2 matthewhall2 force-pushed the atomiclongoffset branch 2 times, most recently from c5bab58 to 13216d5 Compare October 17, 2024 18:06
@r30shah
Copy link
Contributor

r30shah commented Oct 17, 2024 •
edited
Loading

Please follow up on #20323 (comment), as your change is closing the issue you mentioned, please have description with Closes: ...

r30shah
r30shah requested changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use following commit title and the body.

Fix fieldAlignment check for AtomicLong on Z

"value" field in AtomicLong is of type "long" and should be
aligned to 8-byte boundary.
Check field alignment for AtomicLong was incorrectly checking if the
offset of the field is aligned to 4 byte boundary instead of 8.
This commit fixes that.

Closes: https://github.com/eclipse-openj9/openj9/issues/20235

Please also update PR title and description, also if you are using the above commit message, please ensure the body width to 72.

Tag me once you make above changes, I will launch sanity changes.

@matthewhall2 matthewhall2 changed the title Change offset check to 0x7 Fix fieldAlignment check for AtomicLong on Z Oct 18, 2024
@matthewhall2 matthewhall2 requested a review from r30shah October 18, 2024 21:05
r30shah
r30shah requested changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"value" field for in AtomicLong is of type "long" and therefore should be aligned to 8-byte boundary.

Seems like a minor grammatical error value field in AtomicLong...

@matthewhall2
Fix fieldAlignment check for AtomicLong on Z
fc510d4 
"value" field in AtomicLong is of type "long" and therefore should
be aligned to 8-byte boundary.
checkFieldAlignmentForAtomicLong was incorrectly checking that the
offset of the field is a multiple of 4 instead of a multiple of 8.
This commit fixes that.

Closes: eclipse-openj9#20235

Signed-off-by: Matthew Hall <[email protected]>
@matthewhall2 matthewhall2 requested a review from r30shah October 18, 2024 21:10
r30shah
r30shah approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@r30shah
Copy link
Contributor

r30shah commented Oct 18, 2024

jenkins test sanity zlinux jdk11,jdk21

@r30shah
Copy link
Contributor

r30shah commented Oct 21, 2024

The failure seen in JDK11 is same as #14948, launching JDK11 test again.

@r30shah
Copy link
Contributor

r30shah commented Oct 21, 2024

jenkins test sanity zlinux jdk11

@r30shah
Copy link
Contributor

r30shah commented Oct 21, 2024

All test passed,merging.

@r30shah r30shah merged commit abe8b1b into eclipse-openj9:master Oct 21, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthewhall2 @r30shah