Unwinding through `fn()` references across crates causes the behavior to depend on the optimization level

[mahkoh]

In a.rs

#![crate_type = "lib"]

pub struct X(pub u8);

impl Drop for X {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

pub fn f(x: &mut X, g: fn()) {
    x.0 = 1;
    g();
    x.0 = 0;
}

In b.rs

extern crate a;

fn main() {
    a::f(&mut a::X(0), g);
}

fn g() {
    panic!();
}

$ rustc -O a.rs && rustc -L . b.rs && ./b
thread '<main>' panicked at 'explicit panic', b.rs:8
0
$ rustc a.rs && rustc -L . b.rs && ./b 
thread '<main>' panicked at 'explicit panic', b.rs:8
1

$ rustc -V
rustc 1.6.0-nightly (2e07996a9 2015-10-29)

NB: Some pre-1.0 nightlies already behaved this way.

[alexcrichton]

Looks like a codegen bug related to the noalias attribute we place on the IR. It also looks like this isn't necessarily related to a function pointer. The codegen for this function is a little messy but when cleaned up it looks like:

%X = type { i8, i8 }                                  

declare void @foo()                                   

define void @f(%X* noalias) {                         
  %x = alloca %X*                                     
  store %X* %0, %X** %x                               
  %2 = load %X*, %X** %x                              
  %3 = getelementptr inbounds %X, %X* %2, i32 0, i32 0
  store i8 1, i8* %3                                  
  call void @foo()                                    
  %4 = load %X*, %X** %x                              
  %5 = getelementptr inbounds %X, %X* %4, i32 0, i32 0
  store i8 0, i8* %5                                  
  ret void                                            
}                                                     

When optimized, we get:

%X = type { i8, i8 }

declare void @foo()

define void @f(%X* noalias nocapture) {
  %2 = getelementptr inbounds %X, %X* %0, i64 0, i32 0
  tail call void @foo()
  store i8 0, i8* %2, align 1
  ret void
}

And as we can see, the store of 1 has been optimized away. If we instead remove the noalias attribute on the pointer, the store remains

%X = type { i8, i8 }

declare void @foo()

define void @f(%X* nocapture) {
  %2 = getelementptr inbounds %X, %X* %0, i64 0, i32 0
  store i8 1, i8* %2, align 1
  tail call void @foo()
  store i8 0, i8* %2, align 1
  ret void
}

cc @dotdash, perhaps we can't place noalias on &mut? That seems unfortunate, but I guess there's also a possibility this is a bug in LLVM (but seems unlikely).

[Gankra]

I'm no expert, but isn't the bug here that we're emitting call instead of invoke?

AFAIK marking &mut's noalias is correct. It seems to me that LLVM is using the fact that a call can't throw to determine that the first store is unobservable. If we emited invoke it would be forced to conservatively assume that g can throw and perform the store.

I'm wrong.

[eefriedman]

An LLVM IR call is semantically the same as an invoke with a trivial landing pad.

There's a decent argument that LLVM's treatment of noalias is wrong. Consider the following C++ (GNU-dialect) testcase:

#include <iostream>
using namespace std;

void xx() { 
    throw 1;
}

void (*volatile g)() = xx;

void f(int* __restrict x) {
    *x = 1;
    g();
    *x = 0;
}

int main() {
    int x = 0;
    try { f(&x); } catch(...) {}
    std::cout << x;
    return 0;
}

gcc prints 1; clang prints 0.

On the other hand, you're likely to run into resistance proposing any restriction on how alias analysis can reason about noalias pointers.

[sunfishcode]

This isn't just a noalias issue; it can occur with other kinds of alias information as well. For example, the first store gets deleted in this code too, even though the call to foo may unwind.

declare void @foo()

@g = external global i8

define void @f() {
  store i8 1, i8* @g
  call void @foo() readnone
  store i8 0, i8* @g
  ret void
}

MemoryDependenceAnalysis in LLVM currently says that the second store here has a direct local dependence on the first store. There's a sense in which that is a correct answer, but DeadStoreElimination inteprets this answer to mean that the first store is dead. It's not immediately obvious which of these two should be fixed.

call having an implicit control path requires special handling in a other passes too (eg., the inliner has to know how to convert call into invoke if it's being inlined into a context where it'll have a cleanup), so it'd be cool if LLVM had something like an invoke with only one successor, which the optimizer could treat like a kind of conditional return, however, that's a much bigger change.

[nikomatsakis]

triage: P-high

[nikomatsakis]

Is the problem here that LLVM is optimizing too heavily, or that we are providing incorrect annotations?

Either way, it may be prudent to supply fewer annotations for the time being. That'd certainly be an easy patch to do.

[Gankra]

I asked sunfish to clarify on IRC, and he believes this is 100% an llvm bug.

[sunfishcode]

I've now filed an LLVM bug: https://llvm.org/bugs/show_bug.cgi?id=25422

[nikomatsakis]

We were discussing in the compiler team meeting. Do we expect LLVM to fix this in a short time frame? I would think probably not -- we were thinking therefore that it would be prudent to workaround in the time being. Would this mean removing the aliasing annotations and thus taking some speed hit? Are there other solutions? Other implications?

[eefriedman]

Do we expect LLVM to fix this in a short time frame?

No; fixing DSE is probably easy enough... but trusting noalias to work correctly alongside unwinding would probably require auditing every use of alias analysis in LLVM (for example, I'm pretty sure LICM is also broken). In any case, no matter how fast this is fixed, Rust can't unconditionally depend on any fix because we support linking against 3.7.

Would this mean removing the aliasing annotations and thus taking some speed hit?

Yes.

Are there other solutions?

No.

Other implications?

I don't think so.