-
-
Notifications
You must be signed in to change notification settings - Fork 30
cdylib: apply versioning to symbols #420
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
base: main
Are you sure you want to change the base?
Conversation
49969a5 to
eff48c9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
234c03f to
9e7f85a
Compare
|
the failing test (checking that the symbols are actually versioned) succeeds with |
4ad10be to
735b380
Compare
| working-directory: libz-rs-sys-cdylib | ||
| run: | | ||
| cargo build --release --target ${{matrix.target}} --features=gz | ||
| cc -shared -Wl,--whole-archive target/${{matrix.target}}/release/libz_rs.a -Wl,--no-whole-archive -Wl,--version-script=include/zlib.map -Wl,--undefined-version -Wl,-soname,libz_rs.so.1 -lc -o target/${{matrix.target}}/release/libz_rs.versioned.so |
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.
Did you try to just set package.metadata.capi.library.rustflags with the linker line above in Cargo.toml and use cargo-cbuild?
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 added a target-specific section with just rustflags for now. It will hit the release matching current cargo
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 linker has to be invoked separately. If we let rustc invoke the linker, it will pass a version script that conflicts with the version script this PR applies.
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.
wouldn't the last one win?
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 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 seems working with the current lld
Running `rustc --crate-name z_rs --edition=2021 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=252 --crate-type staticlib --crate-type cdylib --emit=dep-info,link -C panic=abort -C embed-bitcode=no -C debuginfo=2 --deny=unsafe_op_in_unsafe_fn --cfg 'feature="c-allocator"' --cfg 'feature="capi"' --cfg 'feature="default"' --check-cfg 'cfg(docsrs,test)' --check-cfg 'cfg(feature, values("__internal-test", "c-allocator", "capi", "custom-prefix", "default", "gz", "gzprintf", "rust-allocator", "semver-prefix"))' -C metadata=e06a1d63542bbf68 --out-dir /root/zlib-rs/libz-rs-sys-cdylib/target/aarch64-unknown-linux-gnu/debug/deps --target aarch64-unknown-linux-gnu -C linker=clang -C incremental=/root/zlib-rs/libz-rs-sys-cdylib/target/aarch64-unknown-linux-gnu/debug/incremental -L dependency=/root/zlib-rs/libz-rs-sys-cdylib/target/aarch64-unknown-linux-gnu/debug/deps -L dependency=/root/zlib-rs/libz-rs-sys-cdylib/target/debug/deps --extern libz_rs_sys=/root/zlib-rs/libz-rs-sys-cdylib/target/aarch64-unknown-linux-gnu/debug/deps/liblibz_rs_sys-f92c9d585058d057.rlib --extern zlib_rs=/root/zlib-rs/libz-rs-sys-cdylib/target/aarch64-unknown-linux-gnu/debug/deps/libzlib_rs-ac94033f0625444d.rlib -C link-arg=-Wl,-soname,libz_rs.so.1 -Clink-arg=-Wl,--no-whole-archive -Clink-arg=-Wl,--version-script=include/zlib.map -Clink-arg=-Wl,--undefined-version --cfg cargo_c --print native-static-libs -C link-arg=-fuse-ld=lld`
LLD 21.1.4 (compatible with GNU linkers)
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.
that invocation has only one version script, so I'm not sure what you are actually running here?
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 other version script is injected by rustc.
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 test it using the following configuration:
[target.aarch64-unknown-linux-gnu]
linker = "clang"
# rustflags = [ "-C", "link-arg=-fuse-ld=ld" ]
# rustflags = [ "-C", "link-arg=-fuse-ld=lld" ]
# rustflags = [ "-C", "link-arg=-fuse-ld=mold" ]uncommenting one at time.
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'm seeing that now lld and ld seem to favour the first linker script so only using mold we get what is expected according to objdump.
735b380 to
69e5cc0
Compare
| gz_error; | ||
| gz_intmax; | ||
| rust_eh_personality; | ||
| _*; |
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.
re #419 (comment), _* is already excluded, so that should cover _R* right?
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.
Right
fixes #419
A tried to use a
.cargo/config.tomlinstead, e.g.but I just can't get that path to work. So here we are with a
build.rs, something I've been wanting to avoid.There is another alternative in manually adding assembly directives, but that seems even more problematic.