From 5d896a6cc6b7127dd4db0bd386ebe33da82d7824 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Mon, 10 Apr 2023 18:27:22 +0200 Subject: spirv: fix use-after-realloc in resolveType() The pointer to a slot in a hash map was fetched before a recursive call. If the hash map's size changed during the recursive call, this would write to an invalid pointer. The solution is to use an index instead of a pointer. Note that care must be taken that resolved types (from the type_cahce) must not be accessed, as they might be incomplete during this operation. --- src/codegen/spirv/Module.zig | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'src/codegen/spirv/Module.zig') diff --git a/src/codegen/spirv/Module.zig b/src/codegen/spirv/Module.zig index 7ae6cb0c6a..be8e5b24d1 100644 --- a/src/codegen/spirv/Module.zig +++ b/src/codegen/spirv/Module.zig @@ -393,11 +393,14 @@ pub fn resolveSourceFileName(self: *Module, decl: *ZigDecl) !IdRef { /// be emitted at this point. pub fn resolveType(self: *Module, ty: Type) !Type.Ref { const result = try self.type_cache.getOrPut(self.gpa, ty); + const index = @intToEnum(Type.Ref, result.index); + if (!result.found_existing) { - result.value_ptr.* = try self.emitType(ty); + const ref = try self.emitType(ty); + self.type_cache.values()[result.index] = ref; } - return @intToEnum(Type.Ref, result.index); + return index; } pub fn resolveTypeId(self: *Module, ty: Type) !IdResultType { -- cgit v1.2.3 From e26d8d060410ff5f62356c41f3c782f8a9081495 Mon Sep 17 00:00:00 2001 From: Robin Voetter Date: Sat, 22 Apr 2023 11:18:58 +0200 Subject: spirv: make decl deps a hash map instead of an arraylist The same declaration can be added to the dependency set multiple times, and in this case we still need to emit it once. By making this list a hash map instead, we can do that quite easily. This commit also introduces some additional debug logging regarding decls. --- src/codegen/spirv.zig | 21 ++++++++++++--------- src/codegen/spirv/Module.zig | 6 +++--- 2 files changed, 15 insertions(+), 12 deletions(-) (limited to 'src/codegen/spirv/Module.zig') diff --git a/src/codegen/spirv.zig b/src/codegen/spirv.zig index 6911588fd0..3bec35af1d 100644 --- a/src/codegen/spirv.zig +++ b/src/codegen/spirv.zig @@ -238,6 +238,7 @@ pub const DeclGen = struct { else => unreachable, }; const spv_decl_index = try self.resolveDecl(fn_decl_index); + try self.func.decl_deps.put(self.spv.gpa, spv_decl_index, {}); return self.spv.declPtr(spv_decl_index).result_id; } @@ -459,7 +460,7 @@ pub const DeclGen = struct { /// If full, its flushed. partial_word: std.BoundedArray(u8, @sizeOf(Word)) = .{}, /// The declaration dependencies of the constant we are lowering. - decl_deps: std.ArrayList(SpvModule.Decl.Index), + decl_deps: std.AutoArrayHashMap(SpvModule.Decl.Index, void), /// Utility function to get the section that instructions should be lowered to. fn section(self: *@This()) *SpvSection { @@ -582,14 +583,15 @@ pub const DeclGen = struct { // just generate an empty pointer. Function pointers are represented by usize for now, // though. try self.addInt(Type.usize, Value.initTag(.zero)); + // TODO: Add dependency return; }, .extern_fn => unreachable, // TODO else => { const result_id = dg.spv.allocId(); - log.debug("addDeclRef {s} = {}", .{ decl.name, result_id.id }); + log.debug("addDeclRef: id = {}, index = {}, name = {s}", .{ result_id.id, @enumToInt(spv_decl_index), decl.name }); - try self.decl_deps.append(spv_decl_index); + try self.decl_deps.put(spv_decl_index, {}); const decl_id = dg.spv.declPtr(spv_decl_index).result_id; // TODO: Do we need a storage class cast here? @@ -861,7 +863,8 @@ pub const DeclGen = struct { assert(storage_class != .Generic and storage_class != .Function); - log.debug("lowerIndirectConstant: ty = {}, val = {}", .{ ty.fmt(self.module), val.fmtDebug() }); + const var_id = self.spv.allocId(); + log.debug("lowerIndirectConstant: id = {}, index = {}, ty = {}, val = {}", .{ var_id.id, @enumToInt(spv_decl_index), ty.fmt(self.module), val.fmtDebug() }); const section = &self.spv.globals.section; @@ -897,7 +900,7 @@ pub const DeclGen = struct { .u32_ty_id = self.typeId(u32_ty_ref), .members = std.ArrayList(SpvType.Payload.Struct.Member).init(self.gpa), .initializers = std.ArrayList(IdRef).init(self.gpa), - .decl_deps = std.ArrayList(SpvModule.Decl.Index).init(self.gpa), + .decl_deps = std.AutoArrayHashMap(SpvModule.Decl.Index, void).init(self.gpa), }; defer icl.members.deinit(); @@ -917,7 +920,6 @@ pub const DeclGen = struct { .constituents = icl.initializers.items, }); - const var_id = self.spv.allocId(); self.spv.globalPtr(spv_decl_index).?.result_id = var_id; try section.emit(self.spv.gpa, .OpVariable, .{ .id_result_type = self.typeId(ptr_constant_struct_ty_ref), @@ -951,7 +953,7 @@ pub const DeclGen = struct { }); } - try self.spv.declareDeclDeps(spv_decl_index, icl.decl_deps.items); + try self.spv.declareDeclDeps(spv_decl_index, icl.decl_deps.keys()); self.spv.endGlobal(spv_decl_index, begin_inst); } @@ -1007,7 +1009,8 @@ pub const DeclGen = struct { false, alignment, ); - try self.func.decl_deps.append(self.spv.gpa, spv_decl_index); + log.debug("indirect constant: index = {}", .{@enumToInt(spv_decl_index)}); + try self.func.decl_deps.put(self.spv.gpa, spv_decl_index, {}); try self.func.body.emit(self.spv.gpa, .OpLoad, .{ .id_result_type = result_ty_id, @@ -1471,7 +1474,7 @@ pub const DeclGen = struct { const spv_decl_index = try self.resolveDecl(self.decl_index); const decl_id = self.spv.declPtr(spv_decl_index).result_id; - log.debug("genDecl {s} = {}", .{ decl.name, decl_id }); + log.debug("genDecl: id = {}, index = {}, name = {s}", .{ decl_id.id, @enumToInt(spv_decl_index), decl.name }); if (decl.val.castTag(.function)) |_| { assert(decl.ty.zigTypeTag() == .Fn); diff --git a/src/codegen/spirv/Module.zig b/src/codegen/spirv/Module.zig index be8e5b24d1..4bd6c834ce 100644 --- a/src/codegen/spirv/Module.zig +++ b/src/codegen/spirv/Module.zig @@ -40,14 +40,14 @@ pub const Fn = struct { /// the end of this function definition. body: Section = .{}, /// The decl dependencies that this function depends on. - decl_deps: std.ArrayListUnmanaged(Decl.Index) = .{}, + decl_deps: std.AutoArrayHashMapUnmanaged(Decl.Index, void) = .{}, /// Reset this function without deallocating resources, so that /// it may be used to emit code for another function. pub fn reset(self: *Fn) void { self.prologue.reset(); self.body.reset(); - self.decl_deps.items.len = 0; + self.decl_deps.clearRetainingCapacity(); } /// Free the resources owned by this function. @@ -358,7 +358,7 @@ pub fn flush(self: *Module, file: std.fs.File) !void { pub fn addFunction(self: *Module, decl_index: Decl.Index, func: Fn) !void { try self.sections.functions.append(self.gpa, func.prologue); try self.sections.functions.append(self.gpa, func.body); - try self.declareDeclDeps(decl_index, func.decl_deps.items); + try self.declareDeclDeps(decl_index, func.decl_deps.keys()); } /// Fetch the result-id of an OpString instruction that encodes the path of the source -- cgit v1.2.3