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

Add LoongArch64 support #46

Merged
merged 1 commit into from
Apr 18, 2023
Merged

Add LoongArch64 support #46

merged 1 commit into from
Apr 18, 2023

Conversation

zhaixiaojuan
Copy link
Contributor

The LoongArch architecture (LoongArch) is an Instruction Set Architecture (ISA) that has a RISC style.

Documentations:
ISA:
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html
ABI:
https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html
More docs can be found at:
https://loongson.github.io/LoongArch-Documentation/README-EN.html

@sunfishcode
Copy link
Owner

It looks like this is failing in CI, with error: unknown target triple 'loongarch64-unknown-linux-gnuf64', please use -triple or -arch; could you take a look?

@zhaixiaojuan
Copy link
Contributor Author

It looks like this is failing in CI, with error: unknown target triple 'loongarch64-unknown-linux-gnuf64', please use -triple or -arch; could you take a look

yes, I'm looking for the reason, thanks

@zhaixiaojuan
Copy link
Contributor Author

The error was caused by the llvm. we should wait for the official support for the loongarch target when llvm16 released

rust-lang/rust#107224

Copy link

@xen0n xen0n left a comment

Choose a reason for hiding this comment

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

We should definitely wait until rust-lang/rust#96971 is merged though.

gen/ioctl/generate.sh Show resolved Hide resolved
@@ -23,6 +23,9 @@ i686-linux-gnu-gcc main.c list.o -o main.exe $cflags
x86_64-linux-gnu-gcc -Iinclude -c list.c $cflags
x86_64-linux-gnu-gcc main.c list.o -o main.exe $cflags
./main.exe >> "$out"
loongarch64-unknown-linux-gnu-gcc -Iinclude -c list.c $cflags
loongarch64-unknown-linux-gnu-gcc main.c list.o -o main.exe $cflags
Copy link

Choose a reason for hiding this comment

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

Which distro this script is supposed to get run on? This looks like the Debian/Ubuntu cross toolchain naming but AFAIK the Debian port still hasn't settled yet?

gen/ioctl/list.c Outdated
@@ -95,7 +95,7 @@ struct sockaddr {
#include <linux/joystick.h>
#include <linux/kd.h>
#include <linux/kcov.h>
#if !defined(__arm__) && !defined(__powerpc64__) && !defined(__riscv) // various errors
#if !defined(__arm__) && !defined(__loongarch64) && !defined(__powerpc64__) && !defined(__riscv) // various errors
Copy link

Choose a reason for hiding this comment

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

Isn't __loongarch__ enough? Even if you want to restrict to 64-bit LoongArch you should probably use __loongarch_lp64 as the __loongarch64 symbol is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for all your advice

@xen0n
Copy link

xen0n commented Mar 29, 2023

Okay, so my Rust porting skills obviously need some refreshing, and this crate is actually blocking Cargo build, so we can't directly jump to Tier-2-with-host-tools in rust-lang/rust#96971. (Tools were basically dep-free from the good ol' days, so rustc and cargo basically always come together IIRC.)

@zhaixiaojuan
Copy link
Contributor Author

@sunfishcode
This ci test fails because llvm14 does not support the loongarch architecture and needs to be upgraded to llvm16 to pass this test. Can you help me upgrade the test environment? thanks

@sunfishcode
Copy link
Owner

I don't know of an easy way to update the test environment; we're already using ubuntu-latest. Would it work for now to disable the tests on loongarch64?

@zhaixiaojuan
Copy link
Contributor Author

I don't know of an easy way to update the test environment; we're already using ubuntu-latest. Would it work for now to disable the tests on loongarch64?

OK. disable the tests on loongarch64 is a good choice at the current test environment

Copy link

@xen0n xen0n left a comment

Choose a reason for hiding this comment

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

