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

Jump to bottom

Support Loongarch64 #1440

Merged
merged 1 commit into from May 30, 2022
Merged

Support Loongarch64 #1440

merged 1 commit into from May 30, 2022

Conversation

Panxuefeng-loongson
Copy link
Contributor

Co-Authored-By: Ao Qi

@Panxuefeng-loongson
Copy link
Contributor Author

libffi code have been merged into upstream, see: libffi/libffi@f259a6f. I have successfully built JNA based on it, tests pass all.

Note: We have Loongarch machines in the GCC compile farm, see: https://cfarm.tetaneutral.net/machines/list/. I think you can build the Loongarch port code with it.

@Panxuefeng-loongson
Copy link
Contributor Author

@dbwiddis @theaoqi Hi all, please review the code for this pr.

@dbwiddis
Copy link
Contributor

Changes look sane -- there is one TODO in the code. Can you elaborate on if this is still uncertain?

@theaoqi theaoqi mentioned this pull request May 25, 2022
Closed
@Panxuefeng-loongson
Copy link
Contributor Author

Changes look sane -- there is one TODO in the code. Can you elaborate on if this is still uncertain?

TODO where I am not very sure, I need to check with my colleagues. Thank you for your valuable comments!

@Panxuefeng-loongson
Copy link
Contributor Author

On my Loongarch machine，multiArchPath is loongarch64-linux-gnu. So, I delete Todo code and please review again. @dbwiddis @theaoqi

theaoqi
theaoqi approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@theaoqi theaoqi left a comment

Choose a reason for hiding this comment

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

The change LGTM. I am not sure if we should wait for the libffi update which contains LoongArch.

src/com/sun/jna/NativeLibrary.java Outdated
@@ -988,6 +988,9 @@ else if (Platform.isARM()) {
else if (Platform.ARCH.equals("mips64el")) {
libc = "-gnuabi64";
}
else if (Platform.ARCH.equals("loongarch64")) {
libc = "-gnuabi64"; // TODO: not sure
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be removed, see https://github.com/loongson/jna/blob/ed826d277fedabbe44d54d392c1219c60c73f0ea/src/com/sun/jna/NativeLibrary.java#L987 . However checking again is suggested by me.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is outdated, you already did it.

@Panxuefeng-loongson
Copy link
Contributor Author

Hi, @dbwiddis. The change looks good to @theaoqi and I have revised your suggestion. Please review again. Thanks

matthiasblaesing
matthiasblaesing reviewed May 27, 2022
View reviewed changes
Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Thank you - generally looks sane to me. The build currently fails as no initial linux-loongarch64.jar is provided. Please initiate it from out-of-date.jar.

In the long run the embedded copy of libffi needs also be updated.

@Panxuefeng-loongson
Copy link
Contributor Author

Thank you - generally looks sane to me. The build currently fails as no initial linux-loongarch64.jar is provided. Please initiate it from out-of-date.jar.

In the long run the embedded copy of libffi needs also be updated.

libffi just merged into upstream a few days ago, see: libffi/libffi@f259a6f and jna needs to sync with it.

By the way, I think linux-loongarch64.jar has been initialized in build.xml,see:
<copy file="${lib.native}/out-of-date.jar" tofile="${lib.native}/linux-loongarch64.jar" overwrite="true"/>

@matthiasblaesing
Copy link
Member

Thank you - generally looks sane to me. The build currently fails as no initial linux-loongarch64.jar is provided. Please initiate it from out-of-date.jar.

By the way, I think linux-loongarch64.jar has been initialized in build.xml,see: <copy file="${lib.native}/out-of-date.jar" tofile="${lib.native}/linux-loongarch64.jar" overwrite="true"/>

It is, if a breaking change of the native interface would have been detected. This is not the case. You need to add the empty file by hand. Have a look at the unittests attached to this PR, which all fail because of this.

@Panxuefeng-loongson
Copy link
Contributor Author

Hi, @matthiasblaesing . Thank you for your advice, I realized my mistake and fixed it.

@matthiasblaesing
Copy link
Member

Thanks for the update. I have one (maybe two) last request(s) before this good to go in:

  • please add an entry to CHANGES.md regarding "Support for Loongarch64" to the features section of the next version
  • I had a look at the raw git commit. This looks in general good. I'm not sure regarding your name. I don't know chinese customs about how to record your name, but the current author entry looks more like a user name and less like a real name. Could please check that? (running git log -n 1 in your branch show you the recorded author information)

@Panxuefeng-loongson Panxuefeng-loongson force-pushed the support-Loongarch64 branch 3 times, most recently from 91ebb08 to ac553b8 Compare May 30, 2022 01:25
theaoqi
theaoqi reviewed May 30, 2022
View reviewed changes
CHANGES.md Outdated
@@ -9,6 +9,7 @@ Features
--------
* [#1433](https://github.com/java-native-access/jna/pull/1433): Add `CFEqual`, `CFDictionaryRef.ByReference`, `CFStringRef.ByReference` to `c.s.j.p.mac.CoreFoundation` - [@shalupov](https://github.com/shalupov)
* [#978](https://github.com/java-native-access/jna/issues/978): Remove use of finalizers in JNA and improve concurrency for `Memory`, `CallbackReference` and `NativeLibrary` - [@matthiasblaesing](https://github.com/matthiasblaesing).
* [#1440](https://github.com/java-native-access/jna/pull/1440): Support Loongarch64 - [@Panxuefeng-loongson](https://github.com/Panxuefeng-loongson).
Copy link
Contributor

Choose a reason for hiding this comment

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

“Support for LoongArch64” may be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

“Support for LoongArch64” may be better.

Done.

@Panxuefeng-loongson Panxuefeng-loongson force-pushed the support-Loongarch64 branch 2 times, most recently from 2d84162 to b0d9853 Compare May 30, 2022 01:47
@Panxuefeng-loongson @theaoqi
Support for LoongArch64
0066b08 
Co-Authored-By: Ao Qi <aoqi@loongson.cn>
@matthiasblaesing
Copy link
Member

Looks good to me. Thank you.

1 similar comment
@matthiasblaesing
Copy link
Member

Looks good to me. Thank you.

@matthiasblaesing matthiasblaesing merged commit bf60e51 into java-native-access:master May 30, 2022
@Panxuefeng-loongson Panxuefeng-loongson deleted the support-Loongarch64 branch May 31, 2022 00:14
@Panxuefeng-loongson Panxuefeng-loongson mentioned this pull request Nov 15, 2022
Closed
@matthiasblaesing matthiasblaesing mentioned this pull request Aug 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theaoqi theaoqi theaoqi approved these changes

@matthiasblaesing matthiasblaesing Awaiting requested review from matthiasblaesing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Panxuefeng-loongson @dbwiddis @matthiasblaesing @theaoqi