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

Warnings Cleanup #20039

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Warnings Cleanup #20039

merged 5 commits into from
Sep 27, 2024

Conversation

matthewhall2
Copy link
Contributor

@matthewhall2 matthewhall2 commented Aug 22, 2024 •
edited
Loading

Suppresses warnings when building on OpenXL

Warnings:

  • uninitialized variables
  • C++ 11 ISO does not allow conversion from string literal to char *
  • unused statements
  • assignments inside if clause
  • unhandled enums in switch

R29 build: https://hyc-runtimes-jenkins.swg-devops.com/job/jvm.29.personal/34216/

@r30shah
Copy link
Contributor

r30shah commented Sep 11, 2024

jenkins test sanity all jdk11,jdk21

@r30shah
Copy link
Contributor

r30shah commented Sep 13, 2024

@matthewhall2 seems like there are some failures in the build, can you check ?

@matthewhall2 matthewhall2 force-pushed the openxl-warnings branch 2 times, most recently from a1aa063 to 9c87930 Compare September 18, 2024 13:45
@matthewhall2
Intialized uninitialized variables
3d00eb5 
Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
@r30shah
Copy link
Contributor

r30shah commented Sep 19, 2024

jenkins test sanity all jdk11,jdk21

@r30shah
Copy link
Contributor

r30shah commented Sep 20, 2024 •
edited
Loading

Looking into JDK21/JDK11 failures on s390, failures in JDK21 is not related to changes in this PR. Issue for the failures seen : #20150

@r30shah
Copy link
Contributor

r30shah commented Sep 20, 2024

Seems like x86 failures are related to #18557.

@r30shah
Copy link
Contributor

r30shah commented Sep 20, 2024

jenkins test sanity xlinux jdk11

@matthewhall2
Copy link
Contributor Author

Looking into JDK21/JDK11 failures on s390, failures in JDK21 is not related to changes in this PR. Issue for the failures seen : #20150

JDK11 failures look to be the same

keithc-ca
keithc-ca requested changes Sep 20, 2024
View reviewed changes
runtime/compiler/z/codegen/J9CodeGenerator.cpp Outdated Show resolved Hide resolved
runtime/compiler/z/codegen/J9CodeGenerator.cpp
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;
Copy link
Contributor

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).

Copy link
Contributor

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 ?

Copy link
Contributor

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.

Copy link
Contributor

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.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
@matthewhall2
Change char * to const char *
de91744 
Suppresses warnings

Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
@matthewhall2
Took out unused statement
9d2cad8 
Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
@matthewhall2
Move assignments outside of if conditions
d308044 
Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
@matthewhall2
Add default cases
80b56ff 
Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
keithc-ca
keithc-ca approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@keithc-ca keithc-ca left a 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.

@r30shah
Copy link
Contributor

r30shah commented Sep 25, 2024

jenkins test sanity all jdk11,jdk21

r30shah
r30shah approved these changes Sep 25, 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.

Will merge this once sanity test finishes

@r30shah
Copy link
Contributor

r30shah commented Sep 26, 2024

@matthewhall2 Can you checkout the failed tests and confirm if they are known issues or something new ?

@matthewhall2
Copy link
Contributor Author

matthewhall2 commented Sep 26, 2024 •
edited
Loading

@r30shah jdk21 s390x_linux failure looks to be #18521

jdk11 and 21 arch64: #20233

mac got these failures (should we re-run?)

15:44:57  update to openj9 sha: origin/pr/20039/merge
15:44:57  git fetch -q --unshallow
15:45:07  error: pathspec 'origin/pr/20039/merge' did not match any file(s) known to git
15:45:07  SHA not yet found. Continue fetching PR refs and tags...
15:45:07  git fetch -q --tags https://github.com/eclipse-openj9/openj9.git +refs/pull/*:refs/remotes/origin/pr/*
15:45:32  error: RPC failed; curl 92 HTTP/2 stream 0 was not closed cleanly: CANCEL (err 8)
15:45:32  error: 7344 bytes of body are still expected
15:45:32  fetch-pack: unexpected disconnect while reading sideband packet
15:45:32  fatal: early EOF
15:45:32  fatal: fetch-pack: invalid index-pack output

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

@r30shah
Copy link
Contributor

r30shah commented Sep 26, 2024

jenkins test sanity xmac jdk21

@r30shah
Copy link
Contributor

r30shah commented Sep 26, 2024

jenkins test sanity zlinux jdk21

@r30shah
Copy link
Contributor

r30shah commented Sep 26, 2024

Thanks @matthewhall2 , I have launched another for zLinux - Will merge this once it finishes.

@r30shah
Copy link
Contributor

r30shah commented Sep 27, 2024

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.

@r30shah r30shah merged commit 475a596 into eclipse-openj9:master Sep 27, 2024
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca approved these changes

@r30shah r30shah r30shah approved these changes

@dsouzai dsouzai Awaiting requested review from dsouzai dsouzai is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@matthewhall2 @r30shah @keithc-ca