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

Missed-optimization: extern "C" fn type calls are not nounwind #64090

Open
gnzlbg opened this issue Sep 2, 2019 · 14 comments
Open

Missed-optimization: extern "C" fn type calls are not nounwind #64090

gnzlbg opened this issue Sep 2, 2019 · 14 comments
Labels
A-FFI Area: Foreign function interface (FFI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 2, 2019

See https://rust.godbolt.org/z/9UvEyu

#![feature(unwind_attributes)]

extern "C" { 
    #[unwind(allow)] fn foo(); 
    // fn bar();
    static bar: extern "C" fn(); 
    static mut BAR: i32;
}

struct Foo;
impl Drop for Foo {
    fn drop(&mut self) {
        unsafe { BAR = 42; }
    }
}

pub unsafe fn unwind() { 
    let x = Foo;
    foo(); 
    std::mem::forget(x);
}
pub unsafe fn nounwind() { 
    let x = Foo;
    bar(); 
    std::mem::forget(x);
}

When the function extern "C" { fn bar(); } is called, the nounwind function is compiled to:

example::nounwind:
        jmpq    *bar@GOTPCREL(%rip)

However, when the function type static bar: extern "C" fn(); is called, this sub-optimal machine code is emitted:

example::nounwind:
        pushq   %rbx
        movq    bar@GOTPCREL(%rip), %rax
        callq   *(%rax)
        popq    %rbx
        retq
        movq    %rax, %rbx
        callq   core::ptr::real_drop_in_place
        movq    %rbx, %rdi
        callq   _Unwind_Resume@PLT
        ud2

This means that we can't call the large majority of C FFI functions, which cannot unwind, and all of the C++ noexcept functions, which cannot unwind either, from Rust function pointers efficiently.

@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2019

I don't think this is a bug. At least if we accept rust-lang/rfcs#2753, until we can track the unwinding ABI in fn ptr types, we cannot and should not assume that functions can't unwind based only on their call ABI.

Before changing our codegen for fn ptrs, IMO we should accept an RFC that basically says the opposite of rust-lang/rfcs#2753.

@nagisa
Copy link
Member

nagisa commented Sep 2, 2019

FWIW LLVM has no way of tracking "nounwind" for function pointers and since bar could be anything, LLVM cannot assume the pointer is nounwind.

@Centril Centril added A-FFI Area: Foreign function interface (FFI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Sep 2, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 6, 2019

@nagisa we can apply attributes to calls through function pointers, e.g., see how clang does it: https://rust.godbolt.org/z/YMv6NJ

@RalfJung That discussion belongs to the RFC PR.

@gnzlbg gnzlbg changed the title Sub-optimal code generation for extern "C" fn type calls Missed-optimization: extern "C" fn type calls are not nounwind Sep 6, 2019
@nagisa
Copy link
Member

nagisa commented Sep 6, 2019

@nagisa we can apply attributes to calls through function pointers, e.g., see how clang does it: https://rust.godbolt.org/z/YMv6NJ

The code generated by clang applies no attributes to its arguments in LLVM-IR, All that happens is clang carefully selecting whether to generate call or invoke during lowering to LLVM, which is different. Though it is true that Rust could do the same, but this then touches heavily upon how certain RFCs will go.

@Centril
Copy link
Contributor

Centril commented Sep 7, 2019

certain RFCs will go.

@nagisa specifically which ones?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 7, 2019

@nagisa See https://godbolt.org/z/7nnS_v, we currently optimize

extern "C" { fn foo(); } 
static FOO: unsafe extern "C" fn() = foo;
pub unsafe fn bar() { FOO() } 

to essentially this

extern "C" { fn foo(); } 
pub unsafe fn bar() { foo() } // bar is nounwind because foo is nounwind

This optimization is sound if the extern "C" ABI of the function pointer is the same as the extern "C" ABI of the function declaration. Since currently they don't unwind this allows us to remove the landing pads in bar and and make bar nounwind (the static is removed because it is private).

Declaring a function with the wrong ABI cannot probably be made UB since it is something that safe Rust code can do, calling an incorrect function declaration is definitely UB. Here, we never call the declaration, only the function pointer. If it were ok for the function pointer to unwind, then AFAICT at least two of the optimizations above (removing the landing pads and making bar nounwind) are unsound and we'd need to stop doing them.

So AFAICT we are already optimizing Rust code under the assumption that the extern "C" ABI of function pointers matches that of function declarations and definitions. We are just not as clever as clang when it comes to choosing call over invoke when we know that an ABI does not unwind.

@comex
Copy link
Contributor

comex commented Sep 8, 2019

Even if we had a separate extern "C+unwind" or similar, we’d probably translate calls to function pointers of that ABI to the same IR that we currently generate for extern "C". Thus, if you declared a function as extern "C", created a pointer to it, casted that to an extern "C+unwind" function pointer, and called the new pointer, you’d run into the same optimization.

As such, either:

  • The optimization is incorrect; or
  • If a function declaration has the wrong ABI, it’s UB not only to call the function directly, but also to call a function pointer derived from that declaration, even if the pointer is casted to the correct type first.

Either way, it seems like an orthogonal issue to whether or not extern "C" function pointers should be implicitly unwind-capable.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2019

So AFAICT we are already optimizing Rust code under the assumption that the extern "C" ABI of function pointers matches that of function declarations and definitions.

We are optimizing based on the assumption that calling a function through a function pointer behaves the same as if calling the function directly.
If we can figure out the function the pointer points to, and if that function is nounwind, that lets us assume that the fn ptr call is also nounwind. And that's what we see happening here.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2019

That discussion belongs to the RFC PR.

Well, right now we are (in our handling of fn ptrs) future compatible with extern "C" functions existing that can unwind. Given those RFCs, now is a bad time to remove that bit of future compatibility.

specifically which ones?

rust-lang/rfcs#2753, I imagine.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2019

Even if we had a separate extern "C+unwind" or similar, we’d probably translate calls to function pointers of that ABI to the same IR that we currently generate for extern "C". Thus, if you declared a function as extern "C", created a pointer to it, casted that to an extern "C+unwind" function pointer, and called the new pointer, you’d run into the same optimization.

I have no idea what you mean by this. Calling an extern "C+unwind" fn ptr would generate IR that can handle unwinding out of that call. What is the problem?

Unwinding from an extern "C" fn is UB (in a world with an "C+unwind" ABI), and the fact that we call an extern "C" fn via an extern "C+unwind" ptr changes nothing. In general, when the caller and callee signature differ, both signatures must be "correct" for the data that is actually used. For example, if you call a fn(bool) by transmuting the ptr to fn(u8) and passing in 0x04, that's UB because the callee expects a bool. But also, if you have a fn(u8) and transmute it to fn(bool) and then pass in 0x04, you produced a 0x04 at type bool so you caused UB. So for unwinding, if either the caller or callee signature says "no unwinding", then it is UB if unwinding happens.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 9, 2019

I have no idea what you mean by this. Calling an extern "C+unwind" fn ptr would generate IR that can handle unwinding out of that call. What is the problem?

If you do this:

extern "C" { fn foo(); } // Declared to never unwind
static BAR: extern "C+unwind" unsafe fn() = foo as _; // this cast is sound
unsafe fn bar() { BAR() } // BOOM: bar is optimized as nounwind

We will optimize bar under the assumption that foo does not unwind because foo is declared as nounwind, so if BAR() actually unwinds the behavior is undefined.

The declaration itself cannot be UB. The cast itself is IMO sound: functions that never unwind can definitely be called from function pointers that support unwinding.

So either this optimization is unsound or it is UB to call a function declared as not-unwinding via a function pointer that supports unwinding if that function actually unwinds.

IMO, this is just another flavor of:

mod bad { extern "C" { fn foo(); } } // never unwinds
extern "C+unwind" { fn foo(); }  // BOOM: becomes nounwind because of bad::foo

@RalfJung
Copy link
Member

So either this optimization is unsound or it is UB to call a function declared as not-unwinding via a function pointer that supports unwinding if that function actually unwinds.

That certainly is UB, as I said above.

It is UB for a function to unwind if at least one of the caller and callee signature says this may not unwind. In your case, the callee signature declared here

extern "C" { fn foo(); } // Declared to never unwind

says "cannot unwind", and hence this may not unwind, no matter the signature used by the caller.

@comex
Copy link
Contributor

comex commented Sep 11, 2019

But that's not the callee signature; it's only a signature we're using on the caller's end to declare the symbol. I'm assuming that the callee's actual implementation (which might be in another crate, or written in a different language altogether) is properly declared as unwinding.

@RalfJung
Copy link
Member

I would say it is the callee signature; if we have both declarations and a definition of the same symbol floating around and they are not the same then we are looking at rust-lang/unsafe-code-guidelines#198 which is an orthogonal issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants