auto merge of #14121 : luqmana/rust/option-ffi, r=alexcrichton

This slightly adjusts the NullablePointer representation for some enums in the case where the non-nullable variant has a single field (the ptr field) to be just that, the pointer. This is in contrast to the current behaviour where we'd wrap that single pointer in a LLVM struct.

Fixes #11040 & #11303.

Author: bors

src/librustc/middle/trans/adt.rs

@@ -85,6 +85,18 @@ pub enum Repr {
      * all start with a field for the discriminant.
      */
     General(IntType, Vec<Struct>),
+    /**
+     * Two cases distinguished by a nullable pointer: the case with discriminant
+     * `nndiscr` must have single field which is known to be nonnull due to its type.
+     * The other case is known to be zero sized. Hence we represent the enum
+     * as simply a nullable pointer: if not null it indicates the `nndiscr` variant,
+     * otherwise it indicates the other case.
+     */
+    RawNullablePointer {
+        pub nndiscr: Disr,
+        pub nnty: ty::t,
+        pub nullfields: Vec<ty::t>
+    },
     /**
      * Two cases distinguished by a nullable pointer: the case with discriminant
      * `nndiscr` is represented by the struct `nonnull`, where the `ptrfield`th
@@ -96,7 +108,7 @@ pub enum Repr {
      * is represented such that `None` is a null pointer and `Some` is the
      * identity function.
      */
-    NullablePointer {
+    StructWrappedNullablePointer {
         pub nonnull: Struct,
         pub nndiscr: Disr,
         pub ptrfield: uint,
@@ -200,17 +212,23 @@ fn represent_type_uncached(cx: &CrateContext, t: ty::t) -> Repr {
                     if cases.get(1 - discr).is_zerolen(cx) {
                         match cases.get(discr).find_ptr() {
                             Some(ptrfield) => {
-                                return NullablePointer {
-                                    nndiscr: discr as u64,
-                                    nonnull: mk_struct(cx,
-                                                       cases.get(discr)
-                                                            .tys
-                                                            .as_slice(),
-                                                       false),
-                                    ptrfield: ptrfield,
-                                    nullfields: cases.get(1 - discr).tys
-                                                                    .clone()
-                                }
+                                let st = mk_struct(cx, cases.get(discr).tys.as_slice(),
+                                                   false);
+
+                                return if st.fields.len() == 1 {
+                                    RawNullablePointer {
+                                        nndiscr: discr as Disr,
+                                        nnty: *st.fields.get(0),
+                                        nullfields: cases.get(1 - discr).tys.clone()
+                                    }
+                                } else {
+                                    StructWrappedNullablePointer {
+                                        nndiscr: discr as Disr,
+                                        nonnull: st,
+                                        ptrfield: ptrfield,
+                                        nullfields: cases.get(1 - discr).tys.clone()
+                                    }
+                                };
                             }
                             None => { }
                         }
@@ -413,8 +431,8 @@ pub fn incomplete_type_of(cx: &CrateContext, r: &Repr, name: &str) -> Type {
 }
 pub fn finish_type_of(cx: &CrateContext, r: &Repr, llty: &mut Type) {
     match *r {
-        CEnum(..) | General(..) => { }
-        Univariant(ref st, _) | NullablePointer{ nonnull: ref st, .. } =>
+        CEnum(..) | General(..) | RawNullablePointer { .. } => { }
+        Univariant(ref st, _) | StructWrappedNullablePointer { nonnull: ref st, .. } =>
             llty.set_struct_body(struct_llfields(cx, st, false).as_slice(),
                                  st.packed)
     }
@@ -423,7 +441,8 @@ pub fn finish_type_of(cx: &CrateContext, r: &Repr, llty: &mut Type) {
 fn generic_type_of(cx: &CrateContext, r: &Repr, name: Option<&str>, sizing: bool) -> Type {
     match *r {
         CEnum(ity, _, _) => ll_inttype(cx, ity),
-        Univariant(ref st, _) | NullablePointer{ nonnull: ref st, .. } => {
+        RawNullablePointer { nnty, .. } => type_of::sizing_type_of(cx, nnty),
+        Univariant(ref st, _) | StructWrappedNullablePointer { nonnull: ref st, .. } => {
             match name {
                 None => {
                     Type::struct_(cx, struct_llfields(cx, st, sizing).as_slice(),
@@ -495,12 +514,10 @@ fn struct_llfields(cx: &CrateContext, st: &Struct, sizing: bool) -> Vec<Type> {
 pub fn trans_switch(bcx: &Block, r: &Repr, scrutinee: ValueRef)
     -> (_match::branch_kind, Option<ValueRef>) {
     match *r {
-        CEnum(..) | General(..) => {
+        CEnum(..) | General(..) |
+        RawNullablePointer { .. } | StructWrappedNullablePointer { .. } => {
             (_match::switch, Some(trans_get_discr(bcx, r, scrutinee, None)))
         }
-        NullablePointer{ nonnull: ref nonnull, nndiscr, ptrfield, .. } => {
-            (_match::switch, Some(nullable_bitdiscr(bcx, nonnull, nndiscr, ptrfield, scrutinee)))
-        }
         Univariant(..) => {
             (_match::single, None)
         }
@@ -528,8 +545,14 @@ pub fn trans_get_discr(bcx: &Block, r: &Repr, scrutinee: ValueRef, cast_to: Opti
             val = C_u8(bcx.ccx(), 0);
             signed = false;
         }
-        NullablePointer{ nonnull: ref nonnull, nndiscr, ptrfield, .. } => {
-            val = nullable_bitdiscr(bcx, nonnull, nndiscr, ptrfield, scrutinee);
+        RawNullablePointer { nndiscr, nnty, .. } =>  {
+            let cmp = if nndiscr == 0 { IntEQ } else { IntNE };
+            let llptrty = type_of::sizing_type_of(bcx.ccx(), nnty);
+            val = ICmp(bcx, cmp, Load(bcx, scrutinee), C_null(llptrty));
+            signed = false;
+        }
+        StructWrappedNullablePointer { nonnull: ref nonnull, nndiscr, ptrfield, .. } => {
+            val = struct_wrapped_nullable_bitdiscr(bcx, nonnull, nndiscr, ptrfield, scrutinee);
             signed = false;
         }
     }
@@ -539,10 +562,10 @@ pub fn trans_get_discr(bcx: &Block, r: &Repr, scrutinee: ValueRef, cast_to: Opti
     }
 }
 
-fn nullable_bitdiscr(bcx: &Block, nonnull: &Struct, nndiscr: Disr, ptrfield: uint,
-                     scrutinee: ValueRef) -> ValueRef {
-    let cmp = if nndiscr == 0 { IntEQ } else { IntNE };
+fn struct_wrapped_nullable_bitdiscr(bcx: &Block, nonnull: &Struct, nndiscr: Disr, ptrfield: uint,
+                                    scrutinee: ValueRef) -> ValueRef {
     let llptr = Load(bcx, GEPi(bcx, scrutinee, [0, ptrfield]));
+    let cmp = if nndiscr == 0 { IntEQ } else { IntNE };
     let llptrty = type_of::type_of(bcx.ccx(), *nonnull.fields.get(ptrfield));
     ICmp(bcx, cmp, llptr, C_null(llptrty))
 }
@@ -590,7 +613,8 @@ pub fn trans_case<'a>(bcx: &'a Block<'a>, r: &Repr, discr: Disr)
         Univariant(..) => {
             bcx.ccx().sess().bug("no cases for univariants or structs")
         }
-        NullablePointer{ .. } => {
+        RawNullablePointer { .. } |
+        StructWrappedNullablePointer { .. } => {
             assert!(discr == 0 || discr == 1);
             _match::single_result(Result::new(bcx, C_i1(bcx.ccx(), discr != 0)))
         }
@@ -621,7 +645,13 @@ pub fn trans_start_init(bcx: &Block, r: &Repr, val: ValueRef, discr: Disr) {
         Univariant(..) => {
             assert_eq!(discr, 0);
         }
-        NullablePointer{ nonnull: ref nonnull, nndiscr, ptrfield, .. } => {
+        RawNullablePointer { nndiscr, nnty, ..} => {
+            if discr != nndiscr {
+                let llptrty = type_of::sizing_type_of(bcx.ccx(), nnty);
+                Store(bcx, C_null(llptrty), val)
+            }
+        }
+        StructWrappedNullablePointer { nonnull: ref nonnull, nndiscr, ptrfield, .. } => {
             if discr != nndiscr {
                 let llptrptr = GEPi(bcx, val, [0, ptrfield]);
                 let llptrty = type_of::type_of(bcx.ccx(),
@@ -651,8 +681,11 @@ pub fn num_args(r: &Repr, discr: Disr) -> uint {
             st.fields.len() - (if dtor { 1 } else { 0 })
         }
         General(_, ref cases) => cases.get(discr as uint).fields.len() - 1,
-        NullablePointer{ nonnull: ref nonnull, nndiscr,
-                         nullfields: ref nullfields, .. } => {
+        RawNullablePointer { nndiscr, ref nullfields, .. } => {
+            if discr == nndiscr { 1 } else { nullfields.len() }
+        }
+        StructWrappedNullablePointer { nonnull: ref nonnull, nndiscr,
+                                       nullfields: ref nullfields, .. } => {
             if discr == nndiscr { nonnull.fields.len() } else { nullfields.len() }
         }
     }
@@ -675,19 +708,25 @@ pub fn trans_field_ptr(bcx: &Block, r: &Repr, val: ValueRef, discr: Disr,
         General(_, ref cases) => {
             struct_field_ptr(bcx, cases.get(discr as uint), val, ix + 1, true)
         }
-        NullablePointer{ nonnull: ref nonnull, nullfields: ref nullfields,
-                         nndiscr, .. } => {
-            if discr == nndiscr {
-                struct_field_ptr(bcx, nonnull, val, ix, false)
-            } else {
-                // The unit-like case might have a nonzero number of unit-like fields.
-                // (e.g., Result or Either with () as one side.)
-                let ty = type_of::type_of(bcx.ccx(), *nullfields.get(ix));
-                assert_eq!(machine::llsize_of_alloc(bcx.ccx(), ty), 0);
-                // The contents of memory at this pointer can't matter, but use
-                // the value that's "reasonable" in case of pointer comparison.
-                PointerCast(bcx, val, ty.ptr_to())
-            }
+        RawNullablePointer { nndiscr, ref nullfields, .. } |
+        StructWrappedNullablePointer { nndiscr, ref nullfields, .. } if discr != nndiscr => {
+            // The unit-like case might have a nonzero number of unit-like fields.
+            // (e.d., Result of Either with (), as one side.)
+            let ty = type_of::type_of(bcx.ccx(), *nullfields.get(ix));
+            assert_eq!(machine::llsize_of_alloc(bcx.ccx(), ty), 0);
+            // The contents of memory at this pointer can't matter, but use
+            // the value that's "reasonable" in case of pointer comparision.
+            PointerCast(bcx, val, ty.ptr_to())
+        }
+        RawNullablePointer { nndiscr, nnty, .. } => {
+            assert_eq!(ix, 0);
+            assert_eq!(discr, nndiscr);
+            let ty = type_of::type_of(bcx.ccx(), nnty);
+            PointerCast(bcx, val, ty.ptr_to())
+        }
+        StructWrappedNullablePointer { ref nonnull, nndiscr, .. } => {
+            assert_eq!(discr, nndiscr);
+            struct_field_ptr(bcx, nonnull, val, ix, false)
         }
     }
 }
@@ -759,7 +798,15 @@ pub fn trans_const(ccx: &CrateContext, r: &Repr, discr: Disr,
             let contents = build_const_struct(ccx, st, vals);
             C_struct(ccx, contents.as_slice(), st.packed)
         }
-        NullablePointer{ nonnull: ref nonnull, nndiscr, .. } => {
+        RawNullablePointer { nndiscr, nnty, .. } => {
+            if discr == nndiscr {
+                assert_eq!(vals.len(), 1);
+                vals[0]
+            } else {
+                C_null(type_of::sizing_type_of(ccx, nnty))
+            }
+        }
+        StructWrappedNullablePointer { nonnull: ref nonnull, nndiscr, .. } => {
             if discr == nndiscr {
                 C_struct(ccx, build_const_struct(ccx,
                                                  nonnull,
@@ -867,7 +914,15 @@ pub fn const_get_discrim(ccx: &CrateContext, r: &Repr, val: ValueRef)
             }
         }
         Univariant(..) => 0,
-        NullablePointer{ nndiscr, ptrfield, .. } => {
+        RawNullablePointer { nndiscr, .. } => {
+            if is_null(val) {
+                /* subtraction as uint is ok because nndiscr is either 0 or 1 */
+                (1 - nndiscr) as Disr
+            } else {
+                nndiscr
+            }
+        }
+        StructWrappedNullablePointer { nndiscr, ptrfield, .. } => {
             if is_null(const_struct_field(ccx, val, ptrfield)) {
                 /* subtraction as uint is ok because nndiscr is either 0 or 1 */
                 (1 - nndiscr) as Disr
@@ -891,7 +946,11 @@ pub fn const_get_field(ccx: &CrateContext, r: &Repr, val: ValueRef,
         CEnum(..) => ccx.sess().bug("element access in C-like enum const"),
         Univariant(..) => const_struct_field(ccx, val, ix),
         General(..) => const_struct_field(ccx, val, ix + 1),
-        NullablePointer{ .. } => const_struct_field(ccx, val, ix)
+        RawNullablePointer { .. } => {
+            assert_eq!(ix, 0);
+            val
+        }
+        StructWrappedNullablePointer{ .. } => const_struct_field(ccx, val, ix)
     }
 }
 

src/librustc/middle/trans/debuginfo.rs

@@ -1675,7 +1675,10 @@ fn prepare_enum_metadata(cx: &CrateContext,
                 }),
             }
         }
-        adt::NullablePointer { nonnull: ref struct_def, nndiscr, .. } => {
+        adt::RawNullablePointer { nnty, .. } => {
+            FinalMetadata(type_metadata(cx, nnty, span))
+        }
+        adt::StructWrappedNullablePointer { nonnull: ref struct_def, nndiscr, .. } => {
             let (metadata_stub,
                  variant_llvm_type,
                  member_description_factory) =

src/test/debuginfo/option-like-enum.rs

@@ -16,10 +16,10 @@
 // gdb-command:finish
 
 // gdb-command:print some
-// gdb-check:$1 = {0x12345678}
+// gdb-check:$1 = (u32 *) 0x12345678
 
 // gdb-command:print none
-// gdb-check:$2 = {0x0}
+// gdb-check:$2 = (u32 *) 0x0
 
 // gdb-command:print full
 // gdb-check:$3 = {454545, 0x87654321, 9988}
@@ -41,6 +41,8 @@
 // contains a non-nullable pointer, then this value is used as the discriminator.
 // The test cases in this file make sure that something readable is generated for
 // this kind of types.
+// If the non-empty variant contains a single non-nullable pointer than the whole
+// item is represented as just a pointer and not wrapped in a struct.
 // Unfortunately (for these test cases) the content of the non-discriminant fields
 // in the null-case is not defined. So we just read the discriminator field in
 // this case (by casting the value to a memory-equivalent struct).

src/test/debuginfo/recursive-struct.rs

@@ -20,53 +20,53 @@
 
 // gdb-command:print stack_unique.value
 // gdb-check:$1 = 0
-// gdb-command:print stack_unique.next.val->value
+// gdb-command:print stack_unique.next->value
 // gdb-check:$2 = 1
 
 // gdb-command:print unique_unique->value
 // gdb-check:$3 = 2
-// gdb-command:print unique_unique->next.val->value
+// gdb-command:print unique_unique->next->value
 // gdb-check:$4 = 3
 
 // gdb-command:print box_unique->val.value
 // gdb-check:$5 = 4
-// gdb-command:print box_unique->val.next.val->value
+// gdb-command:print box_unique->val.next->value
 // gdb-check:$6 = 5
 
 // gdb-command:print vec_unique[0].value
 // gdb-check:$7 = 6.5
-// gdb-command:print vec_unique[0].next.val->value
+// gdb-command:print vec_unique[0].next->value
 // gdb-check:$8 = 7.5
 
 // gdb-command:print borrowed_unique->value
 // gdb-check:$9 = 8.5
-// gdb-command:print borrowed_unique->next.val->value
+// gdb-command:print borrowed_unique->next->value
 // gdb-check:$10 = 9.5
 
 // MANAGED
 // gdb-command:print stack_managed.value
 // gdb-check:$11 = 10
-// gdb-command:print stack_managed.next.val->val.value
+// gdb-command:print stack_managed.next.val->value
 // gdb-check:$12 = 11
 
 // gdb-command:print unique_managed->value
 // gdb-check:$13 = 12
-// gdb-command:print unique_managed->next.val->val.value
+// gdb-command:print unique_managed->next.val->value
 // gdb-check:$14 = 13
 
-// gdb-command:print box_managed->val.value
+// gdb-command:print box_managed.val->value
 // gdb-check:$15 = 14
-// gdb-command:print box_managed->val.next.val->val.value
+// gdb-command:print box_managed->val->next.val->value
 // gdb-check:$16 = 15
 
 // gdb-command:print vec_managed[0].value
 // gdb-check:$17 = 16.5
-// gdb-command:print vec_managed[0].next.val->val.value
+// gdb-command:print vec_managed[0].next.val->value
 // gdb-check:$18 = 17.5
 
 // gdb-command:print borrowed_managed->value
 // gdb-check:$19 = 18.5
-// gdb-command:print borrowed_managed->next.val->val.value
+// gdb-command:print borrowed_managed->next.val->value
 // gdb-check:$20 = 19.5
 
 // LONG CYCLE
@@ -97,7 +97,7 @@
 // gdb-command:print (*****long_cycle_w_anonymous_types).value
 // gdb-check:$31 = 30
 
-// gdb-command:print (*****((*****long_cycle_w_anonymous_types).next.val)).value
+// gdb-command:print (*****((*****long_cycle_w_anonymous_types).next)).value
 // gdb-check:$32 = 31
 
 // gdb-command:continue

src/test/run-pass/nullable-pointer-ffi-compat.rs

@@ -0,0 +1,35 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// #11303, #11040:
+// This would previously crash on i686 linux due to abi differences
+// between returning an Option<T> and T, where T is a non nullable
+// pointer.
+// If we have an enum with two variants such that one is zero sized
+// and the other contains a nonnullable pointer, we don't use a
+// separate discriminant. Instead we use that pointer field to differentiate
+// between the 2 cases.
+// Also, if the variant with the nonnullable pointer has no other fields
+// then we simply express the enum as just a pointer and not wrap it
+// in a struct.
+
+use std::mem;
+
+#[inline(never)]
+extern "C" fn foo<'a>(x: &'a int) -> Option<&'a int> { Some(x) }
+
+static FOO: int = 0xDEADBEE;
+
+pub fn main() {
+    unsafe {
+        let f: extern "C" fn<'a>(&'a int) -> &'a int = mem::transmute(foo);
+        assert_eq!(*f(&FOO), FOO);
+    }
+}