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

RISC-V: backport libffi commit aa3fce08 #14699

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

janvrany
Copy link
Contributor

@janvrany janvrany commented Mar 11, 2022

This commit backports changes in upstream libffi, namely change in
commit

commit aa3fce08ba620c50db17215a9f14dd0f1facf741
Author: Andreas Schwab schwab@linux-m68k.org
Date: Sun Feb 13 21:04:33 2022 +0100

This change properly widens integral return value narrower than
XLEN bits as documented 1. Changes done in OpenJ9 commit 4c0adb4 (see #14610)
depend on this (documented) libffi behavior.

This commit backports changes in upstream libffi, namely change in
commit

   commit aa3fce08ba620c50db17215a9f14dd0f1facf741
   Author: Andreas Schwab <schwab@linux-m68k.org>
   Date:   Sun Feb 13 21:04:33 2022 +0100

This change properly widens integral return value narrower than
XLEN bits as documented [1]. Changes done in OpenJ9 commit 4c0adb
depend on this (documented) libffi behavior.

[1]: https://github.com/libffi/libffi/blob/ab1677106605aba1c27665964ff90bea59612ce3/doc/libffi.texi#L216-L221
@janvrany
Copy link
Contributor Author

FYI: @ChengJin01 , @pshipton , @sxa

@ChengJin01
Copy link

ChengJin01 commented Mar 11, 2022

Hi @janvrany, what we usually do is to get the last version of libffi/riscv in github (specifically https://github.com/libffi/libffi/tree/master/src/riscv in this case) to replace runtime/libffi/riscv instead of directly fixing them up in our code so as to keep it up-to-date and avoid any inconsistency in libffi in the future when updating, which means you need to raise an request with the proposed fix at first at https://github.com/libffi/libffi/issues for their review.

@janvrany
Copy link
Contributor Author

@ChengJin01 The fix is already merged in upstream: libffi/libffi#680

The OpenJ9 version of libffi/riscv/ffi.c pulled into OpenJ9 was not the upstream version anyway - beside IBM copyright, OpenJ9 version uses ALIGN() macro whereas at that time upstream was using FFI_ALIGN() macro for the same purpose. So it seems to me the RISC-V support was back-ported to some older version of libffi which has been pulled into openj9 before RISC-V support got in libffi.

In any case, I can copy the upstream version, add IBM copyright (I guess that's required) and change FFI_ALIGN() back to ALIGN() - but then the diff would be exactly the same.

That being said, if you now update whole libffi from upstream, nothing will break and there will be no inconsistency.

@ChengJin01
Copy link

ChengJin01 commented Mar 11, 2022

@janvrany,

The OpenJ9 version of libffi/riscv/ffi.c pulled into OpenJ9 was not the upstream version anyway - beside IBM copyright, OpenJ9 version uses ALIGN() macro whereas at that time upstream was using FFI_ALIGN() macro for the same purpose. So it seems to me the RISC-V support was back-ported to some older version of libffi which has been pulled into openj9 before RISC-V support got in libffi.

FFI_ALIGN() was already redefined as ALIGN() in https://github.com/eclipse-openj9/openj9/blob/master/runtime/include/ffi_common.h given we updated libffi/x86 last year, in which case all setting/dependency should be kept up-to-date.
The only thing you need to do is to compare /runtime/libffi/riscv with the latest version of /libffi/riscv at https://github.com/libffi/libffi/tree/master/src/riscv and replace them if any difference and also double-check /runtime/libffi/preconf/rv64 to see whether anything in there needs to be updated accordingly.

That being said, if you now update whole libffi from upstream, nothing will break and there will be no inconsistency.

We have a bunch of settings in /runtime/libffi specific to OpenJ9 which are totally different from the upstream, which means replacing them all will screw up the whole code related to libiffi in OpenJ9.

@janvrany
Copy link
Contributor Author

The only thing you need to do...

This is exactly what I did.

Copy link
Contributor

@sxa sxa left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me but since I'm not an openJ9 committer I don't believe my review counts ;-)

@ChengJin01
Copy link

FYI: @tajila, @DanHeidinga, @gacholio

@gacholio
Copy link
Contributor

Would it make sense to reimport the project wholesale?

@janvrany
Copy link
Contributor Author

@gacholio I do not know, but given that it is already a patchwork I'm not sure is it worth. I'm certainly not keen reimporting it myself.

@gacholio gacholio merged commit 5109e0e into eclipse-openj9:master Mar 11, 2022
@janvrany janvrany deleted the pr/riscv-update-libffi branch March 11, 2022 20:21
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.

4 participants