From d79a374ad4d304339e7aa0906c19c89ced314afa Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Wed, 6 Nov 2024 10:27:40 -0800 Subject: [PATCH] Skip vptr when performing object initialization (#4490) The cehck for vptr could be done differently (is this class dynamic and its base class non-dynamic), and if we change the layout (to put the vptr after any base) then this code will break (could add an assertion that the vptr isn't present apart from at the start of the field list if that'd seem worthwhile). There's still later issues with lowering (if this patch causes crashes in lowering, do they result in fuzz failures/need to be avoided before this change is submitted?) --- toolchain/check/convert.cpp | 19 +++++++++++++++---- .../testdata/class/virtual_modifiers.carbon | 14 ++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 155808cfd744..64386c37b7e5 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -392,6 +392,12 @@ static auto ConvertStructToStructOrClass(Context& context, auto& sem_ir = context.sem_ir(); auto src_elem_fields = sem_ir.inst_blocks().Get(src_type.fields_id); auto dest_elem_fields = sem_ir.inst_blocks().Get(dest_type.fields_id); + bool dest_has_vptr = + !dest_elem_fields.empty() && + sem_ir.insts() + .GetAs(dest_elem_fields.front()) + .name_id == SemIR::NameId::Vptr; + auto dest_elem_fields_size = dest_elem_fields.size() - dest_has_vptr; auto value = sem_ir.insts().Get(value_id); auto value_loc_id = sem_ir.insts().GetLocId(value_id); @@ -411,14 +417,14 @@ static auto ConvertStructToStructOrClass(Context& context, // Check that the structs are the same size. // TODO: If not, include the name of the first source field that doesn't // exist in the destination or vice versa in the diagnostic. - if (src_elem_fields.size() != dest_elem_fields.size()) { + if (src_elem_fields.size() != dest_elem_fields_size) { CARBON_DIAGNOSTIC( StructInitElementCountMismatch, Error, "cannot initialize {0:class|struct} with {1} field{1:s} from struct " "with {2} field{2:s}", BoolAsSelect, IntAsSelect, IntAsSelect); context.emitter().Emit(value_loc_id, StructInitElementCountMismatch, - ToClass, dest_elem_fields.size(), + ToClass, dest_elem_fields_size, src_elem_fields.size()); return SemIR::InstId::BuiltinError; } @@ -449,14 +455,19 @@ static auto ConvertStructToStructOrClass(Context& context, // of the source. // TODO: Annotate diagnostics coming from here with the element index. auto new_block = - literal_elems_id.is_valid() + literal_elems_id.is_valid() && !dest_has_vptr ? SemIR::CopyOnWriteInstBlock(sem_ir, literal_elems_id) : SemIR::CopyOnWriteInstBlock( sem_ir, SemIR::CopyOnWriteInstBlock::UninitializedBlock{ - src_elem_fields.size()}); + dest_elem_fields.size()}); for (auto [i, dest_field_id] : llvm::enumerate(dest_elem_fields)) { auto dest_field = sem_ir.insts().GetAs(dest_field_id); + if (dest_field.name_id == SemIR::NameId::Vptr) { + // TODO: Initialize the vptr to point to a vtable. + new_block.Set(i, SemIR::InstId::BuiltinError); + continue; + } // Find the matching source field. auto src_field_index = i; diff --git a/toolchain/check/testdata/class/virtual_modifiers.carbon b/toolchain/check/testdata/class/virtual_modifiers.carbon index 3807b1d08ed3..3cf14070cd5d 100644 --- a/toolchain/check/testdata/class/virtual_modifiers.carbon +++ b/toolchain/check/testdata/class/virtual_modifiers.carbon @@ -37,17 +37,13 @@ base class Derived { extend base: Modifiers.Base; } -// --- fail_todo_init.carbon +// --- init.carbon package Init; import Modifiers; fn F() { - // TODO: The vptr shouldn't be counted for programmer-facing behavior. - // CHECK:STDERR: fail_todo_init.carbon:[[@LINE+3]]:27: error: cannot initialize class with 1 field from struct with 0 fields [StructInitElementCountMismatch] - // CHECK:STDERR: var v: Modifiers.Base = {}; - // CHECK:STDERR: ^~ var v: Modifiers.Base = {}; } @@ -190,7 +186,7 @@ fn F() { // CHECK:STDOUT: // CHECK:STDOUT: virtual fn @F(); // CHECK:STDOUT: -// CHECK:STDOUT: --- fail_todo_init.carbon +// CHECK:STDOUT: --- init.carbon // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %F.type: type = fn_type @F [template] @@ -243,8 +239,10 @@ fn F() { // CHECK:STDOUT: %Base.ref: type = name_ref Base, imports.%import_ref.1 [template = constants.%Base] // CHECK:STDOUT: %v.var: ref %Base = var v // CHECK:STDOUT: %v: ref %Base = bind_name v, %v.var -// CHECK:STDOUT: %.loc11: %.6 = struct_literal () -// CHECK:STDOUT: assign %v.var, +// CHECK:STDOUT: %.loc7_28.1: %.6 = struct_literal () +// CHECK:STDOUT: %.loc7_28.2: init %Base = class_init (), %v.var [template = ] +// CHECK:STDOUT: %.loc7_29: init %Base = converted %.loc7_28.1, %.loc7_28.2 [template = ] +// CHECK:STDOUT: assign %v.var, %.loc7_29 // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: