Skip to content

[FFI/Test] Test suites intended for the duplicate ffi_type for struct - #19922

Merged
tajila merged 1 commit into
eclipse-openj9:masterfrom
ChengJin01:ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites
Aug 9, 2024
Merged

[FFI/Test] Test suites intended for the duplicate ffi_type for struct#19922
tajila merged 1 commit into
eclipse-openj9:masterfrom
ChengJin01:ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites

Conversation

@ChengJin01

@ChengJin01 ChengJin01 commented Jul 26, 2024

Copy link
Copy Markdown

The changes add new test suites which are used to verify
the code that avoids creating the duplicate ffi_type for
the same struct for arguments/return type in downcall.
Also, these tests must work for the existing FFI specific
implementation.

Related: #19714

Signed-off-by: ChengJin01 jincheng@ca.ibm.com

@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 4700c46 to 356fb45 Compare July 26, 2024 04:23
@ChengJin01 ChengJin01 added comp:test project:panama Used to track Project Panama related work labels Jul 26, 2024
@ChengJin01

Copy link
Copy Markdown
Author

The new test suites are verified in personal builds on all supported platforms.

Reviewer: @tajila, @keithc-ca
FYI: @pshipton

@ChengJin01
ChengJin01 requested review from keithc-ca and tajila July 26, 2024 04:25
Comment thread runtime/tests/clinkerffi/CMakeLists.txt Outdated
@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch 7 times, most recently from d180da5 to 9b6226a Compare July 30, 2024 03:26
@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 9b6226a to ee37591 Compare July 30, 2024 19:54
@ChengJin01

Copy link
Copy Markdown
Author

@keithc-ca, is there anything else to be addressed in this PR?

@keithc-ca

Copy link
Copy Markdown
Contributor

Yes, I have been reviewing the current state of this; I will share my comments tomorrow.

@keithc-ca keithc-ca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've highlighted a number of unused imports; please remove those from other packages as well.

@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from ee37591 to 63a6272 Compare August 1, 2024 20:41
@keithc-ca

Copy link
Copy Markdown
Contributor

The description says "avoids the duplicate ffi_type for struct"; please explain what that means.

@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch 2 times, most recently from 2f0ad43 to 2f3fbe8 Compare August 2, 2024 14:26
@ChengJin01

ChengJin01 commented Aug 2, 2024

Copy link
Copy Markdown
Author

The description says "avoids the duplicate ffi_type for struct"; please explain what that means.

As explained in the description at #19714, we will need to optimize the existing code to avoid the creating duplicate ffi_type for the same struct in argument/return type in downcall (already updated the commit description to clarify that).

@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 2f3fbe8 to 9ec3dc2 Compare August 2, 2024 19:44
@ChengJin01

Copy link
Copy Markdown
Author

@keithc-ca, I've gone through all related test suites with unused imports & displaced imports fixed. Please help cross-check whether anything else I ignored the latest update.

@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 9ec3dc2 to 35d9f5d Compare August 2, 2024 21:06

@keithc-ca keithc-ca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of the comments from this round apply to several files other than those explicitly mentioned: please address those issues throughout.

Comment thread test/functional/Java17andUp/src_170/org/openj9/test/jep389/valist/ApiTests.java Outdated
Comment thread test/functional/Java17andUp/src_170/org/openj9/test/jep389/valist/ApiTests.java Outdated
@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 35d9f5d to 59ae9cb Compare August 7, 2024 16:04
@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 59ae9cb to b642c47 Compare August 7, 2024 20:13
The changes add new test suites which are used to verify
the code that avoids creating the duplicate ffi_type for
the same struct for arguments/return type in downcall.
Also, these tests must work for the existing FFI specific
implementation.

Related: eclipse-openj9#19714

Signed-off-by: ChengJin01 <jincheng@ca.ibm.com>
@ChengJin01
ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from b642c47 to 80abde0 Compare August 7, 2024 21:26
@keithc-ca

Copy link
Copy Markdown
Contributor

Jenkins test sanity zlinux jdk17,jdk21,jdk22

@keithc-ca

Copy link
Copy Markdown
Contributor

@tajila Could you review this please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:test project:panama Used to track Project Panama related work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants