openj9
openj9 copied to clipboard
eclipse-openj9
Add thunk/adapter for FFI Upcall on Z
Marked as a draft because I need to confirm that a failure in the builds is not related to my changes, and some formatting changes need to be made.
This commit should pass all up call tests found in @ChengJin01's branch: https://github.com/ChengJin01/openj9/find/jep389_419_ffi_upcall_v4_struct_v2_outofline_mh_v9_fin_v8
under the test/functional/Java18andUp/src/org/openj9/test/jep419/upcall/ directory.
Note however that the Memory Padding in UpcallMHWithStructTests.java is not compatible on zLinux. Removing the padding on zLinux should resolve the problem. Just leaving this as a note here.
I also did my own unit testing as I was building the solution to make sure I'm covering all cases. On zLinux the parameter can be stored in one of two call frames depending on the situation. So there are some unique tests with 10+ arguments to test these scenarios. I've put those tests here. The readme describes the test cases. @ChengJin01 if you think any of these tests would be worth adding to your existing tests then let me know and I can provide you with the necessary files.
@joransiu any concerns/review with this change? Since @dchopra001 have tested the changes with different test case, and the sanity test passes, should we attempt merging this one?
@joransiu any concerns/review with this change? Since @dchopra001 have tested the changes with different test case, and the sanity test passes, should we attempt merging this one?
I would like to see if @dchopra001 will get a chance to address the few minor outstanding comments, and squash the commits.
@zl-wang / @ChengJin01 Do we have any specific tests we should trigger to exercises these paths?
@joransiu, @dchopra001 should be able to verify this PR locally with his own tests specific to Z at https://github.com/dchopra001/ffiUpCallTests (he already verified with my test suites locally with my branch). In terms of generic tests suites for upcall, our test cases at https://github.com/eclipse-openj9/openj9/pull/15310 have not yet merged to the repo as we have not yet finished the code review for all PRs listed at https://github.com/eclipse-openj9/openj9/issues/15068. So there is no way to directly verify with these test suites for the moment due to the incomplete code in the repo.
@dchopra001 & @joransiu, could this PR be merged if there is no more concern to be addressed?
@ChengJin01 What is the timeline within which we need to get this change in?
@r30shah, we should have sufficient time for this PR but I'd like to have it merged as soon as possible if we could. So I will need to double-check with jtreg tests to see whether there is any platform specific issue to be addressed on zLinux.
I did test the last time I pushed code changes to this branch a week or two ago. Just to be sure I'm doing a clean build and will test again. I'll update here once the build is done and the tests are clear.
Not sure why the Copyright check and Line Ending checks suddenly flagged issues in other files, and shows conflicts on merge. @dchopra001 , can you squash and rebase?
@zl-wang / @r30shah : Any other comments/feedback?
@AdamBrousseau, is there any update in terms of the Copyright & Line Endings Check ? It seems the failing lines listed in there totally have nothing to do with this PR.
@ChengJin01 the checks don't work properly when there are merge conflicts in the PR. Once they are resolved, the check should return proper results.
@dchopra001, please double-check whether there is any conflict to be resolved in this PR.
@zl-wang / @r30shah : Any other comments/feedback?
Waiting for @dchopra001 's comment for verifying testing. Overall, changes looks good to me.