Disabling tests for loongarch64 for now is okay for me. We can always do it later once enough infra has been brought up. (We have done quite a lot in the past 2 years, but obviously that's still not "enough".)

@@ -44,5 +44,8 @@ qemu-riscv64 -L /usr/riscv64-linux-gnu ./main.exe >> "$out"
s390x-linux-gnu-gcc -Iinclude -c list.c $cflags
s390x-linux-gnu-gcc main.c list.o -o main.exe $cflags
qemu-s390x -L /usr/s390x-linux-gnu ./main.exe >> "$out"
loongarch64-linux-gnu-gcc -Iinclude -c list.c $cflags
loongarch64-linux-gnu-gcc main.c list.o -o main.exe $cflags
./main.exe >> "$out"
Copy link

Choose a reason for hiding this comment

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

Hmm this is implying the host environment can transparently run LoongArch binaries. See how other non-x86 arches launch their artifacts with qemu linux-user emulation.

But, as we don't have Debian/Ubuntu cross toolchains available yet, in fact even loongarch64-linux-gnu-gcc is not supposed to be available. How are we going to deal with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

qemu linux-user emulation method use the "-L /usr/xxx-linux-gnu",which specifies the library path to be searched by the qemu simulator. This path is usually set by the cross-compilation toolchain installer.
So no matter which method you use, compile the cross tool chain is the first step,
Maybe use the qemu simulator is more complicated, since most os use a lower
qemu version which does not support loongarch yet

Copy link

Choose a reason for hiding this comment

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

Yeah, the key point is: this script would be run by any contributor to this library, and adding those changes as-is would affect anyone not familiar with LoongArch development, which I suppose is the majority. (Also not everyone uses loongarch64-linux-gnu; Gentoo for example uses loongarch64-unknown-linux-gnu, notice the vendor field.)

We may have to disable the part for everyone else for the time being: maybe wrap them in a if false; then ... fi block with some explanations attached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vendor in the triple is configured by itself, eg, centos/fedora use x86_64-redhat-linux-gcc, alpine use x86_64-alpine-linux-musl-gcc, so use the common case without unknown

Copy link

@xen0n xen0n Mar 31, 2023

Choose a reason for hiding this comment

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

The vendor in the triple is configured by itself, eg, centos/fedora use x86_64-redhat-linux-gcc, alpine use x86_64-alpine-linux-musl-gcc, so use the common case without unknown

I'm not saying that we "should" choose one triple over another, but rather there's simply not a common case right now. Other arches are apparently using Debian cross tuples, but we don't currently have such a cross toolchain package available upstream, so loongarch64-linux-gnu is equally unusable as other variants so far.

(And, the argument that loongarch64-linux-gnu should be chosen because it's the "common denominator" is moot, because the current situation inherently favors Debian/Ubuntu hosts over other distros, and other distros will not work even if they all have the "common" parts in their versions of LoongArch triples. The command names are simply different, period. It's not going to magically match only because the "commonality" is recognized somehow.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://wiki.debian.org/Ports/loong64 has publised the loongarch64 target triple "loongarch64-linux-gnu"
So we can use the name as most arches do. yes, We can't get such a cross toolchain directly from the upstream now,
we can compile it. And debian community work is also in progress.

Copy link
Owner

Choose a reason for hiding this comment

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

So for now, would it work to just check in pregenerated loongarch64 files as separate files in the repo, and just have this gen/ioctl/generate.sh script just cat them into the output? That way, it doesn't depend on a toolchain that ubuntu-latest users won't have an easy way to install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't fully understand what you mean. Do you mean that our current patch only adds these files in the output src directory, or just temporarily removes the part of the gen/ioctl/generate.sh file that uses the loongarch cross-tool chain until the upstream cross-tool chain is available?

Copy link

Choose a reason for hiding this comment

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

@zhaixiaojuan I believe it's just a matter of moving the LoongArch part of the generated output to another file, so we can temporarily avoid calling into the toolchain non-existent on others' systems.

In other words, instead of:

loongarch64-linux-gnu-gcc -Iinclude -c list.c $cflags
loongarch64-linux-gnu-gcc main.c list.o -o main.exe $cflags
./main.exe >> "$out"

You should instead do this for the time being:

# cross LoongArch toolchain is not yet packaged in mainstream distros yet,
# so pre-generated output is used for the time being
cat loongarch-ioctls.txt >> "$out"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@zhaixiaojuan
Copy link
Contributor Author

@sunfishcode Disable tests for loongarch64 is still needed for lower llvm versions. thanks

@heiher
Copy link
Contributor

heiher commented Apr 12, 2023

rust-lang/rust#96971 merged. Maybe we are ready to go? :)

@@ -44,5 +44,8 @@ qemu-riscv64 -L /usr/riscv64-linux-gnu ./main.exe >> "$out"
s390x-linux-gnu-gcc -Iinclude -c list.c $cflags
s390x-linux-gnu-gcc main.c list.o -o main.exe $cflags
qemu-s390x -L /usr/s390x-linux-gnu ./main.exe >> "$out"
# As LoongArch cross toolchain is not yet packaged in mainstream distros yet,
# so pre-generated output is used for the time being
Copy link

Choose a reason for hiding this comment

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

Suggested change
# so pre-generated output is used for the time being
# pre-generated output is used for the time being

linux_version,
);
if !pre_gen {
run_bindgen(
Copy link

Choose a reason for hiding this comment

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

Does this mean LoongArch code could potentially get outdated if the bindgen version gets bumped by someone else not aware of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that is indeed a risk of pre-generation. It would be great if we could switch to automatic generation as soon as possible.

@heiher
Copy link
Contributor

heiher commented Apr 14, 2023

Hello, @sunfishcode In my recent pushes, I added support for pre-generation to the generator, which enables LoongArch to function in the current CI environment.

I wanted to check with you if there are any additional changes or improvements that you would like me to make before merging the pull request. I understand that you have a lot on your plate, so I want to be respectful of your time and ensure that the fix is complete and meets the standards of your project. If there are any additional changes that you would like me to make, please do not hesitate to let me know. I am more than happy to work with you to ensure that this fix is implemented correctly.

Thank you for your time and consideration, and I look forward to hearing back from you soon. ❤️

@sunfishcode sunfishcode merged commit 8217872 into sunfishcode:main Apr 18, 2023
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