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

Naked functions with parameters generate load/store instructions in debug mode #34043

Closed
philpax opened this issue Jun 2, 2016 · 16 comments
Closed
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@philpax
Copy link
Contributor

philpax commented Jun 2, 2016

#[naked]
unsafe extern "C" fn test(a: *const c_void, b: *mut c_void) {
    unreachable!();
}

results in

; Function Attrs: naked uwtable
define internal void @_ZN8rust_out4test17h785f8ce455c67198E(i8*, i8*) unnamed_addr #0 !dbg !30 {
entry-block:
  %a = alloca i8*
  %b = alloca i8*
  %const = alloca %str_slice
  %2 = alloca { %str_slice, i32 }*
  store i8* %0, i8** %a
  call void @llvm.dbg.declare(metadata i8** %a, metadata !93, metadata !94), !dbg !95
  store i8* %1, i8** %b
  call void @llvm.dbg.declare(metadata i8** %b, metadata !96, metadata !94), !dbg !95
  %3 = bitcast %str_slice* %const to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %3, i8* bitcast (%str_slice* @const7081 to i8*), i64 16, i32 8, i1 false)
  %4 = getelementptr inbounds %str_slice, %str_slice* %const, i32 0, i32 0
  %5 = load i8*, i8** %4
  %6 = getelementptr inbounds %str_slice, %str_slice* %const, i32 0, i32 1
  %7 = load i64, i64* %6
  store { %str_slice, i32 }* bitcast ({ %str_slice, i32, [4 x i8] }* @_ZN8rust_out4test10_FILE_LINE17h08e1dfa3dee60bceE to { %str_slice, i32 }*), { %str_slice, i32 }** %2, !dbg !97
  %8 = load { %str_slice, i32 }*, { %str_slice, i32 }** %2, !dbg !97, !nonnull !37
  call void @_ZN3std9panicking11begin_panic17h4ebf9fe884b2415fE(i8* noalias nonnull readonly %5, i64 %7, { %str_slice, i32 }* noalias readonly dereferenceable(24) %8), !dbg !102
  unreachable, !dbg !102
}

Most of these instructions should not be present, and result in undesired codegen prior to any user code (i.e. assembly).

@philpax
Copy link
Contributor Author

philpax commented Jun 2, 2016

CC @ticki

@ticki
Copy link
Contributor

ticki commented Jun 2, 2016

Seems to be a result of unwinding.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jun 2, 2016

Yes, the unreachable!() generates a whole lot of code related to unwinding. However, if I replace it with intrinsics::unreachable(), the IR still contains redundant loads and stores:

; Function Attrs: naked uwtable
define void @naked_fn(i8*, i8*) unnamed_addr #0 !dbg !10 {
entry-block:
  %a = alloca i8*
  %b = alloca i8*
  store i8* %0, i8** %a
  call void @llvm.dbg.declare(metadata i8** %a, metadata !19, metadata !20), !dbg !21
  store i8* %1, i8** %b
  call void @llvm.dbg.declare(metadata i8** %b, metadata !22, metadata !20), !dbg !21
  unreachable, !dbg !23
}

(Both this and the IR @philpax posted are in debug mode)

@ticki
Copy link
Contributor

ticki commented Jun 2, 2016

That's even more interesting.

@eddyb
Copy link
Member

eddyb commented Jun 2, 2016

The current implementation doesn't really support arguments though, a correct implementation would enforce that they don't need allocas (which... depends on ABI and MIR optimization passes).

@nagisa nagisa added I-wrong A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS labels Jun 2, 2016
@ticki
Copy link
Contributor

ticki commented Jun 3, 2016

@eddyb Yeah. It should preferably error when given arguments.

@philpax
Copy link
Contributor Author

philpax commented Jun 3, 2016

Hmm; naked functions without arguments seem rather limited in scope. Would that just be a temporary limitation?

@ticki
Copy link
Contributor

ticki commented Jun 3, 2016

@philpax You can still pass arguments through inline assembly. In the long term, it should support arguments, though.

@chasewalden
Copy link

I don't mean to bump an old thread, but I am curious on the state of this.

I have come across this issue as well, but it only seems to appear in debug builds, not in release builds, although this could be a result of the optimizer.

@eddyb
Copy link
Member

eddyb commented Mar 22, 2017

@Chaser53 It's not entirely. You can compile your code without debuginfo and it should be better.
We should make an effort to use llvm.dbg.value which doesn't require alloca to annotate variables.

I should mention I already hit a gdb bug in #40367 (which is only a minor improvement), so that's not encouraging, but I'll try to get a C reproduction for it and/or work around it.

@gmorenz
Copy link

gmorenz commented Jun 5, 2017

Just ran into this. The generated code results in undefined behaviour if you ever call a naked function with arguments compiled with -g on x64 linux. The following was compiled with rustc -g --crate-type=cdylib tmp.rs.

#![feature(asm, naked_functions)]

#[naked]
#[no_mangle]
pub unsafe extern "C" fn naked(_arg: u64) {
    asm!("ret" :::: "intel");
}

#[no_mangle]
pub extern "C" fn non_naked(_arg: u64) {
    return;
}
00000000000004f0 <naked>:
 4f0:   48 89 7d f0             mov    %rdi,-0x10(%rbp)
 4f4:   48 8b 7d f0             mov    -0x10(%rbp),%rdi
 4f8:   48 89 7d f8             mov    %rdi,-0x8(%rbp)
 4fc:   c3                      retq
 4fd:   c3                      retq
 4fe:   66 90                   xchg   %ax,%ax

0000000000000500 <non_naked>:
 500:   55                      push   %rbp
 501:   48 89 e5                mov    %rsp,%rbp
 504:   48 89 7d f0             mov    %rdi,-0x10(%rbp)
 508:   48 8b 7d f0             mov    -0x10(%rbp),%rdi
 50c:   48 89 7d f8             mov    %rdi,-0x8(%rbp)
 510:   5d                      pop    %rbp
 511:   c3                      retq

The naked function immediately starts moving stuff to rbp. Under the System V abi rbp is just another general purpose register, it could be null or anything else. Compare to the non naked function which first sets rbp to the stack pointer before pushing stuff.

@eddyb
Copy link
Member

eddyb commented Jun 18, 2017

The IR with -g is:

; Function Attrs: naked nounwind uwtable
define void @naked(i64) unnamed_addr #0 !dbg !4 {
start:
  %_arg = alloca i64
  %arg0 = alloca i64
  store i64 %0, i64* %arg0
  call void @llvm.dbg.declare(metadata i64* %arg0, metadata !10, metadata !11), !dbg !12
  call void @llvm.dbg.declare(metadata i64* %_arg, metadata !13, metadata !11), !dbg !15
  %1 = load i64, i64* %arg0, !dbg !16
  store i64 %1, i64* %_arg, !dbg !16
  call void asm inteldialect "ret", "~{dirflag},~{fpsr},~{flags}"(), !dbg !17, !srcloc !18
  ret void, !dbg !19
}

; Function Attrs: nounwind uwtable
define void @non_naked(i64) unnamed_addr #1 !dbg !20 {
start:
  %_arg = alloca i64
  %arg0 = alloca i64
  store i64 %0, i64* %arg0
  call void @llvm.dbg.declare(metadata i64* %arg0, metadata !21, metadata !11), !dbg !22
  call void @llvm.dbg.declare(metadata i64* %_arg, metadata !23, metadata !11), !dbg !25
  %1 = load i64, i64* %arg0, !dbg !26
  store i64 %1, i64* %_arg, !dbg !26
  ret void, !dbg !27
}

Without -g it's:

; Function Attrs: naked nounwind uwtable
define void @naked(i64) unnamed_addr #0 {
start:
  call void asm inteldialect "ret", "~{dirflag},~{fpsr},~{flags}"(), !srcloc !0
  ret void
}

; Function Attrs: nounwind uwtable
define void @non_naked(i64) unnamed_addr #1 {
start:
  ret void
}

This means that we have to switch from llvm.dbg.declare to llvm.dbg.value for variables.

@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. and removed I-wrong labels Jul 25, 2017
@steveklabnik
Copy link
Member

Triage: https://godbolt.org/z/WwgiGH

looks like maybe this has been fixed?

example::test:
        call    std::panicking::begin_panic
        ud2

@eddyb
Copy link
Member

eddyb commented Apr 6, 2020

@steveklabnik this issue is about debug mode, and I think we don't have issues without debuginfo anymore, it's just debuginfo that still causes code to be generated.

An easy fix I didn't consider years ago would've been to "just" not emit debuginfo in such functions.

@eddyb eddyb changed the title Naked functions with parameters generate load/store instructions Naked functions with parameters generate load/store instructions in debug mode Apr 6, 2020
@npmccallum
Copy link
Contributor

This should be fixed here: #74105

@jonas-schievink jonas-schievink added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 6, 2020
@jonas-schievink
Copy link
Contributor

Closing as fixed. Please let us know if this is still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests