From 9c169f3cf763c58f91b485f907f80cd8d08c4e12 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Fri, 7 Sep 2018 20:09:33 -0400 Subject: [PATCH] C ABI: support returning large structs on x86_64 also panic instead of emitting bad code for returning small structs See #1481 --- src/all_types.hpp | 7 ++ src/analyze.cpp | 109 +++++++++++++++++++++++++++++-- src/analyze.hpp | 5 +- src/codegen.cpp | 126 +++++++----------------------------- test/behavior.zig | 1 - test/cases/bugs/1230.zig | 14 ---- test/stage1/c_abi/build.zig | 1 + test/stage1/c_abi/cfuncs.c | 25 ++++++- test/stage1/c_abi/main.zig | 34 ++++++++++ 9 files changed, 193 insertions(+), 129 deletions(-) delete mode 100644 test/cases/bugs/1230.zig diff --git a/src/all_types.hpp b/src/all_types.hpp index b80fe17053..6adb53b774 100644 --- a/src/all_types.hpp +++ b/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 basic_block_list; Buf *name; diff --git a/src/analyze.cpp b/src/analyze.cpp index 0fe83e0d96..1c49f696bf 100644 --- a/src/analyze.cpp +++ b/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); +} + diff --git a/src/analyze.hpp b/src/analyze.hpp index 98e4412594..cd3f52d9c5 100644 --- a/src/analyze.hpp +++ b/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 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 diff --git a/src/codegen.cpp b/src/codegen.cpp index 4be08b03e7..71caf18e28 100644 --- a/src/codegen.cpp +++ b/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 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); diff --git a/test/behavior.zig b/test/behavior.zig index 6b35b0c030..25997a2a4b 100644 --- a/test/behavior.zig +++ b/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"); diff --git a/test/cases/bugs/1230.zig b/test/cases/bugs/1230.zig deleted file mode 100644 index b782a77f0b..0000000000 --- a/test/cases/bugs/1230.zig +++ /dev/null @@ -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); -} diff --git a/test/stage1/c_abi/build.zig b/test/stage1/c_abi/build.zig index 02db8f7904..e0a108aeb5 100644 --- a/test/stage1/c_abi/build.zig +++ b/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); diff --git a/test/stage1/c_abi/cfuncs.c b/test/stage1/c_abi/cfuncs.c index ff249d2a59..102cd466b1 100644 --- a/test/stage1/c_abi/cfuncs.c +++ b/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; +} diff --git a/test/stage1/c_abi/main.zig b/test/stage1/c_abi/main.zig index e1356f50ef..196311a283 100644 --- a/test/stage1/c_abi/main.zig +++ b/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; +}