Add support for RISC V #26
Conversation
This is necessary to support architectures that are unavailable on macos.
dodomorandi
force-pushed
the
riscv
branch
2 times, most recently
from
71807e4 to
449a80b
Compare
|
Thanks so much for your contribution! |
| in("s2") $closure_env_ptr, | ||
| in("s3") $jbuf_ptr, | ||
| in("s4") $c2r, | ||
| clobber_abi("C"), // clobber abi reflects call effects, including {x1,x5,x6,x7}... |
There was a problem hiding this comment.
| /// But also: You shouldn't be poking at these! | ||
| #[cfg(target_arch = "riscv64")] | ||
| #[repr(C)] | ||
| pub struct JmpBufFields { |
There was a problem hiding this comment.
do we need to encode the fields at this level of detail here?
I.e. the other JmpBufFields just includes an abstract pile of u64's (where the size of that type is dependent on the target OS+arch) that it expects the C runtime to manage on its own as part of the setjmp/longjmp implementation. Is there an advantage I'm not seeing to including the individual fields in the manner that you have here? (Or worse, some system invariant that I would be violating if I were to attempt a similar implementation strategy here?)
There was a problem hiding this comment.
To be honest, I was surprised to see that the glibc code was so different between different architectures (I expected to find an array of long int/long long int -- I am also wondering why they don't just use a uint64_t, whatever), and in the end I used the same style of glibc to keep it easier to compare.
Said that, the "similarity" with the C code was the only reason behind this. There is no issue whatsoever with alignment or padding (because all the fields perfectly align to 8 bytes), and type punning is totally fine in this case (correct me if I am wrong, but I am pretty confident in this case). Therefore I can use an array of u64s similarly to x86_64 and aarch64 if you want, just let me know what you prefer!
There was a problem hiding this comment.
yeah I agree with you that just ensuring 8-byte alignment seems like it will handle all possible issues here.
and I think it will be easier to maintain over time if we just use an array of u64s.
so if you can make that change, I'll r+ after that.
There was a problem hiding this comment.
Ok, now I only made the JMP_BUF_SIZE target specific. I added a very brief comment for one hardcoded value, it should be fine.
|
(once my question about JmpBufFields has been addressed, I'll be happy to land this.) |
The current code should only be compiled when using a x86_64 or a aarch64 architecture. I think that this should be considered a breaking change, but on the other hand I don't think the crate could be used outside those two architectures at the moment.
The macos_compat layer should also include a `JMP_BUF_SIZE` const (or a totally different structure, just like with glibc), but it has been omitted because there is no support for RISC V at the moment. The asm based implementation should be fine for riscv32, but I did not check what `JmpBufFields` should be.
2 participants
I am adding support for RISC V architecture.
In order to do so, I also needed to disable
macos_compatcompilation for targets different than macos. This is because at the moment there is not support for RISC V for mac, and it is not possible for me to get the correct layout ofJmpBufFieldsorSigJmpBufFields.Unfortunately tests are failing on myriscv64gc-unknown-linuxmachine, therefore this is still in draft. I still have to dig deeper, but any help is more than welcome.Things looks fine, but if you give a better look it is probably better.