From fc5826b7ca2695b7441b1564440b48d731c3f934 Mon Sep 17 00:00:00 2001 From: Delyan Angelov Date: Thu, 22 Dec 2022 21:47:39 +0200 Subject: [PATCH] cgen: minimise `sizeof(EmptyStruct)` to 0 for gcc/clang and to 1 for tcc/msvc, by changing EMPTY_STRUCT_DECLARATION and EMPTY_STRUCT_INITIALIZATION (#16733) --- vlib/arrays/arrays_test.v | 2 +- vlib/builtin/array.v | 18 +++---- vlib/builtin/builtin.c.v | 10 ++++ vlib/builtin/map.v | 4 +- vlib/builtin/map_d_gcboehm_opt.v | 22 ++++---- vlib/v/gen/c/array.v | 6 +-- vlib/v/gen/c/cheaders.v | 14 ++--- ...d_maps_of_empty_structs_should_work_test.v | 40 ++++++++++++++ vlib/v/tests/empty_struct_test.v | 53 +++++++++++++++++++ 9 files changed, 135 insertions(+), 34 deletions(-) create mode 100644 vlib/v/tests/arrays_and_maps_of_empty_structs_should_work_test.v create mode 100644 vlib/v/tests/empty_struct_test.v diff --git a/vlib/arrays/arrays_test.v b/vlib/arrays/arrays_test.v index 65585460c..6180e505d 100644 --- a/vlib/arrays/arrays_test.v +++ b/vlib/arrays/arrays_test.v @@ -355,7 +355,7 @@ fn test_array_append_empty_struct() { assert (XYZ{} in names) == true // test fixed array - array := [XYZ{}] + array := [XYZ{}]! assert (XYZ{} in names) == true } diff --git a/vlib/builtin/array.v b/vlib/builtin/array.v index 9a006aaec..e57f987e5 100644 --- a/vlib/builtin/array.v +++ b/vlib/builtin/array.v @@ -47,9 +47,13 @@ fn __new_array_with_default(mylen int, cap int, elm_size int, val voidptr) array len: mylen cap: cap_ } + // x := []EmptyStruct{cap:5} ; for clang/gcc with -gc none, + // -> sizeof(EmptyStruct) == 0 -> elm_size == 0 + // -> total_size == 0 -> malloc(0) -> panic; + // to avoid it, just allocate a single byte total_size := u64(cap_) * u64(elm_size) if cap_ > 0 && mylen == 0 { - arr.data = unsafe { malloc(total_size) } + arr.data = unsafe { malloc(__at_least_one(total_size)) } } else { arr.data = vcalloc(total_size) } @@ -78,7 +82,7 @@ fn __new_array_with_array_default(mylen int, cap int, elm_size int, val array, d cap_ := if cap < mylen { mylen } else { cap } mut arr := array{ element_size: elm_size - data: unsafe { malloc(u64(cap_) * u64(elm_size)) } + data: unsafe { malloc(__at_least_one(u64(cap_) * u64(elm_size))) } len: mylen cap: cap_ } @@ -99,7 +103,7 @@ fn __new_array_with_map_default(mylen int, cap int, elm_size int, val map) array cap_ := if cap < mylen { mylen } else { cap } mut arr := array{ element_size: elm_size - data: unsafe { malloc(u64(cap_) * u64(elm_size)) } + data: unsafe { malloc(__at_least_one(u64(cap_) * u64(elm_size))) } len: mylen cap: cap_ } @@ -156,7 +160,7 @@ fn (mut a array) ensure_cap(required int) { cap *= 2 } new_size := u64(cap) * u64(a.element_size) - new_data := unsafe { malloc(new_size) } + new_data := unsafe { malloc(__at_least_one(new_size)) } if a.data != unsafe { nil } { unsafe { vmemcpy(new_data, a.data, u64(a.len) * u64(a.element_size)) } // TODO: the old data may be leaked when no GC is used (ref-counting?) @@ -601,13 +605,9 @@ pub fn (a &array) clone() array { // recursively clone given array - `unsafe` when called directly because depth is not checked [unsafe] pub fn (a &array) clone_to_depth(depth int) array { - mut size := u64(a.cap) * u64(a.element_size) - if size == 0 { - size++ - } mut arr := array{ element_size: a.element_size - data: vcalloc(size) + data: vcalloc(u64(a.cap) * u64(a.element_size)) len: a.len cap: a.cap } diff --git a/vlib/builtin/builtin.c.v b/vlib/builtin/builtin.c.v index c8e81544b..7636a6080 100644 --- a/vlib/builtin/builtin.c.v +++ b/vlib/builtin/builtin.c.v @@ -399,6 +399,16 @@ pub fn malloc_noscan(n isize) &u8 { return res } +[inline] +fn __at_least_one(how_many u64) u64 { + // handle the case for allocating memory for empty structs, which have sizeof(EmptyStruct) == 0 + // in this case, just allocate a single byte, avoiding the panic for malloc(0) + if how_many == 0 { + return 1 + } + return how_many +} + // malloc_uncollectable dynamically allocates a `n` bytes block of memory // on the heap, which will NOT be garbage-collected (but its contents will). [unsafe] diff --git a/vlib/builtin/map.v b/vlib/builtin/map.v index 48a5fbc3b..7b96d124a 100644 --- a/vlib/builtin/map.v +++ b/vlib/builtin/map.v @@ -100,8 +100,8 @@ fn new_dense_array(key_bytes int, value_bytes int) DenseArray { len: 0 deletes: 0 all_deleted: 0 - keys: unsafe { malloc(cap * key_bytes) } - values: unsafe { malloc(cap * value_bytes) } + keys: unsafe { malloc(__at_least_one(u64(cap) * u64(key_bytes))) } + values: unsafe { malloc(__at_least_one(u64(cap) * u64(value_bytes))) } } } diff --git a/vlib/builtin/map_d_gcboehm_opt.v b/vlib/builtin/map_d_gcboehm_opt.v index 001c621ee..4deb51616 100644 --- a/vlib/builtin/map_d_gcboehm_opt.v +++ b/vlib/builtin/map_d_gcboehm_opt.v @@ -6,19 +6,17 @@ module builtin +[inline] +fn __malloc_at_least_one(how_many_bytes u64, noscan bool) &u8 { + if noscan { + return unsafe { malloc_noscan(__at_least_one(how_many_bytes)) } + } + return unsafe { malloc(__at_least_one(how_many_bytes)) } +} + [inline] fn new_dense_array_noscan(key_bytes int, key_noscan bool, value_bytes int, value_noscan bool) DenseArray { cap := 8 - keys := if key_noscan { - unsafe { malloc_noscan(cap * key_bytes) } - } else { - unsafe { malloc(cap * key_bytes) } - } - values := if value_noscan { - unsafe { malloc_noscan(cap * value_bytes) } - } else { - unsafe { malloc(cap * value_bytes) } - } return DenseArray{ key_bytes: key_bytes value_bytes: value_bytes @@ -26,8 +24,8 @@ fn new_dense_array_noscan(key_bytes int, key_noscan bool, value_bytes int, value len: 0 deletes: 0 all_deleted: 0 - keys: keys - values: values + keys: __malloc_at_least_one(u64(cap) * u64(key_bytes), key_noscan) + values: __malloc_at_least_one(u64(cap) * u64(value_bytes), value_noscan) } } diff --git a/vlib/v/gen/c/array.v b/vlib/v/gen/c/array.v index e743564d0..d1f74d639 100644 --- a/vlib/v/gen/c/array.v +++ b/vlib/v/gen/c/array.v @@ -34,8 +34,6 @@ fn (mut g Gen) array_init(node ast.ArrayInit, var_name string) { noscan := g.check_noscan(elem_type.typ) if elem_type.unaliased_sym.kind == .function { g.write('new_array_from_c_array(${len}, ${len}, sizeof(voidptr), _MOV((voidptr[${len}]){') - } else if g.is_empty_struct(elem_type) { - g.write('new_array_from_c_array${noscan}(${len}, ${len}, sizeof(voidptr), _MOV((${elem_styp}[${len}]){') } else { g.write('new_array_from_c_array${noscan}(${len}, ${len}, sizeof(${elem_styp}), _MOV((${elem_styp}[${len}]){') } @@ -216,7 +214,7 @@ fn (mut g Gen) array_init_with_fields(node ast.ArrayInit, elem_type Type, is_amp } else { g.write('0, ') } - if elem_type.unaliased_sym.kind == .function || g.is_empty_struct(elem_type) { + if elem_type.unaliased_sym.kind == .function { g.write('sizeof(voidptr), ') } else { g.write('sizeof(${elem_styp}), ') @@ -292,7 +290,7 @@ fn (mut g Gen) array_init_with_fields(node ast.ArrayInit, elem_type Type, is_amp } else { g.write('0, ') } - if elem_type.unaliased_sym.kind == .function || g.is_empty_struct(elem_type) { + if elem_type.unaliased_sym.kind == .function { g.write('sizeof(voidptr), ') } else { g.write('sizeof(${elem_styp}), ') diff --git a/vlib/v/gen/c/cheaders.v b/vlib/v/gen/c/cheaders.v index 2dd11a8a9..8fbe7e788 100644 --- a/vlib/v/gen/c/cheaders.v +++ b/vlib/v/gen/c/cheaders.v @@ -253,8 +253,8 @@ static void* __closure_create(void* fn, void* data) { const c_common_macros = ' #define EMPTY_VARG_INITIALIZATION 0 -#define EMPTY_STRUCT_INITIALIZATION 0 -#define EMPTY_STRUCT_DECLARATION voidptr _dummy_pad +#define EMPTY_STRUCT_DECLARATION +#define EMPTY_STRUCT_INITIALIZATION // Due to a tcc bug, the length of an array needs to be specified, but GCC crashes if it is... #define EMPTY_ARRAY_OF_ELEMS(x,n) (x[]) #define TCCSKIP(x) x @@ -312,9 +312,12 @@ const c_common_macros = ' #ifdef __clang__ #undef __V_GCC__ #endif + #ifdef _MSC_VER #undef __V_GCC__ + #undef EMPTY_STRUCT_DECLARATION #undef EMPTY_STRUCT_INITIALIZATION + #define EMPTY_STRUCT_DECLARATION unsigned char _dummy_pad #define EMPTY_STRUCT_INITIALIZATION 0 #endif @@ -332,7 +335,9 @@ const c_common_macros = ' #ifdef __TINYC__ #define _Atomic volatile #undef EMPTY_STRUCT_DECLARATION - #define EMPTY_STRUCT_DECLARATION voidptr _dummy_pad + #undef EMPTY_STRUCT_INITIALIZATION + #define EMPTY_STRUCT_DECLARATION unsigned char _dummy_pad + #define EMPTY_STRUCT_INITIALIZATION 0 #undef EMPTY_ARRAY_OF_ELEMS #define EMPTY_ARRAY_OF_ELEMS(x,n) (x[n]) #undef __NOINLINE @@ -594,10 +599,7 @@ voidptr memdup(voidptr src, int sz); #define _Atomic volatile // MSVC cannot parse some things properly - #undef EMPTY_STRUCT_DECLARATION #undef OPTION_CAST - - #define EMPTY_STRUCT_DECLARATION voidptr _dummy_pad #define OPTION_CAST(x) #undef __NOINLINE #undef __IRQHANDLER diff --git a/vlib/v/tests/arrays_and_maps_of_empty_structs_should_work_test.v b/vlib/v/tests/arrays_and_maps_of_empty_structs_should_work_test.v new file mode 100644 index 000000000..9eef41b43 --- /dev/null +++ b/vlib/v/tests/arrays_and_maps_of_empty_structs_should_work_test.v @@ -0,0 +1,40 @@ +struct EmptyStruct {} + +fn test_array_append_empty_struct() { + size_of_empty_struct := dump(sizeof(EmptyStruct)) + assert size_of_empty_struct <= 1 + mut names := []EmptyStruct{cap: 2} + names << EmptyStruct{} + dump(names) + assert (EmptyStruct{} in names) == true + // + mut fa := [EmptyStruct{}, EmptyStruct{}]! + assert fa.len == 2 + dump(fa[0]) + fa[0] = EmptyStruct{} + dump(fa[0]) + assert fa[0] == EmptyStruct{} + assert fa[1] == EmptyStruct{} +} + +fn test_map_of_int_set_empty_struct() { + mut names := map[int]EmptyStruct{} + names[10] = EmptyStruct{} + names[99] = EmptyStruct{} + dump(names) + assert 10 in names + assert 99 in names + assert names[10] == EmptyStruct{} + assert names[99] == EmptyStruct{} +} + +fn test_map_of_string_set_empty_struct() { + mut names := map[string]EmptyStruct{} + names['ab'] = EmptyStruct{} + names['cd'] = EmptyStruct{} + dump(names) + assert 'ab' in names + assert 'cd' in names + assert names['ab'] == EmptyStruct{} + assert names['cd'] == EmptyStruct{} +} diff --git a/vlib/v/tests/empty_struct_test.v b/vlib/v/tests/empty_struct_test.v new file mode 100644 index 000000000..94d28d721 --- /dev/null +++ b/vlib/v/tests/empty_struct_test.v @@ -0,0 +1,53 @@ +struct Z0 {} + +struct Z1 { + padding1 char +} + +struct Z2 { + padding1 char + padding2 char +} + +struct Z3 { + padding1 char + padding2 char + padding3 char +} + +struct Z4 { + padding1 char + padding2 char + padding3 char + padding4 char +} + +fn test_struct_sizes() { + assert dump(sizeof(Z0)) <= 1 // valid for all + $if tinyc { + // TCC has no problems with 0 sized structs in almost cases, + // except when they are used in fixed arrays, or their address is taken, + // in which case, it produces a compilation error. To avoid it, for it + // empty structs are 1 byte in size. + assert dump(sizeof(Z0)) == 1 + } + $if msvc { + // MSVC seems to have no way at all to have empty structs in C mode. It produces the following error: + // `error c2016: C requires that a struct or union have at least one member`. + // Note that MSVC allows empty structs in C++ mode, but that has other restrictions, + // and is not suitable for the generated code of most V programs. Besides, even in C++ mode, the size of + // an empty struct is still 1, not 0. + // For that reason, empty structs are 1 byte in size for MSVC too. + assert dump(sizeof(Z0)) == 1 + } + $if clang { + assert dump(sizeof(Z0)) == 0 + } + $if gcc { + assert dump(sizeof(Z0)) == 0 + } + assert dump(sizeof(Z1)) < sizeof(Z2) + assert dump(sizeof(Z2)) < sizeof(Z3) + assert dump(sizeof(Z3)) < sizeof(Z4) + assert dump(sizeof(Z4)) == 4 +} -- 2.30.2