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.

Sign up for GitHub

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

Jump to bottom

#[used(linker)] attribute #91504

Merged
merged 5 commits into from
Feb 10, 2022
Merged

#[used(linker)] attribute #91504

merged 5 commits into from
Feb 10, 2022

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Dec 4, 2021

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_gcc

cc @antoyo

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2021
@cynecx
Copy link
Contributor Author

cynecx commented Dec 4, 2021

r? @nikic

@rust-highfive rust-highfive assigned nikic and unassigned matthewjasper Dec 4, 2021
@nikic
Copy link
Contributor

nikic commented Dec 4, 2021

Generally okay with this addition, but I believe this should land under a feature gate first. There's at least a bit of design space here. For example, is #[used(retain)] the best way to expose this functionality? The "retain" terminology is an ELF-ism. Possibly distinguishing #[used] == #[used(compiler)] and #[used(linker)] or something like that would be more descriptive.

Of course this needs some tests, at least a codegen test for @llvm.used presence.

cc @nagisa

@cynecx
Copy link
Contributor Author

cynecx commented Dec 5, 2021

@nikic Is there a way to gate used(linker) and used(compiler) but keep used ungated? Or do we need a new attribute?

@camelid camelid added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-codegen Area: Code generation A-linkage Area: linking into static, shared libraries and binaries labels Dec 14, 2021
@nikic nikic mentioned this pull request Feb 5, 2022
Closed
@cynecx cynecx changed the title #[used(retain)] attribute #[used(linker)] attribute Feb 6, 2022
@cynecx
Copy link
Contributor Author

cynecx commented Feb 6, 2022

I've changed the attribute arg to linker and compiler as suggested by @nikic. Remaining things are tests...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
nikic
nikic reviewed Feb 6, 2022
View reviewed changes
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the attribute arg to linker and compiler as suggested by @nikic. Remaining things are tests...

Thanks for the update! Yes, I believe we need one test for the feature gate, and one codegen test that checks llvm.used/llvm.compiler.used is emitted in LLVM IR.

Can you please also open a tracking issue for the feature?

compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
nikic
nikic reviewed Feb 7, 2022
View reviewed changes
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like src/collect.rs will need a // ignore-tidy-filelength because you were the lucky person to cross an arbitrary threshold...

compiler/rustc_typeck/src/collect.rs Show resolved Hide resolved
src/test/codegen/used_with_arg.rs Outdated Show resolved Hide resolved
src/test/codegen/used_with_arg.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/consts.rs Show resolved Hide resolved
@cynecx
Copy link
Contributor Author

cynecx commented Feb 8, 2022

@nikic Thanks! I've addressed all your comments.

@cynecx cynecx marked this pull request as ready for review February 8, 2022 22:53
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@adetaylor adetaylor mentioned this pull request Feb 8, 2022
Open
@nikic nikic mentioned this pull request Feb 9, 2022
Open
5 tasks
@nikic
Copy link
Contributor

nikic commented Feb 9, 2022

I've opened a tracking issue and added it here.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 9, 2022

📌 Commit 1705933 has been approved by nikic

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2022
@cynecx
Copy link
Contributor Author

cynecx commented Feb 9, 2022

@nikic Ah, that got under my radar. Thanks for opening a tracking issue :)

@matthiaskrgr matthiaskrgr mentioned this pull request Feb 9, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 9, 2022
@matthiaskrgr
Rollup merge of rust-lang#91504 - cynecx:used_retain, r=nikic
93da8ca 
`#[used(linker)]` attribute

See dtolnay/linkme#41 (comment).
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2022
@bors
Auto merge of rust-lang#93836 - matthiaskrgr:rollup-d1ssiwl, r=matthi…
5d6ee0d 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#91443 (Better suggestions when user tries to collect into an unsized `[_]`)
 - rust-lang#91504 (`#[used(linker)]` attribute)
 - rust-lang#93503 (debuginfo: Fix DW_AT_containing_type vtable debuginfo regression)
 - rust-lang#93753 (Complete removal of #[main] attribute from compiler)
 - rust-lang#93799 (Fix typo in `std::fmt` docs)
 - rust-lang#93813 (Make a few cleanup MIR passes public)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3f4aaf4 into rust-lang:master Feb 10, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 10, 2022
@cynecx cynecx deleted the used_retain branch February 12, 2022 00:33
@bjorn3 bjorn3 mentioned this pull request Jan 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

@fbstj fbstj fbstj left review comments

Assignees

@nikic nikic

Labels
A-codegen Area: Code generation A-linkage Area: linking into static, shared libraries and binaries S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
9 participants
@cynecx @rust-highfive @nikic @rust-log-analyzer @bors @fbstj @matthewjasper @camelid @rustbot