-
Notifications
You must be signed in to change notification settings - Fork 78
Conversation
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.
Thank you! Looks good overall, but there are a few issues :)
build.rs
Outdated
@@ -12,9 +12,9 @@ fn main() { | |||
let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); | |||
let name = env::var("CARGO_PKG_NAME").unwrap(); | |||
|
|||
if target.starts_with("riscv") { | |||
if target.starts_with("riscv") && env::var_os("CARGO_FEATURE_INLINE_ASM").is_none() { |
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 inline-asm
feature cannot be used here: riscv-rt assembly library is always linked to, it cannot be inlined.
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, I found it, but we can make use the global_asm
feature in nightly build, I tried it, but since I am not familiar with rust, I processed the conditional compilation manually(split the asm.S file to asm64.s and asm32.s), and using macro global_asm!
to inline them. I thought this can be inline-asm
.
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.
There is no plans to use global_asm
currently, we only use asm.S
.
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.
OK, I know that. But can I know why this crate decided not to use global_asm
? Dose it have some disadvantages?
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.
It doesn't make sense. With global_asm
you will need two implementations: one for those who want to use global_asm
and asm.S
for those who don't. And since there is no profit from using global_asm
, it's better to use a single implementation.
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.
Well, just a thought, what about adding something into build.rs
to generate a asm_inline.s
from asm.S
,and then using this generated file with global_asm
?
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.
But what profit you can get with this approach?
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.
Well, totally git rid of gcc
toolchain? Every thing can build from source with Rust it self? Maybe this thought is unpractical, and maintain more code for it is a totally wasting?
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.
I believe you can use this crate without gcc toolchain. It is only required for updating the blobs.
Actually there is a way to implement what you want, but it's more complicated. You can take a look at https://github.com/rust-embedded/cortex-m/. It's still more useful for riscv
than riscv-rt
...
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.
OK, I will take a look. Thank you!
@@ -7,13 +7,40 @@ crate=riscv-rt | |||
# remove existing blobs because otherwise this will append object files to the old blobs | |||
rm -f bin/*.a | |||
|
|||
for ext in i ic im imc |
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.
M extension support is not present in the resulting scripts for some reason. We still need it :)
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.
Well, I did not known that. You see, I just copy this from riscv
crate. So, Adding these extensions to the build script may be a solution? Just like this:
riscv64-unknown-elf-gcc -c -mabi=ilp32 -march=rv32im asm.S -o bin/$crate.o ar crs bin/riscv32i-unknown-none-elf.a bin/$crate.o
riscv64-unknown-elf-gcc -c -mabi=ilp32 -march=rv32imc asm.S -o bin/$crate.o ar crs bin/riscv32ic-unknown-none-elf.a bin/$crate.o
or append if ifc ...
to ext
?
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.
Whatever you like as long as it's correct
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.
OK, I will work on it later. Thanks for your help and reply!
# remove existing blobs because otherwise this will append object files to the old blobs | ||
Remove-Item -Force bin/*.a | ||
|
||
$crate = "riscv-rt" | ||
$extension_sets = @("i", "im", "ic", "imc") |
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.
Why this change? It seems cleaner to me in the old code.
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 for that, but the the abi
changed too. So I just copy this build script from riscv
crate. I am trying on it.
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.
Looks good to me, thanks!
bors r+ |
Build succeeded: |
hi everyone, I am a new guy with both github, rust and riscv, and this is my first PR, so if I miss something or do something wrong, please let me know (and forgive my poor english, since I am not a native speaker).
crate
riscv
v0.7 solved link error aboutdifferent hardware float abi
, but riscv-rt still depend onriscv
v0.6.