-
Notifications
You must be signed in to change notification settings - Fork 22
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 cdylib support by building some static tables in C #1322
base: main
Are you sure you want to change the base?
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.
I want to look into this more. I really don't like moving anything to C. Right now rav1d
is all Rust and asm, and this adds back C, which is really not great. And importing statics is not safe, so this adds a bunch of extra unsafety.
If you're able to find a better solution that'd be great, but I already went through this with @rinon and this was the most portable solution we were able to come up with. One potential alternative approach was to use a custom linker script to override the symbols to not be global, but that then required that the project be linked with |
I don't want to spend more time on this, @kkysen. Rust doesn't support what we need to make these symbols accessible to assembly but also hidden visibility. |
Would it be possible to make the cdylib a separate crate that links and re-exports all of rav1d? Then it could be optional and the extra C tables need only be linked into it? |
That would be much better. Is it also possible to detect if we are building a cdylib and only compile C then? I'd prefer to keep the tables in Rust, as well, as everything is fully unchecked and unsafe with them only in C. Also, who requires a cdylib vs just a staticlib? |
I want a drop-in replacement compatible with libdav1d.so to be available. We can't add |
So we do have the option of leaving the tables in Rust, but ALSO compiling the C tables for asm. But that means duplicating the tables in the final binary, and I'm pretty sure the compiler can't optimize/deduplicate that. The only way to avoid the duplication afaik is to have the Rust code use the tables that are exported from C. |
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 cdylib
is not even supported at all on aarch64-unknown-linux-gnu
. I get this warning:
> cargo build --target aarch64-unknown-linux-gnu
warning: dropping unsupported crate type `cdylib` for target `aarch64-unknown-linux-gnu`
Do we really need to add cdylib
support? We don't even know yet if people care about cdylib
support at all.
And from rust-lang/rust#73958, it sounds like chromium is also running into this issue, so whatever workaround they use would probably work if they try to integrate rav1d
as a cdylib
.
cdylib is supported on |
485f2a9
to
b49e3c4
Compare
Yeah, I figured it out with @rinon. I had a |
This seems like the best way to me, having talked to Stephen about it now. |
In order to add cdylib support, we needed to resolve linker errors that were occurring due to
#[no_mangle]
symbols being exported globally. The corresponding symbols in the original C are declared asextern __attribute__((visibility("hidden")))
, and the assembly relocations are written assuming that visibility level. When we ported the tables to Rust and made them#[no_mangle]
they became global symbols, which broke the relocations needed to build rav1d as a cdylib with asm support.There's currently no way to declare a Rust symbol with the same hidden visibility (see this rust-lang issue for relevant discussion), so to work around this we've opted to build the original C definitions for the tables and import them into Rust. This fixes linking when building as a cdylib with asm support, at the cost of some complexity. I've opted to only import the specific symbols that are referenced by asm, and have left the remaining tables in Rust.
One additional note: Currently the tables that are now declared in both Rust and
tables.c
are duplicated, but only in the static archive (librav1d.a
). In both the finaldav1d
executable andlibrav1d.so
the C versions are dead code eliminated. I opted to leave it this way to avoid modifyingtables.h
/tables.c
, since we can rely on the compiler to optimize out the unused tables. But if that's a bad idea for some reason I'm missing, it shouldn't be too hard to add aRAV1D
define and wrap the unused tables in#ifndef RAV1D
to remove them at compile time.