Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the cast issue with return values in dispatcher for FFI upcall #15900

Merged

Conversation

ChengJin01
Copy link

The changes fix the cast issue with the values returned from native2InterpJavaUpcallImpl().

Signed-off-by: Cheng Jin jincheng@ca.ibm.com

@ChengJin01
Copy link
Author

Reviewer: @keithc-ca
FYI: @tajila

Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

Other comments/questions:

  • in native2InterpJavaUpcallImpl

    • returnStorage should be of type U_64
    • line 244 can be simplified to returnsObject = (J9NtcPointer == returnType) || (J9NtcStruct == returnType);
    • lines 297-299: why is the shift only applied for float?
    • please fix the comment on line 300 which should be /* !defined(J9VM_ENV_LITTLE_ENDIAN) */
    • please fix the indentation on lines 337-339
  • please add the missing space before ( on line 426

runtime/vm/UpcallVMHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/UpcallVMHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/UpcallVMHelpers.cpp Outdated Show resolved Hide resolved
@ChengJin01 ChengJin01 force-pushed the fix_cast_integral_dispatcher_ffi branch from e1aa684 to 4748599 Compare September 15, 2022 16:20
@ChengJin01
Copy link
Author

ChengJin01 commented Sep 15, 2022

returnStorage should be of type U_64

Updated at all places using returnStorage.

@ChengJin01
Copy link
Author

line 244 can be simplified to returnsObject = (J9NtcPointer == returnType) || (J9NtcStruct == returnType);

Agreed and fixed as suggested.

@ChengJin01
Copy link
Author

ChengJin01 commented Sep 15, 2022

lines 297-299: why is the shift only applied for float?

We have java code to specially handle float by converting to int at first and further to long in downcall handler at

private static final long floatToLongArg(float argValue) {
, in which case we need to address this case in native before passing it on the java stack in upcall on the Big-Endian(BE) platforms. I just updated the comment for clarification.

@ChengJin01
Copy link
Author

ChengJin01 commented Sep 15, 2022

please fix the comment on line 300 which should be /* !defined(J9VM_ENV_LITTLE_ENDIAN) */
please fix the indentation on lines 337-339

Fixed.

runtime/vm/UpcallVMHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/UpcallVMHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/UpcallVMHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/UpcallVMHelpers.cpp Show resolved Hide resolved
@ChengJin01 ChengJin01 force-pushed the fix_cast_integral_dispatcher_ffi branch from 4748599 to 969e1ea Compare September 15, 2022 18:50
@keithc-ca
Copy link
Contributor

I think this has reached a good place. Please let me know whether your testing confirms that.

@ChengJin01 ChengJin01 force-pushed the fix_cast_integral_dispatcher_ffi branch 2 times, most recently from aeb2a48 to 70e4fb3 Compare September 15, 2022 20:34
@keithc-ca
Copy link
Contributor

Can you fix these minor things:

  • the missing period on lines 165, 207, 219, 400, 419, 449 & 616
  • the indentation of lines 320-322

The changes fix the cast issue with the values returned
from native2InterpJavaUpcallImpl().

Signed-off-by: Cheng Jin <jincheng@ca.ibm.com>
@ChengJin01 ChengJin01 force-pushed the fix_cast_integral_dispatcher_ffi branch from 70e4fb3 to 8db423b Compare September 16, 2022 16:44
@ChengJin01
Copy link
Author

the missing period on lines 165, 207, 219, 400, 419, 449 & 616
the indentation of lines 320-322

Fixed.

@keithc-ca
Copy link
Contributor

Thanks for fixing the other minor issues I missed. ;-)

@keithc-ca
Copy link
Contributor

jenkins test sanity alinux64 jdk19

@keithc-ca keithc-ca merged commit d3ebc82 into eclipse-openj9:master Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants