Add new unstable attribute: #[export_visibility = ...].

[anforowicz]

View all comments

This PR is an implementation of the RFC tracked in #151425

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
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

[Kivooeo]

i had a quick look, mostly looks good, but i'd like to maybe @JonathanBrouwer take a look on this as well, i may overlooked something

r? JonathanBrouwer

[JonathanBrouwer]

Would like to take a look, will do so tomorrow :)

[JonathanBrouwer]

Some minor questions, looks good on a high level :)

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rust-bors]

💔 Test for 814edc7 failed: CI. Failed job:

• try - test-various (web logs, enhanced plaintext logs)

[bjorn3]

exported_dynamic_symbol_names doesn't work on wasm. It uses object::File::exports, which isn't implemented yet for wasm.

[anforowicz]

object::File::exports, which isn't implemented yet for wasm.

Thanks! I think you are referring to gimli-rs/object#471.

I think for this PR we have 2 options:

• Land the tests in the current shape - the tests will start failing once object supports parsing .wasm exports.
  • Pros: the test failure will mean somebody will have to tweak the test to cover the right Wasm behavior
  • Cons: having to tweak the test will make it more difficult to roll/update the object crate used by Rust project
    • To make this easier I can add comments to the new rmake test. Not sure how much it would help.
• MY PROPOSAL: Add a test directive to ignore the new remake test when targeting wasm
  • Pros: simpler test code in the new test
  • Cons: no wasm coverage + no auto-incentive in the future to fix the tests once object supports parsing .wasm exports

Any feedback?

[bjorn3]

I think you are referring to gimli-rs/object#471.

Not quite. That is for wasm relocatable object files. The issue in this PR is that also for linked wasm modules exports aren't listed.

[anforowicz]

I can repro the test failures from #151431 (comment) by running ./x.py test --target nvptx64-nvidia-cuda tests/run-make/cdylib-export-visibility on a Linux host.

I can fix cannot find macro "line" in this scope by using #![no_std] in tests/run-make/cdylib-export-visibility/foo.rs.

I don't know how to fix "#[panic_handler]" function required, but not found. I tried using #![no_core] but it seems that //@ add-minicore doesn't work in remake tests (?), so I gave up.

Maybe I should consider adding //@needs-target-std to the new rmake test?

[anforowicz]

I don't know how to fix "#[panic_handler]" function required, but not found. I tried using #![no_core] but it seems that //@ add-minicore doesn't work in remake tests (?), so I gave up.

Maybe I should consider adding //@needs-target-std to the new rmake test?

Hmmm... actually, I guess I can define a no-op panic handler. But this still makes the test fail when targeting nvptx64-nvidia-cuda. The new error is: linker llvm-bitcode-linker not found.

The test probably needs to skip/ignore this target architecture, but I am not quite sure how to accomplish this (after skimming through https://rustc-dev-guide.rust-lang.org/tests/directives.html?highlight=%40ignore#controlling-when-tests-are-run)... :-/

[anforowicz]

The test probably needs to skip/ignore this target architecture, but I am not quite sure how to accomplish this (after skimming through https://rustc-dev-guide.rust-lang.org/tests/directives.html?highlight=%40ignore#controlling-when-tests-are-run)... :-/

FWIW tests/run-make/cdylib/rmake.rs, tests/run-make/cdylib-export-c-library-symbols/rmake.rs, tests/run-make/cdylib-dylib-linkage/rmake.rs etc. say //@ ignore-cross-compile. Maybe I should also consider this in my new test?

[alexcrichton]

For wasm what I'd recommend, assuming you're ok with it, is to indeed skip this test on wasm. I think it'd be best to discuss the wasm-specific parts in a follow-up issue perhaps which tracks the wasm testing for this and what would be necessary for that.

[anforowicz]

For wasm what I'd recommend, assuming you're ok with it, is to indeed skip this test on wasm. I think it'd be best to discuss the wasm-specific parts in a follow-up issue perhaps which tracks the wasm testing for this and what would be necessary for that.

Thanks for the feedback! I update the PR to ignore Wasm targets in the new rmake test.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[anforowicz]

✌️ @anforowicz, you can now approve this pull request!

I hope this means that it is okay if I do:

@bors r+ rollup=iffy

[rust-bors]

📌 Commit b12629f has been approved by anforowicz

It is now in the queue for this repository.