Commit 9c169f3cf7

Andrew Kelley <superjoe30@gmail.com>
2018-09-08 02:09:33
C ABI: support returning large structs on x86_64
also panic instead of emitting bad code for returning small structs See #1481
1 parent 9017efe
src/all_types.hpp
@@ -40,6 +40,13 @@ struct Tld;
 struct TldExport;
 struct IrAnalyze;
 
+enum X64CABIClass {
+    X64CABIClass_Unknown,
+    X64CABIClass_MEMORY,
+    X64CABIClass_INTEGER,
+    X64CABIClass_SSE,
+};
+
 struct IrExecutable {
     ZigList<IrBasicBlock *> basic_block_list;
     Buf *name;
src/analyze.cpp
@@ -1009,10 +1009,6 @@ ZigType *get_bound_fn_type(CodeGen *g, ZigFn *fn_entry) {
     return bound_fn_type;
 }
 
-bool calling_convention_does_first_arg_return(CallingConvention cc) {
-    return cc == CallingConventionUnspecified;
-}
-
 const char *calling_convention_name(CallingConvention cc) {
     switch (cc) {
         case CallingConventionUnspecified: return "undefined";
@@ -1061,6 +1057,26 @@ ZigType *get_ptr_to_stack_trace_type(CodeGen *g) {
     return g->ptr_to_stack_trace_type;
 }
 
+bool want_first_arg_sret(CodeGen *g, FnTypeId *fn_type_id) {
+    if (fn_type_id->cc == CallingConventionUnspecified) {
+        return handle_is_ptr(fn_type_id->return_type);
+    }
+    if (fn_type_id->cc != CallingConventionC) {
+        return false;
+    }
+    if (type_is_c_abi_int(g, fn_type_id->return_type)) {
+        return false;
+    }
+    if (g->zig_target.arch.arch == ZigLLVM_x86_64) {
+        X64CABIClass abi_class = type_c_abi_x86_64_class(g, fn_type_id->return_type);
+        if (abi_class == X64CABIClass_MEMORY) {
+            return true;
+        }
+        zig_panic("TODO implement C ABI for x86_64 return types. '%s'", buf_ptr(&fn_type_id->return_type->name));
+    }
+    zig_panic("TODO implement C ABI for this architecture");
+}
+
 ZigType *get_fn_type(CodeGen *g, FnTypeId *fn_type_id) {
     Error err;
     auto table_entry = g->fn_type_table.maybe_get(fn_type_id);
@@ -1116,8 +1132,7 @@ ZigType *get_fn_type(CodeGen *g, FnTypeId *fn_type_id) {
     // next, loop over the parameters again and compute debug information
     // and codegen information
     if (!skip_debug_info) {
-        bool first_arg_return = calling_convention_does_first_arg_return(fn_type_id->cc) &&
-            handle_is_ptr(fn_type_id->return_type);
+        bool first_arg_return = want_first_arg_sret(g, fn_type_id);
         bool is_async = fn_type_id->cc == CallingConventionAsync;
         bool is_c_abi = fn_type_id->cc == CallingConventionC;
         bool prefix_arg_error_return_trace = g->have_err_ret_tracing && fn_type_can_fail(fn_type_id);
@@ -6367,3 +6382,85 @@ not_integer:
     }
     return nullptr;
 }
+
+X64CABIClass type_c_abi_x86_64_class(CodeGen *g, ZigType *ty) {
+    size_t ty_size = type_size(g, ty);
+    if (get_codegen_ptr_type(ty) != nullptr)
+        return X64CABIClass_INTEGER;
+    switch (ty->id) {
+        case ZigTypeIdEnum:
+        case ZigTypeIdInt:
+        case ZigTypeIdBool:
+            return X64CABIClass_INTEGER;
+        case ZigTypeIdFloat:
+            return X64CABIClass_SSE;
+        case ZigTypeIdStruct: {
+            // "If the size of an object is larger than four eightbytes, or it contains unaligned
+            // fields, it has class MEMORY"
+            if (ty_size > 32)
+                return X64CABIClass_MEMORY;
+            if (ty->data.structure.layout != ContainerLayoutExtern) {
+                // TODO determine whether packed structs have any unaligned fields
+                return X64CABIClass_Unknown;
+            }
+            // "If the size of the aggregate exceeds two eightbytes and the first eight-
+            // byte isn’t SSE or any other eightbyte isn’t SSEUP, the whole argument
+            // is passed in memory."
+            if (ty_size > 16) {
+                // Zig doesn't support vectors and large fp registers yet, so this will always
+                // be memory.
+                return X64CABIClass_MEMORY;
+            }
+            X64CABIClass working_class = X64CABIClass_Unknown;
+            for (uint32_t i = 0; i < ty->data.structure.src_field_count; i += 1) {
+                X64CABIClass field_class = type_c_abi_x86_64_class(g, ty->data.structure.fields->type_entry);
+                if (field_class == X64CABIClass_Unknown)
+                    return X64CABIClass_Unknown;
+                if (i == 0 || field_class == X64CABIClass_MEMORY || working_class == X64CABIClass_SSE) {
+                    working_class = field_class;
+                }
+            }
+            return working_class;
+        }
+        case ZigTypeIdUnion: {
+            // "If the size of an object is larger than four eightbytes, or it contains unaligned
+            // fields, it has class MEMORY"
+            if (ty_size > 32)
+                return X64CABIClass_MEMORY;
+            if (ty->data.unionation.layout != ContainerLayoutExtern)
+                return X64CABIClass_MEMORY;
+            // "If the size of the aggregate exceeds two eightbytes and the first eight-
+            // byte isn’t SSE or any other eightbyte isn’t SSEUP, the whole argument
+            // is passed in memory."
+            if (ty_size > 16) {
+                // Zig doesn't support vectors and large fp registers yet, so this will always
+                // be memory.
+                return X64CABIClass_MEMORY;
+            }
+            X64CABIClass working_class = X64CABIClass_Unknown;
+            for (uint32_t i = 0; i < ty->data.unionation.src_field_count; i += 1) {
+                X64CABIClass field_class = type_c_abi_x86_64_class(g, ty->data.unionation.fields->type_entry);
+                if (field_class == X64CABIClass_Unknown)
+                    return X64CABIClass_Unknown;
+                if (i == 0 || field_class == X64CABIClass_MEMORY || working_class == X64CABIClass_SSE) {
+                    working_class = field_class;
+                }
+            }
+            return working_class;
+        }
+        default:
+            return X64CABIClass_Unknown;
+    }
+}
+
+// NOTE this does not depend on x86_64
+bool type_is_c_abi_int(CodeGen *g, ZigType *ty) {
+    return (ty->id == ZigTypeIdInt ||
+        ty->id == ZigTypeIdFloat ||
+        ty->id == ZigTypeIdBool ||
+        ty->id == ZigTypeIdEnum ||
+        ty->id == ZigTypeIdVoid ||
+        ty->id == ZigTypeIdUnreachable ||
+        get_codegen_ptr_type(ty) != nullptr);
+}
+
src/analyze.hpp
@@ -182,7 +182,6 @@ size_t type_id_index(ZigType *entry);
 ZigType *get_generic_fn_type(CodeGen *g, FnTypeId *fn_type_id);
 Result<bool> type_is_copyable(CodeGen *g, ZigType *type_entry);
 LinkLib *create_link_lib(Buf *name);
-bool calling_convention_does_first_arg_return(CallingConvention cc);
 LinkLib *add_link_lib(CodeGen *codegen, Buf *lib);
 
 uint32_t get_abi_alignment(CodeGen *g, ZigType *type_entry);
@@ -211,6 +210,8 @@ bool calling_convention_allows_zig_types(CallingConvention cc);
 const char *calling_convention_name(CallingConvention cc);
 
 void walk_function_params(CodeGen *g, ZigType *fn_type, FnWalk *fn_walk);
-
+X64CABIClass type_c_abi_x86_64_class(CodeGen *g, ZigType *ty);
+bool type_is_c_abi_int(CodeGen *g, ZigType *ty);
+bool want_first_arg_sret(CodeGen *g, FnTypeId *fn_type_id);
 
 #endif
src/codegen.cpp
@@ -577,19 +577,25 @@ static LLVMValueRef fn_llvm_value(CodeGen *g, ZigFn *fn_table_entry) {
         // use the ABI alignment, which is fine.
     }
 
+    unsigned init_gen_i = 0;
     if (!type_has_bits(return_type)) {
         // nothing to do
     } else if (type_is_codegen_pointer(return_type)) {
         addLLVMAttr(fn_table_entry->llvm_value, 0, "nonnull");
-    } else if (handle_is_ptr(return_type) && calling_convention_does_first_arg_return(cc)) {
+    } else if (want_first_arg_sret(g, &fn_type->data.fn.fn_type_id)) {
         addLLVMArgAttr(fn_table_entry->llvm_value, 0, "sret");
         addLLVMArgAttr(fn_table_entry->llvm_value, 0, "nonnull");
+        if (cc == CallingConventionC) {
+            addLLVMArgAttr(fn_table_entry->llvm_value, 0, "noalias");
+        }
+        init_gen_i = 1;
     }
 
     // set parameter attributes
     FnWalk fn_walk = {};
     fn_walk.id = FnWalkIdAttrs;
     fn_walk.data.attrs.fn = fn_table_entry;
+    fn_walk.data.attrs.gen_i = init_gen_i;
     walk_function_params(g, fn_type, &fn_walk);
 
     uint32_t err_ret_trace_arg_index = get_err_ret_trace_arg_index(g, fn_table_entry);
@@ -1905,95 +1911,6 @@ static LLVMValueRef build_alloca(CodeGen *g, ZigType *type_entry, const char *na
     return result;
 }
 
-enum X64CABIClass {
-    X64CABIClass_Unknown,
-    X64CABIClass_MEMORY,
-    X64CABIClass_INTEGER,
-    X64CABIClass_SSE,
-};
-
-static X64CABIClass type_c_abi_x86_64_class(CodeGen *g, ZigType *ty) {
-    size_t ty_size = type_size(g, ty);
-    if (get_codegen_ptr_type(ty) != nullptr)
-        return X64CABIClass_INTEGER;
-    switch (ty->id) {
-        case ZigTypeIdEnum:
-        case ZigTypeIdInt:
-        case ZigTypeIdBool:
-            return X64CABIClass_INTEGER;
-        case ZigTypeIdFloat:
-            return X64CABIClass_SSE;
-        case ZigTypeIdStruct: {
-            // "If the size of an object is larger than four eightbytes, or it contains unaligned
-            // fields, it has class MEMORY"
-            if (ty_size > 32)
-                return X64CABIClass_MEMORY;
-            if (ty->data.structure.layout != ContainerLayoutExtern) {
-                // TODO determine whether packed structs have any unaligned fields
-                return X64CABIClass_Unknown;
-            }
-            // "If the size of the aggregate exceeds two eightbytes and the first eight-
-            // byte isn’t SSE or any other eightbyte isn’t SSEUP, the whole argument
-            // is passed in memory."
-            if (ty_size > 16) {
-                // Zig doesn't support vectors and large fp registers yet, so this will always
-                // be memory.
-                return X64CABIClass_MEMORY;
-            }
-            X64CABIClass working_class = X64CABIClass_Unknown;
-            for (uint32_t i = 0; i < ty->data.structure.src_field_count; i += 1) {
-                X64CABIClass field_class = type_c_abi_x86_64_class(g, ty->data.structure.fields->type_entry);
-                if (field_class == X64CABIClass_Unknown)
-                    return X64CABIClass_Unknown;
-                if (i == 0 || field_class == X64CABIClass_MEMORY || working_class == X64CABIClass_SSE) {
-                    working_class = field_class;
-                }
-            }
-            return working_class;
-        }
-        case ZigTypeIdUnion: {
-            // "If the size of an object is larger than four eightbytes, or it contains unaligned
-            // fields, it has class MEMORY"
-            if (ty_size > 32)
-                return X64CABIClass_MEMORY;
-            if (ty->data.unionation.layout != ContainerLayoutExtern)
-                return X64CABIClass_MEMORY;
-            // "If the size of the aggregate exceeds two eightbytes and the first eight-
-            // byte isn’t SSE or any other eightbyte isn’t SSEUP, the whole argument
-            // is passed in memory."
-            if (ty_size > 16) {
-                // Zig doesn't support vectors and large fp registers yet, so this will always
-                // be memory.
-                return X64CABIClass_MEMORY;
-            }
-            X64CABIClass working_class = X64CABIClass_Unknown;
-            for (uint32_t i = 0; i < ty->data.unionation.src_field_count; i += 1) {
-                X64CABIClass field_class = type_c_abi_x86_64_class(g, ty->data.unionation.fields->type_entry);
-                if (field_class == X64CABIClass_Unknown)
-                    return X64CABIClass_Unknown;
-                if (i == 0 || field_class == X64CABIClass_MEMORY || working_class == X64CABIClass_SSE) {
-                    working_class = field_class;
-                }
-            }
-            return working_class;
-        }
-        default:
-            return X64CABIClass_Unknown;
-    }
-}
-
-// NOTE this does not depend on x86_64
-static bool type_is_c_abi_int(CodeGen *g, ZigType *ty) {
-    size_t ty_size = type_size(g, ty);
-    if (ty_size > g->pointer_size_bytes)
-        return false;
-    return (ty->id == ZigTypeIdInt ||
-        ty->id == ZigTypeIdFloat ||
-        ty->id == ZigTypeIdBool ||
-        ty->id == ZigTypeIdEnum ||
-        get_codegen_ptr_type(ty) != nullptr);
-}
-
 static bool iter_function_params_c_abi(CodeGen *g, ZigType *fn_type, FnWalk *fn_walk, size_t src_i) {
     // Initialized from the type for some walks, but because of C var args,
     // initialized based on callsite instructions for that one.
@@ -2327,15 +2244,13 @@ static LLVMValueRef ir_render_return(CodeGen *g, IrExecutable *executable, IrIns
     LLVMValueRef value = ir_llvm_value(g, return_instruction->value);
     ZigType *return_type = return_instruction->value->value.type;
 
-    if (handle_is_ptr(return_type)) {
-        if (calling_convention_does_first_arg_return(g->cur_fn->type_entry->data.fn.fn_type_id.cc)) {
-            assert(g->cur_ret_ptr);
-            gen_assign_raw(g, g->cur_ret_ptr, get_pointer_to_type(g, return_type, false), value);
-            LLVMBuildRetVoid(g->builder);
-        } else {
-            LLVMValueRef by_val_value = gen_load_untyped(g, value, 0, false, "");
-            LLVMBuildRet(g->builder, by_val_value);
-        }
+    if (want_first_arg_sret(g, &g->cur_fn->type_entry->data.fn.fn_type_id)) {
+        assert(g->cur_ret_ptr);
+        gen_assign_raw(g, g->cur_ret_ptr, get_pointer_to_type(g, return_type, false), value);
+        LLVMBuildRetVoid(g->builder);
+    } else if (handle_is_ptr(return_type)) {
+        LLVMValueRef by_val_value = gen_load_untyped(g, value, 0, false, "");
+        LLVMBuildRet(g->builder, by_val_value);
     } else {
         LLVMBuildRet(g->builder, value);
     }
@@ -3551,8 +3466,7 @@ static LLVMValueRef ir_render_call(CodeGen *g, IrExecutable *executable, IrInstr
 
     CallingConvention cc = fn_type->data.fn.fn_type_id.cc;
 
-    bool first_arg_ret = ret_has_bits && handle_is_ptr(src_return_type) &&
-            calling_convention_does_first_arg_return(cc);
+    bool first_arg_ret = ret_has_bits && want_first_arg_sret(g, fn_type_id);
     bool prefix_arg_err_ret_stack = get_prefix_arg_err_ret_stack(g, fn_type_id);
     bool is_var_args = fn_type_id->is_var_args;
     ZigList<LLVMValueRef> gen_param_values = {};
@@ -6260,13 +6174,14 @@ static void do_code_gen(CodeGen *g) {
     // Generate function definitions.
     for (size_t fn_i = 0; fn_i < g->fn_defs.length; fn_i += 1) {
         ZigFn *fn_table_entry = g->fn_defs.at(fn_i);
-        CallingConvention cc = fn_table_entry->type_entry->data.fn.fn_type_id.cc;
+        FnTypeId *fn_type_id = &fn_table_entry->type_entry->data.fn.fn_type_id;
+        CallingConvention cc = fn_type_id->cc;
         bool is_c_abi = cc == CallingConventionC;
 
         LLVMValueRef fn = fn_llvm_value(g, fn_table_entry);
         g->cur_fn = fn_table_entry;
         g->cur_fn_val = fn;
-        ZigType *return_type = fn_table_entry->type_entry->data.fn.fn_type_id.return_type;
+        ZigType *return_type = fn_type_id->return_type;
         if (handle_is_ptr(return_type)) {
             g->cur_ret_ptr = LLVMGetParam(fn, 0);
         } else {
@@ -6344,13 +6259,15 @@ static void do_code_gen(CodeGen *g) {
 
         ImportTableEntry *import = get_scope_import(&fn_table_entry->fndef_scope->base);
 
+        unsigned gen_i_init = want_first_arg_sret(g, fn_type_id) ? 1 : 0;
+
         // create debug variable declarations for variables and allocate all local variables
         FnWalk fn_walk_var = {};
         fn_walk_var.id = FnWalkIdVars;
         fn_walk_var.data.vars.import = import;
         fn_walk_var.data.vars.fn = fn_table_entry;
         fn_walk_var.data.vars.llvm_fn = fn;
-        fn_walk_var.data.vars.gen_i = 0;
+        fn_walk_var.data.vars.gen_i = gen_i_init;
         for (size_t var_i = 0; var_i < fn_table_entry->variable_list.length; var_i += 1) {
             ZigVar *var = fn_table_entry->variable_list.at(var_i);
 
@@ -6429,6 +6346,7 @@ static void do_code_gen(CodeGen *g) {
         fn_walk_init.id = FnWalkIdInits;
         fn_walk_init.data.inits.fn = fn_table_entry;
         fn_walk_init.data.inits.llvm_fn = fn;
+        fn_walk_init.data.inits.gen_i = gen_i_init;
         walk_function_params(g, fn_table_entry->type_entry, &fn_walk_init);
 
         ir_render(g, fn_table_entry);
test/cases/bugs/1230.zig
@@ -1,14 +0,0 @@
-const assert = @import("std").debug.assert;
-
-const S = extern struct {
-    x: i32,
-};
-
-extern fn ret_struct() S {
-    return S{ .x = 42 };
-}
-
-test "extern return small struct (bug 1230)" {
-    const s = ret_struct();
-    assert(s.x == 42);
-}
test/stage1/c_abi/build.zig
@@ -5,6 +5,7 @@ pub fn build(b: *Builder) void {
 
     const c_obj = b.addCObject("cfuncs", "cfuncs.c");
     c_obj.setBuildMode(rel_opts);
+    c_obj.setNoStdLib(true);
 
     const main = b.addTest("main.zig");
     main.setBuildMode(rel_opts);
test/stage1/c_abi/cfuncs.c
@@ -59,6 +59,8 @@ struct SplitStructInts {
 };
 void zig_split_struct_ints(struct SplitStructInts);
 
+struct BigStruct zig_big_struct_both(struct BigStruct);
+
 void run_c_tests(void) {
     zig_u8(0xff);
     zig_u16(0xfffe);
@@ -77,8 +79,7 @@ void run_c_tests(void) {
 
     zig_bool(true);
 
-    // TODO making this non-static crashes for some reason
-    static uint8_t array[10] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0'};
+    uint8_t array[10] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0'};
     zig_array(array);
 
     {
@@ -95,6 +96,16 @@ void run_c_tests(void) {
         struct SplitStructInts s = {1234, 100, 1337};
         zig_split_struct_ints(s);
     }
+
+    {
+        struct BigStruct s = {30, 31, 32, 33, 34};
+        struct BigStruct res = zig_big_struct_both(s);
+        assert_or_panic(res.a == 20);
+        assert_or_panic(res.b == 21);
+        assert_or_panic(res.c == 22);
+        assert_or_panic(res.d == 23);
+        assert_or_panic(res.e == 24);
+    }
 }
 
 void c_u8(uint8_t x) {
@@ -185,3 +196,13 @@ void c_split_struct_ints(struct SplitStructInts x) {
     assert_or_panic(x.b == 100);
     assert_or_panic(x.c == 1337);
 }
+
+struct BigStruct c_big_struct_both(struct BigStruct x) {
+    assert_or_panic(x.a == 1);
+    assert_or_panic(x.b == 2);
+    assert_or_panic(x.c == 3);
+    assert_or_panic(x.d == 4);
+    assert_or_panic(x.e == 5);
+    struct BigStruct y = {10, 11, 12, 13, 14};
+    return y;
+}
test/stage1/c_abi/main.zig
@@ -203,3 +203,37 @@ export fn zig_split_struct_ints(x: SplitStructInt) void {
     assertOrPanic(x.b == 100);
     assertOrPanic(x.c == 1337);
 }
+
+extern fn c_big_struct_both(BigStruct) BigStruct;
+
+test "C ABI sret and byval together" {
+    var s = BigStruct{
+        .a = 1,
+        .b = 2,
+        .c = 3,
+        .d = 4,
+        .e = 5,
+    };
+    var y = c_big_struct_both(s);
+    assertOrPanic(y.a == 10);
+    assertOrPanic(y.b == 11);
+    assertOrPanic(y.c == 12);
+    assertOrPanic(y.d == 13);
+    assertOrPanic(y.e == 14);
+}
+
+export fn zig_big_struct_both(x: BigStruct) BigStruct {
+    assertOrPanic(x.a == 30);
+    assertOrPanic(x.b == 31);
+    assertOrPanic(x.c == 32);
+    assertOrPanic(x.d == 33);
+    assertOrPanic(x.e == 34);
+    var s = BigStruct{
+        .a = 20,
+        .b = 21,
+        .c = 22,
+        .d = 23,
+        .e = 24,
+    };
+    return s;
+}
test/behavior.zig
@@ -9,7 +9,6 @@ comptime {
     _ = @import("cases/bitcast.zig");
     _ = @import("cases/bool.zig");
     _ = @import("cases/bugs/1111.zig");
-    _ = @import("cases/bugs/1230.zig");
     _ = @import("cases/bugs/1277.zig");
     _ = @import("cases/bugs/1421.zig");
     _ = @import("cases/bugs/394.zig");