-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
On the tracking issue a The changes in this PR seem like good direction. |
I think that separately from any internal implementation change, the surface syntax of rust should use |
I agree that it is still a good idea to add But that is separate from how the codegen works, which is what this PR is for. |
if let Visibility::Hidden = item_data.visibility { | ||
writeln!(begin, ".hidden {asm_name}").unwrap(); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
fn inline_to_global_operand<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | ||
cx: &'a Bx::CodegenCx, |
There was a problem hiding this comment.
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()), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
f54a458
to
6783e26
Compare
☔ The latest upstream changes (presumably #128298) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts. |
…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`
…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``
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``
12eb028
to
7fc596f
Compare
This comment has been minimized.
This comment has been minimized.
7fc596f
to
14c5df6
Compare
Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
This comment has been minimized.
This comment has been minimized.
14c5df6
to
eafb371
Compare
This comment has been minimized.
This comment has been minimized.
I think this is as far as I can take it without some external feedback @rustbot ready |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
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
40049b0
to
c22bd19
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #130066) made this pull request unmergeable. Please resolve the merge conflicts. |
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