Skip to content
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

-Zdefault-hidden-visibility causes linking errors due to intrinsic-related calls #123427

Open
chbaker0 opened this issue Apr 3, 2024 · 6 comments
Labels
A-intrinsics Area: Intrinsics A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@chbaker0
Copy link
Contributor

chbaker0 commented Apr 3, 2024

-Zdefault-hidden-visibility should affect symbols defined in a crate. References to externally-defined symbols should not have hidden visibility.

Certain intrinsics such as compare_bytes may emit calls to libc.so functions (e.g. memcmp). When -Zdefault-hidden-visibility is used, these symbol references are hidden. This leads to linking errors since an undefined hidden symbol cannot be resolved to a symbol in a dynamic library.

Repro:

#![feature(core_intrinsics)]

fn main() {
    let a = 0u8;
    let b = 0u8;

    println!("{}", unsafe {
        std::intrinsics::compare_bytes(&a as *const _, &b as *const _, 1)
    });
}

error:

note: /usr/bin/ld: .../rust/build/x86_64-unknown-linux-gnu/test/ui/default-hidden-visibility-intrinsic/default-hidden-visibility-intrinsic.default_hidden_visibility_intrinsic.adfc02789a1ff675-cgu.0.rcgu.o: in function `default_hidden_visibility_intrinsic::main':
           default_hidden_visibility_intrinsic.adfc02789a1ff675-cgu.0:(.text._ZN35default_hidden_visibility_intrinsic4main17hf16163bc798359ebE+0x1e): undefined reference to `memcmp'
           /usr/bin/ld: .../rust/build/x86_64-unknown-linux-gnu/test/ui/default-hidden-visibility-intrinsic/default-hidden-visibility-intrinsic: hidden symbol `memcmp' isn't defined
           /usr/bin/ld: final link failed: bad value
           collect2: error: ld returned 1 exit status

The flag was added in #118417; MCP rust-lang/compiler-team#656

While this flag isn't really intended for use on bin crates, I suspect crates downstream of an rlib or staticlib would have the same errors.

@chbaker0 chbaker0 added the C-bug Category: This is a bug. label Apr 3, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 3, 2024
@chbaker0
Copy link
Contributor Author

chbaker0 commented Apr 3, 2024

cc @bridiver @anforowicz @danakj

@bridiver
Copy link

bridiver commented Apr 3, 2024

In my original test of this locally I only changed https://github.com/rust-lang/rust/pull/118417/files#diff-6e38fd176c5407241bf7308381c05b28fd0b2d7f768ea52721cd3eaa6145653fR87 and it appeared to work correctly in terms of the resulting symbol table.

@chbaker0
Copy link
Contributor Author

chbaker0 commented Apr 3, 2024

If that's the desired behavior, perhaps this flag should have a different name. I'd assume the original target config field's behavior was intentional, and it's confusing to have a compiler flag with the same name but subtly different behavior.

@bridiver
Copy link

bridiver commented Apr 3, 2024

If that's the desired behavior, perhaps this flag should have a different name. I'd assume the original target config field's behavior was intentional, and it's confusing to have a compiler flag with the same name but subtly different behavior.

That makes sense and I'm not sure this is the root of either issue, but just wanted to mention that not all of these are strictly needed for our purposes. It could also be that the behavior should be different on macOS for both cases and it just never came up because there was no way to disable symbols on Mac before this change. So if the problem exists here, then it would also exist if you built rust with a custom configuration to hide symbols by default on macOS so it seems then that the flag isn't necessarily broken, but instead the underlying functionality is broken on macOS?

@jieyouxu jieyouxu added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-intrinsics Area: Intrinsics requires-nightly This issue requires a nightly compiler in some way. labels Apr 4, 2024
@chbaker0
Copy link
Contributor Author

chbaker0 commented Apr 4, 2024

Two tests that should pass, but fail with the flag: chbaker0@eec2e8e

@chbaker0
Copy link
Contributor Author

chbaker0 commented Apr 4, 2024

@alexcrichton the target spec field default-hidden-visibility was originally added by your pull request #47889

Was it intended to apply hidden visibility to rustc-generated references to memcmp, memcpy?

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 7, 2024
tgross35 added a commit to tgross35/rust that referenced this issue Aug 1, 2024
…petrochenkov

Use Default visibility for rustc-generated C symbol declarations

Previously, visibility for these symbols was determined by the `default-hidden-visibility` target option or the presence of `-Zdefault-hidden-visibility`. This leads to issue rust-lang#123427, where use of the flag leads to undefined hidden symbols (i.e., references that can never be resolved to an exported symbol from another shared library) for functions often provided by a platform shared library, such as `memcpy` and `memcmp` from `libc.so`.

References to symbols provided by shared libraries must have default visibility. Hidden visibility is mostly useful for _defined_ symbols.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 11, 2024
…vis, r=Urgau

Use Default visibility for rustc-generated C symbol declarations

Non-default visibilities should only be used for definitions, not declarations, otherwise linking can fail.

This is based on rust-lang#123994.

Issue rust-lang#123427

When I changed `default-hidden-visibility` to `default-visibility` in rust-lang#130005, I updated all places in the code that used `default-hidden-visibility`, replicating the hidden-visibility bug to also happen for protected visibility.

Without this change, trying to build rustc with `-Z default-visibility=protected` fails with a link error.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 11, 2024
Rollup merge of rust-lang#131519 - davidlattimore:intrinsics-default-vis, r=Urgau

Use Default visibility for rustc-generated C symbol declarations

Non-default visibilities should only be used for definitions, not declarations, otherwise linking can fail.

This is based on rust-lang#123994.

Issue rust-lang#123427

When I changed `default-hidden-visibility` to `default-visibility` in rust-lang#130005, I updated all places in the code that used `default-hidden-visibility`, replicating the hidden-visibility bug to also happen for protected visibility.

Without this change, trying to build rustc with `-Z default-visibility=protected` fails with a link error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intrinsics Area: Intrinsics A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants