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

codegen #[naked] functions using global asm #128004

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

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Jul 20, 2024

tracking issue: #90957

Fixes #124375

This implements the approach suggested in the tracking issue: use the existing global assembly infrastructure to emit the body of #[naked] functions. The main advantage is that we now have full control over what gets generated, and are no longer dependent on LLVM not sneakily messing with our output (inlining, adding extra instructions, etc).

I discussed this approach with @Amanieu and while I think the general direction is correct, there is probably a bunch of stuff that needs to change or move around here. I'll leave some inline comments on things that I'm not sure about.

Combined with #127853, if both accepted, I think that resolves all steps from the tracking issue.

r? @Amanieu

@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 Jul 20, 2024
compiler/rustc_codegen_ssa/src/codegen_attrs.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/naked_asm.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/naked_asm.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/naked_asm.rs Outdated Show resolved Hide resolved
tests/codegen/naked-fn/x86_64-linux.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor

On the tracking issue a naked_asm! macro was proposed that would closer follow global_asm! (cc @Lokathor since you liked this idea). It sounds like this PR effectively turns #[naked] + asm! into exactly what naked_asm would do, so that would no longer be necessary?

The changes in this PR seem like good direction.

@Lokathor
Copy link
Contributor

I think that separately from any internal implementation change, the surface syntax of rust should use naked_asm separately from asm, because the two have different enough user interface and semantics.

@folkertdev
Copy link
Contributor Author

I agree that it is still a good idea to add naked_asm! as a public api, if only because it makes the documentation much more straightforward: instead of having to explain the interaction between #[naked] and asm!, we just mandate that #[naked] must use a naked_asm! block and the naked_asm! docs can give the exact details and restrictions.

But that is separate from how the codegen works, which is what this PR is for.

Comment on lines +182 to +174
if let Visibility::Hidden = item_data.visibility {
writeln!(begin, ".hidden {asm_name}").unwrap();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not been able to actually generate code that triggers this if. Visibility just seems to always be Default. So this is entirely untested at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

For anything like this, drop a question on Zulip https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp. Unfortunately that enum doesn't seem to be very well documented

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

}

fn inline_to_global_operand<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
cx: &'a Bx::CodegenCx,
Copy link
Member

Choose a reason for hiding this comment

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

This function can probably just take TyCtxt.

cx.tcx(),
value.span,
const_value,
cx.layout_of(value.ty()),
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this layout_of call uses the CodegenCx. This is during codegen though, so RevealAllLayoutCx (a wrapper around TyCtxt) is enough: https://github.com/rust-lang/rustc_codegen_cranelift/blob/b70ad2defd4bb5fba6af7958893e22be0f33dfdd/src/common.rs#L450-L518 Maybe it should be uplifted out of cg_clif?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems useful, should that be part of this PR though?

@folkertdev

This comment was marked as resolved.

@folkertdev folkertdev marked this pull request as ready for review July 21, 2024 15:05
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Jul 26, 2024
@rustbot

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the naked-fn-asm branch 2 times, most recently from f54a458 to 6783e26 Compare July 26, 2024 17:32
@bors
Copy link
Contributor

bors commented Jul 28, 2024

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

@bors
Copy link
Contributor

bors commented Jul 29, 2024

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

tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
…isibility, r=bjorn3

add test for symbol visibility of `#[naked]` functions

tracking issue: rust-lang#90957

This test is extracted from rust-lang#128004

That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration.

In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function.

Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress.

Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something.

r? `@bjorn3`
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
…isibility, r=bjorn3

add test for symbol visibility of `#[naked]` functions

tracking issue: rust-lang#90957

This test is extracted from rust-lang#128004

That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration.

In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function.

Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress.

Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something.

r? ``@bjorn3``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2024
Rollup merge of rust-lang#128362 - folkertdev:naked-function-symbol-visibility, r=bjorn3

add test for symbol visibility of `#[naked]` functions

tracking issue: rust-lang#90957

This test is extracted from rust-lang#128004

That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration.

In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function.

Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress.

Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something.

r? ``@bjorn3``
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

I think this is as far as I can take it without some external feedback

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2024
Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I'm still not 100% sure about the linkage-related things, but I think it's good enough to merge. We can handle any issues that are discovered later.

Copy link
Member

Choose a reason for hiding this comment

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

Also test that at the end of each function the assember mode is restored to the default for the target.

Copy link
Contributor Author

@folkertdev folkertdev Sep 5, 2024

Choose a reason for hiding this comment

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

what are these assembler modes based on? I don't see .arm here at all, and for thumb only thumb_function appears to be used.

https://godbolt.org/z/rjjc4KxnG

and even then only at the start, not at the end

        .section        .text.test_thumb,"ax",%progbits
        .globl  test_thumb
        .p2align        1
        .type   test_thumb,%function
        .code   16
        .thumb_func
test_thumb:
.Lfunc_begin0:
        .fnstart
        .cfi_sections .debug_frame
        .cfi_startproc
        .file   1 "/app" "example.rs"
        .loc    1 19 5 prologue_end
        bx      lr
        .inst.n 0xdefe
.Lfunc_end0:
        .size   test_thumb, .Lfunc_end0-test_thumb
        .cfi_endproc
        .cantunwind
        .fnend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is confusing, the assert is triggered in this example

    let is_thumb = tcx.sess.unstable_target_features.contains(&sym::thumb_mode);

    let attrs = tcx.codegen_fn_attrs(instance.def_id());
    let link_section = attrs.link_section.map(|symbol| symbol.as_str().to_string());
    let align = attrs.alignment.map(|a| a.bytes()).unwrap_or(4);

    let (arch_prefix, arch_suffix) = if is_arm {
        (
            match attrs.instruction_set {
                None => match is_thumb {
                    true => ".thumb\n.thumb_func",
                    false => ".arm",
                },
                Some(InstructionSetAttr::ArmT32) => {
                    assert!(is_thumb);
                    ".thumb\n.thumb_func"
                }

that conflicts with

/// Computes the set of target features used in a function for the purposes of
/// inline assembly.
fn asm_target_features(tcx: TyCtxt<'_>, did: DefId) -> &FxIndexSet<Symbol> {
    let mut target_features = tcx.sess.unstable_target_features.clone();
    if tcx.def_kind(did).has_codegen_attrs() {
        let attrs = tcx.codegen_fn_attrs(did);
        target_features.extend(attrs.target_features.iter().map(|feature| feature.name));
        match attrs.instruction_set {
            None => {}
            Some(InstructionSetAttr::ArmA32) => {
                // FIXME(#120456) - is `swap_remove` correct?
                target_features.swap_remove(&sym::thumb_mode);
            }
            Some(InstructionSetAttr::ArmT32) => {
                target_features.insert(sym::thumb_mode);
            }
        }
    }

    tcx.arena.alloc(target_features)
}

so information is getting lost somewhere I think?

compiler/rustc_codegen_ssa/src/mir/naked_asm.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/naked_asm.rs Outdated Show resolved Hide resolved
first steps of codegen for `#[naked]` functions using `global_asm!`

configure external linkage if no linkage is explicitly set

create a `FunctionCx` and properly evaluate consts

inline attribute is no longer relevant for naked functions

the naked attribute no longer needs to be set by llvm/...

we're generating global asm now, so this attribute is meaningless for the codegen backend
correctly emit `.hidden`

this test was added in rust-lang#105193

but actually NO_COVERAGE is no longer a thing in the compiler. Sadly,
the link to the issue is broken, so I don't know what the problem was
originally, but I don't think this is relevant any more with the global
asm approach

rename test file

because it now specifically checks for directives only used by
non-macos, non-windows x86_64

add codegen tests for 4 interesting platforms

add codegen test for the `#[instruction_set]` attribute

add test for `#[link_section]`

use `tcx.codegen_fn_attrs` to get attribute info

Fix rust-lang#124375

inline const monomorphization/evaluation

getting rid of FunctionCx

mark naked functions as `InstantiatedMode::GloballyShared`

this makes sure that the function prototype is defined correctly, and we don't see LLVM complaining about a global value with invalid linkage

monomorphize type given to `SymFn`

remove hack that always emits `.globl`

monomorphize type given to `Const`

remove `linkage_directive`

make naked functions always have external linkage

mark naked functions as `#[inline(never)]`

add test file for functional generics/const/impl/trait usage of naked functions
… it earlier, then some other logic causes invalid visibility for the item (exporting when it shouldn't).
- better way of checking for thumb
- get the mangled name from the codegen backend
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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
Status: In progress
Development

Successfully merging this pull request may close these issues.

ICE: codegen: index out of bounds: the len is 3 but the index is 4
9 participants