Use Default visibility for rustc-generated C symbol declarations

[chbaker0]

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 #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.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[chbaker0]

tagging @alexcrichton because their wasm additions introduced the original default-hidden-visibility field. I'm still unsure of all the nuances of how it should work for wasm.

[alexcrichton]

At the time the wasm target was added I vaguely recall that this was basically required to get the target working. For example it was just a quirk that was wasm-specific and wasn't intended to affect any other target. I don't recall the exact reason why it was necessary, but I vaguely recall it being required as otherwise all symbols were always exported from all wasm binaries. I just tried a local compilation with RUSTFLAGS=-Zdefault-hidden-visibility=no for wasm and it only exported the symbols I expected to be exported. Given that it should be ok to just flat out remove this option if it's causing problems.

[chbaker0]

We do actually want the command line flag for another use case: controlling symbol export when the final link step is not performed by rustc. Related: rust-lang/compiler-team#656 and #73295

If you build a staticlib or rlib without this option, then link it with C code into a shared object, all Rust symbols are visible in the SO. This is an issue if Rust is an internal implementation detail of the SO, and especially if you have more than one SO linked with Rust code.

[alexcrichton]

Ah ok makes sense! I can clarify then that at least for wasm-related stuff it's ok to disgregard this, I'll send a parallel PR to disable this option for wasm instead of enable it by default.

[lcnr]

r? compiler

[petrochenkov]

This looks like a reasonable default for foreign functions.

The change have very little impact on unsuspecting users now because none of the built-in targets use default_hidden_visibility now (wasm was migrated off it), and -Zdefault-hidden-visibility is unstable and opt-in.

r=me after considering #123994 (comment).
@rustbot author

[chbaker0]

Sorry I sat on this so long. Not sure if I should go ahead and submit, so I'll wait

[chbaker0]

@rustbot ready

[petrochenkov]

@bors r+

[bors]

📌 Commit a1c27e3 has been approved by petrochenkov

It is now in the queue for this repository.

[tgross35]

@bors r-

Looks like this failed in #128519 (comment)

[chbaker0]

The failure happens when targeting wasm32-wasip1, and the rustc output simply indicates that dylib is unsupported on that platform.

The test should only run on platforms where dylibs are supported. I'm not sure how to accomplish that with compiletest. I added an ignore-wasm32 directive but now that's just playing whack-a-mole with platforms that don't have dylib support.

What do other tests do? I can't find examples.

@rustbot ready

[petrochenkov]

The failure happens when targeting wasm32-wasip1, and the rustc output simply indicates that dylib is unsupported on that platform.

The test should only run on platforms where dylibs are supported. I'm not sure how to accomplish that with compiletest. I added an ignore-wasm32 directive but now that's just playing whack-a-mole with platforms that don't have dylib support.

What do other tests do? I can't find examples.

@rustbot ready

The //@ needs-dynamic-linking directive seems to be what you need.
@rustbot author

[chbaker0]

@rustbot ready

//@ needs-dynamic-linking does not work for wasm32-wasip1; it runs anyway. I assume this is a quirk or bug related to that target specifically. The latest revision includes both that directive and //@ ignore-wasm32

[petrochenkov]

Ah, ok, the target supports dynamic linking, but only cdylibs, not dylib crates.
All wasm targets and also nvptx64-nvidia-cuda have this property.
So the right directives are

//@ needs-dynamic-linking
//@ ignore-wasm
//@ ignore-nvptx64

@rustbot author

[bors]

☔ The latest upstream changes (presumably #131111) made this pull request unmergeable. Please resolve the merge conflicts.

[davidlattimore]

My change - #130005 - caused conflicts with this PR. @chbaker0 - I'm happy to pick this up unless you wanted to?

[Dylan-DPC]

@chbaker0 any updates on this? thanks

[danakj]

@davidlattimore I think it's okay for you to pick this up, thanks for offering.

[danakj]

Oh, looks like it's done already in #131519

[bjorn3]

This PR can be closed, right?

[davidlattimore]

From my perspective, it's fine to close