Increment stackSlotCount for structs correctly in FFI Upcall on z/OS #20322
Conversation
dchopra001
force-pushed
the
ffi_incStackCounterForStructs
branch
from
1cd7257
to
4632a14
Compare
|
jenkins test sanity jdk21 zlinux |
dchopra001
force-pushed
the
ffi_incStackCounterForStructs
branch
from
4632a14
to
cd423b0
Compare
dchopra001
force-pushed
the
ffi_incStackCounterForStructs
branch
from
cd423b0
to
fc65fff
Compare
|
Jenkins line endings check |
|
@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). |
I agree, assuming you verified that internal z/OS build was good. |
|
I did launch an internal build yesterday that shows I will try launching another one and confirm when it passes. |
|
Still seeing the same issue: I've launched another one. Still waiting on results. |
|
@r30shah Build finally passed so this should be good to merge (URL: view/Nightly/job/Build_JDK21_s390x_zos_Nightly/294/) |
There was a problem hiding this comment.
I am good with this getting merged. Just seeing that @keithc-ca still need to approve this change, I will wait for him to approve this.
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. Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
dchopra001
force-pushed
the
ffi_incStackCounterForStructs
branch
from
6fdda30
to
99f0667
Compare
|
The build referenced above (#20322 (comment)) 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 99f0667 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: 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 What is the status of your internal build ? |
|
I'm seeing some infra related errors when trying to build the JDK on Jenkins: I'll try again today until I get a successful compilation. |
|
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 #20322 (comment)), 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. |
|
@keithc-ca Ping to see if you are OK with this one getting merged? |
|
I'd still like to see a build and some testing with this change. |
|
@keithc-ca URLs for internal Jenkins (hyc-runtimes-jenkins) jobs: Here's the build: The failures are unrelated and documented/explained in runtimes/openj9-openjdk-jdk21-zos/issues/423#issuecomment-93599926 |
|
The change only affects 64-bit z/OS builds - tested internally. |
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.