-
Notifications
You must be signed in to change notification settings - Fork 38
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
Conversation
It looks like this is failing in CI, with |
yes, I'm looking for the reason, thanks |
The error was caused by the llvm. we should wait for the official support for the loongarch target when llvm16 released |
There was a problem hiding this 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
Outdated
@@ -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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.) |
@sunfishcode |
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 |
There was a problem hiding this 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".)
gen/ioctl/generate.sh
Outdated
@@ -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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
@sunfishcode Disable tests for loongarch64 is still needed for lower llvm versions. thanks |
rust-lang/rust#96971 merged. Maybe we are ready to go? :) |
gen/ioctl/generate.sh
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. ❤️ |
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