-
Notifications
You must be signed in to change notification settings - Fork 737
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
Warnings Cleanup #20039
Warnings Cleanup #20039
Conversation
jenkins test sanity all jdk11,jdk21 |
@matthewhall2 seems like there are some failures in the build, can you check ? |
a1aa063
to
9c87930
Compare
Signed-off-by: Matthew Hall <[email protected]>
9c87930
to
b95c575
Compare
jenkins test sanity all jdk11,jdk21 |
Looking into JDK21/JDK11 failures on s390, failures in JDK21 is not related to changes in this PR. Issue for the failures seen : #20150 |
Seems like x86 failures are related to #18557. |
jenkins test sanity xlinux jdk11 |
JDK11 failures look to be the same |
b95c575
to
802ee2e
Compare
int32_t fieldNameLen = 5; | ||
char * fieldSig = "J"; | ||
const char * fieldSig = "J"; | ||
int32_t fieldSigLen = 1; | ||
int32_t intOrBoolOffset = self()->fe()->getObjectHeaderSizeInBytes() + self()->fej9()->getInstanceFieldOffset(classBlock, fieldName, fieldNameLen, fieldSig, fieldSigLen); | ||
return (intOrBoolOffset & 0x3) == 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the name of the function, I suspect 0x3
ought to be 0x07
(that the offset is a multiple of 8).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part of code needs some investigation - Looking at the places where we would use this query will be while inlining method from AtomicLong
and the value (confirmed with sig
is long), Sincename intOrBoolOffset
sounds contradictory to the name of the function, and change this as a separate PR if Keith you are OK with it, I would like @matthewhall2 to verify and fix this as a separate PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keithc-ca ping to see what you think about above ? As I said what you pointed out is correct, but given that the this code is there since long I think a verification from @matthewhall2 is warranted on places where this query is used, so I think this can be done as a separate change (As original purpose of this PR from @matthewhall2 was to clean-up some of the warnings we saw with OpenXL compiles).
@matthewhall2 can open up the issue and get started on it to keep track of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it makes sense to investigate and make any necessary changes for that mask separately.
Suppresses warnings Signed-off-by: Matthew Hall <[email protected]>
Signed-off-by: Matthew Hall <[email protected]>
Signed-off-by: Matthew Hall <[email protected]>
Signed-off-by: Matthew Hall <[email protected]>
802ee2e
to
80b56ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving on the assumption that the offset mask discussed in #20039 (comment) will be addressed separately.
jenkins test sanity all jdk11,jdk21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge this once sanity test finishes
@matthewhall2 Can you checkout the failed tests and confirm if they are known issues or something new ? |
@r30shah jdk21 s390x_linux failure looks to be #18521 jdk11 and 21 arch64: #20233 mac got these failures (should we re-run?)
windows is not showing any failures and all 3 of build, sanity functional, and sanity openjdk have no errors, not sure why its showings as failed |
jenkins test sanity xmac jdk21 |
jenkins test sanity zlinux jdk21 |
Thanks @matthewhall2 , I have launched another for zLinux - Will merge this once it finishes. |
Failure seen on s390 was same as #19618, I am merging this one as satisfied with the passed tests so far and I do not expect the changes in this PR to cause issue with the targets we could not get passing due to existing issues. |
Suppresses warnings when building on OpenXL
Warnings:
R29 build: https://hyc-runtimes-jenkins.swg-devops.com/job/jvm.29.personal/34216/