openj9 icon indicating copy to clipboard operation
openj9 copied to clipboard

Increment stackSlotCount for structs correctly in FFI Upcall on z/OS

Open dchopra001 opened this issue 1 year ago • 11 comments

This commit fixes a bug in FFI Upcall on z/OS where the counter for stack slots in memory is not always increased when iterating through arguments. Specifically, the counter was not being incremented when structs were present in the argument list. It will now be incremented correctly for every argument.

dchopra001 avatar Oct 09 '24 15:10 dchopra001

jenkins test sanity jdk21 zlinux

r30shah avatar Oct 09 '24 21:10 r30shah

Jenkins line endings check

keithc-ca avatar Oct 10 '24 21:10 keithc-ca

@dchopra001 Can you share the build number of the hyc jenkins build for z/OS ? Given that this is z/OS specific code, I do not think, I need to launch another build (Unless @keithc-ca thinks otherwise).

r30shah avatar Oct 11 '24 14:10 r30shah

I do not think, I need to launch another build

I agree, assuming you verified that internal z/OS build was good.

keithc-ca avatar Oct 11 '24 16:10 keithc-ca

I did launch an internal build yesterday that shows UpcallThunkGen.cpp was compiled successfully. However the build didn't complete due to the following issue:

00:52:29  make[10]: *** duping jobs pipe: EDC5129I No such file or directory..  Stop.
00:52:29  make[10]: *** Waiting for unfinished jobs....
00:52:29  make[9]: *** [CMakeFiles/Makefile2:83: CMakeFiles/j9ddr.dir/all] Error 2
00:52:29  make[8]: *** [Makefile:91: all] Error 2
00:52:29  make[7]: *** [runtime/CMakeFiles/j9ddr.dir/build.make:70: runtime/CMakeFiles/j9ddr] Error 2
00:52:29  make[6]: *** [CMakeFiles/Makefile2:3502: runtime/CMakeFiles/j9ddr.dir/all] Error 2
00:52:29  make[5]: *** [CMakeFiles/Makefile2:3509: runtime/CMakeFiles/j9ddr.dir/rule] Error 2
00:52:29  make[4]: *** [Makefile:264: runtime/CMakeFiles/j9ddr.dir/rule] Error 2
00:52:29  make[3]: *** [/jenkins/workspace/Build_JDK21_s390x_zos_Nightly/closed/OpenJ9.gmk:599: build-j9] Error 2
00:52:29  make[2]: *** [/jenkins/workspace/Build_JDK21_s390x_zos_Nightly/closed/custom/Main.gmk:49: j9vm-build] Error 2

I will try launching another one and confirm when it passes.

dchopra001 avatar Oct 11 '24 17:10 dchopra001

Still seeing the same issue:

14:52:52  20 warnings generated.
14:53:00  java version "21.0.4-internal" 2024-07-16
14:53:00  IBM Semeru Runtime Certified Edition for z/OS (build 21.0.4-internal-adhoc.JENKINS.BuildJDK21s390xzosNightly)
14:53:00  IBM J9 VM (build ffi_incStackCounterForStructs_internal-8cd5fb6ea9e, JRE 21 z/OS s390x-64-Bit Compressed References 20241011_292 (JIT enabled, AOT enabled)
14:53:00  OpenJ9   - 8cd5fb6ea9e
14:53:00  OMR      - 55ddfd47ab0
14:53:00  IBM      - 3c87141
14:53:00  JCL      - 72d099bfae1 based on jdk-21.0.4+7)
14:53:39  Compiling up to 3 files for BUILD_DEMO_CodePointIM
14:53:39  Updating support/demos/image/jfc/CodePointIM/src.zip
...
...
...
...
14:55:04  Creating jdk image
14:55:31  make[3]: *** duping jobs pipe: EDC5113I Bad file descriptor..  Stop.
14:55:31  make[3]: *** Waiting for unfinished jobs....
14:55:31  make[2]: *** [/jenkins/workspace/Build_JDK21_s390x_zos_Nightly/closed/custom/Main.gmk:68: debug-image] Error 2
14:55:31  make[2]: *** Waiting for unfinished jobs....

I've launched another one. Still waiting on results.

dchopra001 avatar Oct 11 '24 20:10 dchopra001

@r30shah Build finally passed so this should be good to merge (URL: view/Nightly/job/Build_JDK21_s390x_zos_Nightly/294/)

dchopra001 avatar Oct 12 '24 01:10 dchopra001

The build referenced above (https://github.com/eclipse-openj9/openj9/pull/20322#issuecomment-2408287911) doesn't seem to have used the changes here (it consumed 8cd5fb6ea9e which I don't see in the history of this pull request); that build also didn't run any testing. Please launch a new build, making sure that it uses https://github.com/eclipse-openj9/openj9/commit/99f0667b4089a63c1f66ab018c1b0a9c0e171e0e and does FFI testing at least.

keithc-ca avatar Oct 16 '24 19:10 keithc-ca

The build referenced above (https://github.com/eclipse-openj9/openj9/pull/20322#issuecomment-2408287911) doesn't seem to have used the changes here (it consumed 8cd5fb6ea9e which I don't see in the history of this pull request); that build also didn't run any testing. Please launch a new build, making sure that it uses https://github.com/eclipse-openj9/openj9/commit/99f0667b4089a63c1f66ab018c1b0a9c0e171e0e and does FFI testing at least.

Since I launched an internal build, I was using an internal branch for testing. That's why it's not seen here. I did internal testing as well with the original fix before opening the PR and documented the results of the tests in our internal GitHub here: runtimes/openj9-openjdk-jdk21-zos/issues/423#issuecomment-93599926.

The review changes requested here weren't revolving around the problem that this PR was intending to fix. Given the nature of the changes requested, I don't expect them to cause any new failures. I have launched another build and tests. I'll update here with links once it's complete.

dchopra001 avatar Oct 17 '24 18:10 dchopra001

@dchopra001 What is the status of your internal build ?

r30shah avatar Oct 18 '24 14:10 r30shah

I'm seeing some infra related errors when trying to build the JDK on Jenkins:

Failed fetching commit message from git directory: /jenkins/workspace/Build_JDK21_s390x_zos_Nightly/.git
15:53:51  With the following error: ���z@��������@���@�����

I'll try again today until I get a successful compilation.

dchopra001 avatar Oct 18 '24 14:10 dchopra001

I see that couple of attempts from @dchopra001 have failed in getting the build, and the internal build that he had launched only misses last two changes recommended (After this comment https://github.com/eclipse-openj9/openj9/pull/20322#issuecomment-2408287911), which I think should make code more resilient with fixing Signed vs unsigned comparison.

I think these changes are safe to be merged (Only z/OS related changes), @keithc-ca if you agree with this.

r30shah avatar Oct 24 '24 14:10 r30shah

@keithc-ca Ping to see if you are OK with this one getting merged?

r30shah avatar Oct 28 '24 21:10 r30shah

I'd still like to see a build and some testing with this change.

keithc-ca avatar Oct 30 '24 20:10 keithc-ca

@keithc-ca URLs for internal Jenkins (hyc-runtimes-jenkins) jobs:

Here's the build: view/Nightly/job/Build_JDK21_s390x_zos_Nightly/319/ Here's the tests: job/Grinder_CR/25682/

The failures are unrelated and documented/explained in runtimes/openj9-openjdk-jdk21-zos/issues/423#issuecomment-93599926

dchopra001 avatar Nov 01 '24 18:11 dchopra001

The change only affects 64-bit z/OS builds - tested internally.

keithc-ca avatar Nov 05 '24 20:11 keithc-ca