From 917bd4192d4586fb2e8f3855d9e2e2eb0bf45921 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Thu, 9 May 2019 10:19:02 +0200 Subject: [PATCH 1/5] Validate enum tag for extern enum The C specification mandates the enum to be compatible with signed char, signed int or unsigned int. --- src/analyze.cpp | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index c441676e778e..3f50ccc2b996 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -1908,6 +1908,30 @@ static Error resolve_union_type(CodeGen *g, ZigType *union_type) { return ErrorNone; } +static bool type_is_int_compatible(ZigType *t1, ZigType *t2) { + assert(t1->id == ZigTypeIdInt); + assert(t2->id == ZigTypeIdInt); + + if (t1 == t2) + return true; + + return (t1->data.integral.bit_count == t2->data.integral.bit_count && + t1->data.integral.is_signed == t2->data.integral.is_signed); +} + +static bool type_is_valid_extern_enum_tag(CodeGen *g, ZigType *ty) { + // According to the ANSI C standard: + // Each enumerated type shall be compatible with char, a signed integer + // type, or an unsigned integer type. + ZigType *c_uint_type = get_c_int_type(g, CIntTypeInt); + ZigType *c_int_type = get_c_int_type(g, CIntTypeInt); + ZigType *c_schar_type = g->builtin_types.entry_i8; + + return (type_is_int_compatible(ty, c_schar_type) || + type_is_int_compatible(ty, c_int_type) || + type_is_int_compatible(ty, c_uint_type)); +} + static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { assert(enum_type->id == ZigTypeIdEnum); @@ -1965,7 +1989,6 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { enum_type->abi_size = tag_int_type->abi_size; enum_type->abi_align = tag_int_type->abi_align; - // TODO: Are extern enums allowed to have an init_arg_expr? if (decl_node->data.container_decl.init_arg_expr != nullptr) { ZigType *wanted_tag_int_type = analyze_type_expr(g, scope, decl_node->data.container_decl.init_arg_expr); if (type_is_invalid(wanted_tag_int_type)) { @@ -1974,6 +1997,14 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { enum_type->data.enumeration.is_invalid = true; add_node_error(g, decl_node->data.container_decl.init_arg_expr, buf_sprintf("expected integer, found '%s'", buf_ptr(&wanted_tag_int_type->name))); + } else if (enum_type->data.enumeration.layout == ContainerLayoutExtern && + !type_is_valid_extern_enum_tag(g, wanted_tag_int_type)) { + enum_type->data.enumeration.is_invalid = true; + ErrorMsg *msg = add_node_error(g, decl_node->data.container_decl.init_arg_expr, + buf_sprintf("'%s' is not a valid tag type for an extern union", + buf_ptr(&wanted_tag_int_type->name))); + add_error_note(g, msg, decl_node->data.container_decl.init_arg_expr, + buf_sprintf("valid types are 'i8', 'c_int' and 'c_uint' or compatible types")); } else if (wanted_tag_int_type->data.integral.is_signed) { enum_type->data.enumeration.is_invalid = true; add_node_error(g, decl_node->data.container_decl.init_arg_expr, @@ -1987,6 +2018,7 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { tag_int_type = wanted_tag_int_type; } } + enum_type->data.enumeration.tag_int_type = tag_int_type; enum_type->size_in_bits = tag_int_type->size_in_bits; enum_type->abi_size = tag_int_type->abi_size; From c766f3f9ca84b81a9f5669dd5132e46355ef284b Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Thu, 9 May 2019 18:40:00 +0200 Subject: [PATCH 2/5] Support signed types as enum tags --- src/analyze.cpp | 57 ++++++++++++++++++++--------------- test/compile_errors.zig | 18 +---------- test/stage1/behavior/enum.zig | 12 ++++++++ 3 files changed, 45 insertions(+), 42 deletions(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index 3f50ccc2b996..d4e69a1fe65f 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -2005,15 +2005,6 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { buf_ptr(&wanted_tag_int_type->name))); add_error_note(g, msg, decl_node->data.container_decl.init_arg_expr, buf_sprintf("valid types are 'i8', 'c_int' and 'c_uint' or compatible types")); - } else if (wanted_tag_int_type->data.integral.is_signed) { - enum_type->data.enumeration.is_invalid = true; - add_node_error(g, decl_node->data.container_decl.init_arg_expr, - buf_sprintf("expected unsigned integer, found '%s'", buf_ptr(&wanted_tag_int_type->name))); - } else if (wanted_tag_int_type->data.integral.bit_count < tag_int_type->data.integral.bit_count) { - enum_type->data.enumeration.is_invalid = true; - add_node_error(g, decl_node->data.container_decl.init_arg_expr, - buf_sprintf("'%s' too small to hold all bits; must be at least '%s'", - buf_ptr(&wanted_tag_int_type->name), buf_ptr(&tag_int_type->name))); } else { tag_int_type = wanted_tag_int_type; } @@ -2078,30 +2069,46 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { } // Now iterate again and populate the unspecified tag values - uint32_t next_maybe_unoccupied_index = 0; + BigInt next_maybe_unoccupied_index; + bigint_init_unsigned(&next_maybe_unoccupied_index, 0); + + // Since we're allocating positive values only we have one less bit + // available if the tag type is signed (eg. for a i8 we can only use (0,127)) + unsigned tag_bit_width = tag_int_type->size_in_bits; + if (tag_int_type->data.integral.is_signed) + tag_bit_width--; for (uint32_t field_i = 0; field_i < field_count; field_i += 1) { AstNode *field_node = decl_node->data.container_decl.fields.at(field_i); TypeEnumField *type_enum_field = &enum_type->data.enumeration.fields[field_i]; AstNode *tag_value = field_node->data.struct_field.value; - if (tag_value == nullptr) { - if (occupied_tag_values.size() == 0) { - bigint_init_unsigned(&type_enum_field->value, next_maybe_unoccupied_index); - next_maybe_unoccupied_index += 1; - } else { - BigInt proposed_value; - for (;;) { - bigint_init_unsigned(&proposed_value, next_maybe_unoccupied_index); - next_maybe_unoccupied_index += 1; - auto entry = occupied_tag_values.put_unique(proposed_value, field_node); - if (entry != nullptr) { - continue; - } + // Already handled in the loop above + if (tag_value != nullptr) + continue; + + // Make sure we can represent this number with tag_int_type + const unsigned repr_bits = bigint_bits_needed(&next_maybe_unoccupied_index); + if (repr_bits > tag_bit_width) { + enum_type->data.enumeration.is_invalid = true; + add_node_error(g, field_node, + buf_sprintf("enumeration value %" ZIG_PRI_u64 " too large for type '%s'", + bigint_as_unsigned(&next_maybe_unoccupied_index), + buf_ptr(&tag_int_type->name))); + break; + } + + if (occupied_tag_values.size() == 0) { + type_enum_field->value = next_maybe_unoccupied_index; + bigint_incr(&next_maybe_unoccupied_index); + } else { + for (;;) { + auto entry = occupied_tag_values.put_unique(next_maybe_unoccupied_index, field_node); + if (entry == nullptr) break; - } - bigint_init_bigint(&type_enum_field->value, &proposed_value); + bigint_incr(&next_maybe_unoccupied_index); } + type_enum_field->value = next_maybe_unoccupied_index; } } diff --git a/test/compile_errors.zig b/test/compile_errors.zig index 6fc2fa65e731..eef5bc202cc8 100644 --- a/test/compile_errors.zig +++ b/test/compile_errors.zig @@ -5397,7 +5397,7 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { \\ var x = Small.One; \\} , - "tmp.zig:1:21: error: 'u2' too small to hold all bits; must be at least 'u3'", + "tmp.zig:6:5: error: enumeration value 4 too large for type 'u2'" ); cases.add( @@ -5448,22 +5448,6 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { "tmp.zig:10:31: error: expected type 'u2', found 'u3'", ); - cases.add( - "non unsigned integer enum tag type", - \\const Small = enum(i2) { - \\ One, - \\ Two, - \\ Three, - \\ Four, - \\}; - \\ - \\export fn entry() void { - \\ var y = Small.Two; - \\} - , - "tmp.zig:1:20: error: expected unsigned integer, found 'i2'", - ); - cases.add( "struct fields with value assignments", \\const MultipleChoice = struct { diff --git a/test/stage1/behavior/enum.zig b/test/stage1/behavior/enum.zig index 70515ec35b41..5e49b6d49131 100644 --- a/test/stage1/behavior/enum.zig +++ b/test/stage1/behavior/enum.zig @@ -938,3 +938,15 @@ test "enum literal in array literal" { expect(array[0] == .one); expect(array[1] == .two); } + +test "signed integer as enum tag" { + const SignedEnum = enum (i2) { + A0 = -1, + A1 = 0, + A2 = 1, + }; + + expect(@enumToInt(SignedEnum.A0) == -1); + expect(@enumToInt(SignedEnum.A1) == 0); + expect(@enumToInt(SignedEnum.A2) == 1); +} From 655794f44fc8563f9fa4d45c208e859382a6a599 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Fri, 10 May 2019 18:16:28 +0200 Subject: [PATCH 3/5] amend type_is_valid_extern_enum_tag --- src/analyze.cpp | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index d4e69a1fe65f..9462ce0121e1 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -1908,28 +1908,16 @@ static Error resolve_union_type(CodeGen *g, ZigType *union_type) { return ErrorNone; } -static bool type_is_int_compatible(ZigType *t1, ZigType *t2) { - assert(t1->id == ZigTypeIdInt); - assert(t2->id == ZigTypeIdInt); - - if (t1 == t2) - return true; - - return (t1->data.integral.bit_count == t2->data.integral.bit_count && - t1->data.integral.is_signed == t2->data.integral.is_signed); -} - static bool type_is_valid_extern_enum_tag(CodeGen *g, ZigType *ty) { - // According to the ANSI C standard: - // Each enumerated type shall be compatible with char, a signed integer - // type, or an unsigned integer type. - ZigType *c_uint_type = get_c_int_type(g, CIntTypeInt); - ZigType *c_int_type = get_c_int_type(g, CIntTypeInt); - ZigType *c_schar_type = g->builtin_types.entry_i8; - - return (type_is_int_compatible(ty, c_schar_type) || - type_is_int_compatible(ty, c_int_type) || - type_is_int_compatible(ty, c_uint_type)); + // Only integer types are allowed by the C ABI + if(ty->id != ZigTypeIdInt) + return false; + + // According to the ANSI C standard the enumeration type should be either a + // signed char, a signed integer or an unsigned one. But GCC/Clang allow + // other integral types as a compiler extension so let's accomodate them + // aswell. + return type_allowed_in_extern(g, ty); } static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { From b05e8d46ec0a1a26c533118d5a0bab2262a99a63 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Fri, 10 May 2019 19:28:13 +0200 Subject: [PATCH 4/5] Change the enum value allocation strategy --- src/analyze.cpp | 93 +++++++++++++++-------------------- test/compile_errors.zig | 24 ++++++--- test/stage1/behavior/enum.zig | 22 +++++++-- 3 files changed, 74 insertions(+), 65 deletions(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index 9462ce0121e1..3f662c246c8b 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -2003,6 +2003,11 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { enum_type->abi_size = tag_int_type->abi_size; enum_type->abi_align = tag_int_type->abi_align; + BigInt bi_one; + bigint_init_unsigned(&bi_one, 1); + + TypeEnumField *last_enum_field = nullptr; + for (uint32_t field_i = 0; field_i < field_count; field_i += 1) { AstNode *field_node = decl_node->data.container_decl.fields.at(field_i); TypeEnumField *type_enum_field = &enum_type->data.enumeration.fields[field_i]; @@ -2028,76 +2033,58 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { AstNode *tag_value = field_node->data.struct_field.value; - // In this first pass we resolve explicit tag values. - // In a second pass we will fill in the unspecified ones. if (tag_value != nullptr) { + // A user-specified value is available ConstExprValue *result = analyze_const_value(g, scope, tag_value, tag_int_type, nullptr); if (type_is_invalid(result->type)) { enum_type->data.enumeration.is_invalid = true; continue; } + assert(result->special != ConstValSpecialRuntime); - assert(result->type->id == ZigTypeIdInt || - result->type->id == ZigTypeIdComptimeInt); - auto entry = occupied_tag_values.put_unique(result->data.x_bigint, tag_value); - if (entry == nullptr) { - bigint_init_bigint(&type_enum_field->value, &result->data.x_bigint); + assert(result->type->id == ZigTypeIdInt || result->type->id == ZigTypeIdComptimeInt); + + bigint_init_bigint(&type_enum_field->value, &result->data.x_bigint); + } else { + // No value was explicitly specified: allocate the last value + 1 + // or, if this is the first element, zero + if (last_enum_field != nullptr) { + bigint_add(&type_enum_field->value, &last_enum_field->value, &bi_one); } else { - Buf *val_buf = buf_alloc(); - bigint_append_buf(val_buf, &result->data.x_bigint, 10); + bigint_init_unsigned(&type_enum_field->value, 0); + } - ErrorMsg *msg = add_node_error(g, tag_value, - buf_sprintf("enum tag value %s already taken", buf_ptr(val_buf))); - add_error_note(g, msg, entry->value, - buf_sprintf("other occurrence here")); + // Make sure we can represent this number with tag_int_type + if (!bigint_fits_in_bits(&type_enum_field->value, + tag_int_type->size_in_bits, + tag_int_type->data.integral.is_signed)) { enum_type->data.enumeration.is_invalid = true; - continue; - } - } - } - // Now iterate again and populate the unspecified tag values - BigInt next_maybe_unoccupied_index; - bigint_init_unsigned(&next_maybe_unoccupied_index, 0); + Buf *val_buf = buf_alloc(); + bigint_append_buf(val_buf, &type_enum_field->value, 10); + add_node_error(g, field_node, + buf_sprintf("enumeration value %s too large for type '%s'", + buf_ptr(val_buf), buf_ptr(&tag_int_type->name))); - // Since we're allocating positive values only we have one less bit - // available if the tag type is signed (eg. for a i8 we can only use (0,127)) - unsigned tag_bit_width = tag_int_type->size_in_bits; - if (tag_int_type->data.integral.is_signed) - tag_bit_width--; + break; + } + } - for (uint32_t field_i = 0; field_i < field_count; field_i += 1) { - AstNode *field_node = decl_node->data.container_decl.fields.at(field_i); - TypeEnumField *type_enum_field = &enum_type->data.enumeration.fields[field_i]; - AstNode *tag_value = field_node->data.struct_field.value; + // Make sure the value is unique + auto entry = occupied_tag_values.put_unique(type_enum_field->value, field_node); + if (entry != nullptr) { + enum_type->data.enumeration.is_invalid = true; - // Already handled in the loop above - if (tag_value != nullptr) - continue; + Buf *val_buf = buf_alloc(); + bigint_append_buf(val_buf, &type_enum_field->value, 10); - // Make sure we can represent this number with tag_int_type - const unsigned repr_bits = bigint_bits_needed(&next_maybe_unoccupied_index); - if (repr_bits > tag_bit_width) { - enum_type->data.enumeration.is_invalid = true; - add_node_error(g, field_node, - buf_sprintf("enumeration value %" ZIG_PRI_u64 " too large for type '%s'", - bigint_as_unsigned(&next_maybe_unoccupied_index), - buf_ptr(&tag_int_type->name))); - break; + ErrorMsg *msg = add_node_error(g, field_node, + buf_sprintf("enum tag value %s already taken", buf_ptr(val_buf))); + add_error_note(g, msg, entry->value, + buf_sprintf("other occurrence here")); } - if (occupied_tag_values.size() == 0) { - type_enum_field->value = next_maybe_unoccupied_index; - bigint_incr(&next_maybe_unoccupied_index); - } else { - for (;;) { - auto entry = occupied_tag_values.put_unique(next_maybe_unoccupied_index, field_node); - if (entry == nullptr) - break; - bigint_incr(&next_maybe_unoccupied_index); - } - type_enum_field->value = next_maybe_unoccupied_index; - } + last_enum_field = type_enum_field; } enum_type->data.enumeration.zero_bits_loop_flag = false; diff --git a/test/compile_errors.zig b/test/compile_errors.zig index eef5bc202cc8..297673235d2d 100644 --- a/test/compile_errors.zig +++ b/test/compile_errors.zig @@ -12,6 +12,19 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { "tmp.zig:3:32: error: cast discards const qualifier", ); + cases.add( + "overflow in enum value allocation", + \\const Moo = enum(u8) { + \\ Last = 255, + \\ Over, + \\}; + \\pub fn main() void { + \\ var y = Moo.Last; + \\} + , + "tmp.zig:3:5: error: enumeration value 256 too large for type 'u8'", + ); + cases.add( "attempt to cast enum literal to error", \\export fn entry() void { @@ -5383,8 +5396,7 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { "tmp.zig:12:20: note: referenced here", ); - cases.add( - "specify enum tag type that is too small", + cases.add("specify enum tag type that is too small", \\const Small = enum (u2) { \\ One, \\ Two, @@ -5396,9 +5408,7 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { \\export fn entry() void { \\ var x = Small.One; \\} - , - "tmp.zig:6:5: error: enumeration value 4 too large for type 'u2'" - ); + , "tmp.zig:6:5: error: enumeration value 4 too large for type 'u2'"); cases.add( "specify non-integer enum tag type", @@ -5506,8 +5516,8 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { \\ var x = MultipleChoice.C; \\} , - "tmp.zig:6:9: error: enum tag value 60 already taken", - "tmp.zig:4:9: note: other occurrence here", + "tmp.zig:6:5: error: enum tag value 60 already taken", + "tmp.zig:4:5: note: other occurrence here", ); cases.add( diff --git a/test/stage1/behavior/enum.zig b/test/stage1/behavior/enum.zig index 5e49b6d49131..8a5edaa3dd76 100644 --- a/test/stage1/behavior/enum.zig +++ b/test/stage1/behavior/enum.zig @@ -940,13 +940,25 @@ test "enum literal in array literal" { } test "signed integer as enum tag" { - const SignedEnum = enum (i2) { + const SignedEnum = enum(i2) { A0 = -1, - A1 = 0, - A2 = 1, + A1 = 0, + A2 = 1, }; expect(@enumToInt(SignedEnum.A0) == -1); - expect(@enumToInt(SignedEnum.A1) == 0); - expect(@enumToInt(SignedEnum.A2) == 1); + expect(@enumToInt(SignedEnum.A1) == 0); + expect(@enumToInt(SignedEnum.A2) == 1); +} + +test "enum value allocation" { + const LargeEnum = enum(u32) { + A0 = 0x80000000, + A1, + A2, + }; + + expect(@enumToInt(LargeEnum.A0) == 0x80000000); + expect(@enumToInt(LargeEnum.A1) == 0x80000001); + expect(@enumToInt(LargeEnum.A2) == 0x80000002); } From d210628c91f2d33a168c71fc7633745ead925dfc Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sat, 11 May 2019 21:27:52 +0200 Subject: [PATCH 5/5] Amend the error messages --- src/analyze.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index 3f662c246c8b..57244aba6af0 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -1989,10 +1989,10 @@ static Error resolve_enum_zero_bits(CodeGen *g, ZigType *enum_type) { !type_is_valid_extern_enum_tag(g, wanted_tag_int_type)) { enum_type->data.enumeration.is_invalid = true; ErrorMsg *msg = add_node_error(g, decl_node->data.container_decl.init_arg_expr, - buf_sprintf("'%s' is not a valid tag type for an extern union", + buf_sprintf("'%s' is not a valid tag type for an extern enum", buf_ptr(&wanted_tag_int_type->name))); add_error_note(g, msg, decl_node->data.container_decl.init_arg_expr, - buf_sprintf("valid types are 'i8', 'c_int' and 'c_uint' or compatible types")); + buf_sprintf("any integral type of size 8, 16, 32, 64 or 128 bit is valid")); } else { tag_int_type = wanted_tag_int_type; }