openj9
openj9 copied to clipboard
eclipse-openj9
[FFI] Fix the issue with the struct FF/DD in upcall on z/OS
[FFI] Fix the issue with the struct FF/DD in upcall on z/OS
The changes resolve the issue with the register mapping for the struct FF/DD or its variants in upcall on z/OS by categorizing them into ALL_SP/ALL_DP placed on FPRs while other non-complex type are categorized into AGGREGATE_OTHER placed on GPRs.
Fixes: #19952
Signed-off-by: ChengJin01 [email protected]
Reviewer: @zl-wang (the code related to the encoding signature), @tajila (the FFI code in java) FYI: @dchopra001, @r30shah, @joransiu, @pshipton
also, is there any special treatment in zOS ABI for single-member struct/union? something like:
struct {
int me;
}
Hopefully no ... such that it is good as it is.
also, is there any special treatment in zOS ABI for single-member struct/union? something like:
If the passing parameter is union, it will regardless of member type, it will be passed in GPR. For the single element struct, it will be in GPR as well (whether it is Floating point or not).
If the passing parameter is union, it will regardless of member type, it will be passed in GPR. For the single element struct, it will be in GPR as well (whether it is Floating point or not).
for union treated as you described, is it limited to single-member and no-more-than-8-byte? or, you meant any union?
how about nested single-member struct? something like: struct { struct {int a; int b;} c; } similar questions as well.
for union treated as you described, is it limited to single-member and no-more-than-8-byte? or, you meant any union? how about nested single-member struct? something like: struct { struct {int a; int b;} c; } similar questions as well.
According to LE Vendor reference [1], for argument passing, any aggregate type that is not union and contains exactly two floating point types of same size, is considered complex type. All other parameters from the argument lists are passed in GPR1-3 if available (With exception of floating point type.
So the case you shared, if we have nested single member struct, as the aggregate has only one member, it will be passed in available GPRs.
[1]. https://www.ibm.com/docs/en/SSLTBW_2.5.0/pdf/ceev100_v2r5.pdf
ok ... that makes it clear. the implementation only needs to tighten up on a few cases mentioned above, and we are good to go.
@zl-wang, is there is anything left to be updated against the clarification by @r30shah? If no more concern, we should be able to get this merged asap given we've got a bunch of Jtreg test suites to be verified with fix.
@ChengJin01 at least three categorization bugs I can see, mentioned previously:
struct { float a[2]; }should not be complex (even though ALL_SP and 8-byte in size);union { float a[2]; float b}should not be complex (even though ALL_SP and 8-byte in size);struct { struct {float a; float b;} c;}should not be complex either (even though ALL_SP and 8-byte in size); (@r30shah agree? i am not sure about this case) while you are fixing these, keep in mind the few being-complex cases mentioned previously.
@ChengJin01 at least three categorization bugs I can see, mentioned previously:
1. `struct { float a[2]; }` should not be complex (even though ALL_SP and 8-byte in size); 2. `union { float a[2]; float b}` should not be complex (even though ALL_SP and 8-byte in size); 3. `struct { struct {float a; float b;} c;} `should not be complex either (even though ALL_SP and 8-byte in size); (@r30shah agree? i am not sure about this case) while you are fixing these, keep in mind the few being-complex cases mentioned previously.
@zl-wang,
As for case 2 union { float a[2]; float b}, it is treated as non-complex in this PR (the code in isStructWithFFOrDD already excludes any union type in the case of ALL_SP in 8 bytes) (the test case is https://github.com/eclipse-openj9/openj9/blob/9249e24cff6d9fb80e30378bdde9e25bb7192458/test/functional/Java21Only/src/org/openj9/test/jep442/upcall/UpcallMHWithUnionTests.java#L1610 in OpenJ9 which is already verified with the fix).
As for case 3 struct { struct {float a; float b;} float c;} (is it struct { struct {float a; float b;}} ?) , it is more than 8 bytes which is out of our scope in discussion (which is definitely non-complex that is already covered in this PR) (the test case is https://github.com/eclipse-openj9/openj9/blob/9249e24cff6d9fb80e30378bdde9e25bb7192458/test/functional/Java21Only/src/org/openj9/test/jep442/upcall/UpcallMHWithStructTests.java#L2224 in OpenJ9 which is already verified with the fix).
I agree with the cases @zl-wang shared, all those should be considered non complex.
@dchopra001,
please help confirm whether these 3 cases above are covered in this PR against z/OS ABI as the fix is totally based on z/OS ABI (my understanding is that case 2 & 3 should be addressed as explained at https://github.com/eclipse-openj9/openj9/pull/20018#issuecomment-2302479915).
@zl-wang & @r30shah,
Do the rules also apply to double?
e.g.
struct { double a[2]; } should not be complex (even though ALL_DP and 16-byte in size);
union { double a[2]; double b} should not be complex (even though ALL_DP and 16-byte in size);
struct { struct {double a; double b;}} should not be complex either (even though ALL_DP and 16-byte in size);
If so, we will need to unify the code to handle them altogether.
@ChengJin01 Yes , rules include all floating point types, so doubles as well.
@ChengJin01 Yes , rules include all floating point types, so doubles as well.
I've updated the code to handle all the 3 cases above for float & double.
@ChengJin01
As for case 3
struct { struct {float a; float b;} float c;}
you added float before c ... making it invalid declaration.
struct { struct {float a; float b;}} is an invalid type declaration as well ... you need a field name at least (I named it c previously in my case 3). but i understood you are referring to the same "type" as I ... it is 8-byte nested struct ALL_SP. since it has only one member ... so, it is not a complex. current code did it right?
so, it is not a complex. current code did it right?
@zl-wang,
Yes, the latest changes already covers this case (a struct with a 8-byte nested struct ALL_SP) as that's my understanding of our clarification above.
Just fixed the code conflicts when rebasing the branch with the latest changes.
@dchopra001, please help confirm whether the fix works good in OpenJ9 FFI test suites.
@dchopra001, please help confirm whether the fix works good in OpenJ9 FFI test suites.
This change fixes the issues we observed in openj9 functional tests.
We still see some failures in openjdk tests. I can't say with 100% certainty yet if they are all unrelated to this issue as I'm still working through them. However they don't look related so far.
The test failures at https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_aarch64_linux_Personal/204/tapTestReport/ (jobs were not yet finished) had nothing to do with FFI related tests (passed).