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

Share the naked asm impl between cg_ssa and cg_clif #134232

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Dec 12, 2024

This was introduced in #128004.

@bjorn3 bjorn3 added the A-cranelift Things relevant to the [future] cranelift backend label Dec 12, 2024
@bjorn3
Copy link
Member Author

bjorn3 commented Dec 12, 2024

cc @folkertdev

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 12, 2024
@rust-log-analyzer

This comment has been minimized.

@bjorn3 bjorn3 force-pushed the naked_asm_improvements branch 2 times, most recently from b8677a2 to d6b3c06 Compare December 13, 2024 08:55
@rust-log-analyzer

This comment has been minimized.

@bjorn3 bjorn3 force-pushed the naked_asm_improvements branch from d6b3c06 to 0dd4292 Compare December 13, 2024 09:30
@bjorn3 bjorn3 marked this pull request as ready for review December 13, 2024 09:30
@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@bjorn3 bjorn3 added A-inline-assembly Area: Inline assembly (`asm!(…)`) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS F-naked_functions `#![feature(naked_functions)]` labels Dec 13, 2024
Comment on lines -214 to +239
if let Visibility::Hidden = item_data.visibility {
writeln!(begin, ".hidden {asm_name}").unwrap();
match item_data.visibility {
Visibility::Default => {}
Visibility::Protected => writeln!(begin, ".protected {asm_name}").unwrap(),
Visibility::Hidden => writeln!(begin, ".hidden {asm_name}").unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we discussed documenting https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/mono/enum.Visibility.html a while ago, now might be a good time for that? (you had some ideas for that I believe)?

Also I don't think .hidden and .protected are ever hit by tests, is there any way we could reach them?

Copy link
Member Author

@bjorn3 bjorn3 Dec 13, 2024

Choose a reason for hiding this comment

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

I think we discussed documenting https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/mono/enum.Visibility.html a while ago, now might be a good time for that? (you had some ideas for that I believe)?

Will open a separate PR for that. Edit: #134261

Also I don't think .hidden and .protected are ever hit by tests, is there any way we could reach them?

You can set the visibility for all symbols using -Zdefault-visibility. Also I'm not sure if using .protected or .hidden in case of internal linkage will cause the symbol to be exported anyway, but that is a pre-existing issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that's fine, we just want to make sure that naked functions behave like standard functions

@bors
Copy link
Contributor

bors commented Dec 17, 2024

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

@bjorn3 bjorn3 force-pushed the naked_asm_improvements branch from 42ae211 to 1358f53 Compare December 18, 2024 17:13
@bjorn3 bjorn3 force-pushed the naked_asm_improvements branch from 1358f53 to dcf8b87 Compare January 9, 2025 17:36
@chenyukang
Copy link
Member

r? compiler

@rustbot rustbot assigned estebank and unassigned chenyukang Jan 12, 2025
@bors
Copy link
Contributor

bors commented Jan 24, 2025

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

While LLVM is rather permissive in this regards, some other codegen
backends demand that once you declare a function for definition you
actually define contents of the function, which doesn't happen for naked
functions as we actually generate assembly for them.
@bjorn3 bjorn3 force-pushed the naked_asm_improvements branch from dcf8b87 to 0b43964 Compare January 24, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cranelift Things relevant to the [future] cranelift backend A-inline-assembly Area: Inline assembly (`asm!(…)`) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS F-naked_functions `#![feature(naked_functions)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants