From 667ad9250f7250670778beedcebc45f5c0284446 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 16:48:44 -0700 Subject: [PATCH 1/3] Sema: fix coerce_result_ptr in case of inferred result type Previously, the logic for analyzing coerce_result_ptr would generate invalid bitcast instructions which did not include coercion logic, such as optional wrapping, resulting in miscompilations. Now, the logic of resolve_inferred_alloc goes back over all the placeholders inserted by coerce_result_ptr, and replaces them with logic doing the proper coercions. Closes #12045 --- src/Sema.zig | 107 ++++++++++++++++++++++++++++++++++++++++--- src/value.zig | 11 ++++- test/behavior/if.zig | 12 +++++ 3 files changed, 122 insertions(+), 8 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index 29840820d0..d4de78e5d1 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -1974,8 +1974,6 @@ fn zirCoerceResultPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileE defer trash_block.instructions.deinit(sema.gpa); const operand = try trash_block.addBitCast(pointee_ty, .void_value); - try inferred_alloc.stored_inst_list.append(sema.arena, operand); - try sema.requireRuntimeBlock(block, src); const ptr_ty = try Type.ptr(sema.arena, sema.mod, .{ .pointee_type = pointee_ty, @@ -1983,6 +1981,12 @@ fn zirCoerceResultPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileE .@"addrspace" = addr_space, }); const bitcasted_ptr = try block.addBitCast(ptr_ty, ptr); + + try inferred_alloc.prongs.append(sema.arena, .{ + .stored_inst = operand, + .placeholder = Air.refToIndex(bitcasted_ptr).?, + }); + return bitcasted_ptr; }, .inferred_alloc_comptime => { @@ -2026,8 +2030,23 @@ fn zirCoerceResultPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileE defer trash_block.instructions.deinit(sema.gpa); const dummy_ptr = try trash_block.addTy(.alloc, sema.typeOf(ptr)); + return coerceResultPtr(sema, block, src, ptr, dummy_ptr, pointee_ty, &trash_block); +} + +fn coerceResultPtr( + sema: *Sema, + block: *Block, + src: LazySrcLoc, + ptr: Air.Inst.Ref, + dummy_ptr: Air.Inst.Ref, + pointee_ty: Type, + trash_block: *Block, +) CompileError!Air.Inst.Ref { + const target = sema.mod.getTarget(); + const addr_space = target_util.defaultAddressSpace(target, .local); + const dummy_operand = try trash_block.addBitCast(pointee_ty, .void_value); - try sema.storePtr2(&trash_block, src, dummy_ptr, src, dummy_operand, src, .bitcast); + try sema.storePtr2(trash_block, src, dummy_ptr, src, dummy_operand, src, .bitcast); { const air_tags = sema.air_instructions.items(.tag); @@ -2066,6 +2085,12 @@ fn zirCoerceResultPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileE if (try sema.resolveDefinedValue(block, src, new_ptr)) |ptr_val| { return sema.addConstant(ptr_ty, ptr_val); } + if (pointee_ty.eql(Type.@"null", sema.mod)) { + const opt_ty = sema.typeOf(new_ptr).childType(); + const null_inst = try sema.addConstant(opt_ty, Value.@"null"); + _ = try block.addBinOp(.store, new_ptr, null_inst); + return Air.Inst.Ref.void_value; + } return sema.bitCast(block, ptr_ty, new_ptr, src); } const ty_op = air_datas[trash_inst].ty_op; @@ -3141,7 +3166,7 @@ fn zirResolveInferredAlloc(sema: *Sema, block: *Block, inst: Zir.Inst.Index) Com }, .inferred_alloc => { const inferred_alloc = ptr_val.castTag(.inferred_alloc).?; - const peer_inst_list = inferred_alloc.data.stored_inst_list.items; + const peer_inst_list = inferred_alloc.data.prongs.items(.stored_inst); const final_elem_ty = try sema.resolvePeerTypes(block, ty_src, peer_inst_list, .none); const final_ptr_ty = try Type.ptr(sema.arena, sema.mod, .{ @@ -3250,6 +3275,71 @@ fn zirResolveInferredAlloc(sema: *Sema, block: *Block, inst: Zir.Inst.Index) Com .tag = .alloc, .data = .{ .ty = final_ptr_ty }, }); + + // Now we need to go back over all the coerce_result_ptr instructions, which + // previously inserted a bitcast as a placeholder, and do the logic as if + // the new result ptr type was available. + const placeholders = inferred_alloc.data.prongs.items(.placeholder); + const gpa = sema.gpa; + + var trash_block = block.makeSubBlock(); + trash_block.is_comptime = false; + trash_block.is_coerce_result_ptr = true; + defer trash_block.instructions.deinit(gpa); + + const mut_final_ptr_ty = try Type.ptr(sema.arena, sema.mod, .{ + .pointee_type = final_elem_ty, + .mutable = true, + .@"align" = inferred_alloc.data.alignment, + .@"addrspace" = target_util.defaultAddressSpace(target, .local), + }); + const dummy_ptr = try trash_block.addTy(.alloc, mut_final_ptr_ty); + const empty_trash_count = trash_block.instructions.items.len; + + for (placeholders) |bitcast_inst, i| { + const sub_ptr_ty = sema.typeOf(Air.indexToRef(bitcast_inst)); + + if (mut_final_ptr_ty.eql(sub_ptr_ty, sema.mod)) { + // New result location type is the same as the old one; nothing + // to do here. + continue; + } + + var bitcast_block = block.makeSubBlock(); + defer bitcast_block.instructions.deinit(gpa); + + trash_block.instructions.shrinkRetainingCapacity(empty_trash_count); + const pointee_ty = sema.typeOf(peer_inst_list[i]); + const sub_ptr = try coerceResultPtr(sema, &bitcast_block, src, ptr, dummy_ptr, pointee_ty, &trash_block); + + assert(bitcast_block.instructions.items.len > 0); + // If only one instruction is produced then we can replace the bitcast + // placeholder instruction with this instruction; no need for an entire block. + if (bitcast_block.instructions.items.len == 1) { + const only_inst = bitcast_block.instructions.items[0]; + sema.air_instructions.set(bitcast_inst, sema.air_instructions.get(only_inst)); + continue; + } + + // Here we replace the placeholder bitcast instruction with a block + // that does the coerce_result_ptr logic. + _ = try bitcast_block.addBr(bitcast_inst, sub_ptr); + const ty_inst = sema.air_instructions.items(.data)[bitcast_inst].ty_op.ty; + try sema.air_extra.ensureUnusedCapacity( + gpa, + @typeInfo(Air.Block).Struct.fields.len + bitcast_block.instructions.items.len, + ); + sema.air_instructions.set(bitcast_inst, .{ + .tag = .block, + .data = .{ .ty_pl = .{ + .ty = ty_inst, + .payload = sema.addExtraAssumeCapacity(Air.Block{ + .body_len = @intCast(u32, bitcast_block.instructions.items.len), + }), + } }, + }); + sema.air_extra.appendSliceAssumeCapacity(bitcast_block.instructions.items); + } }, else => unreachable, } @@ -4086,9 +4176,6 @@ fn storeToInferredAlloc( inferred_alloc: *Value.Payload.InferredAlloc, ) CompileError!void { const operand_ty = sema.typeOf(operand); - // Add the stored instruction to the set we will use to resolve peer types - // for the inferred allocation. - try inferred_alloc.data.stored_inst_list.append(sema.arena, operand); // Create a runtime bitcast instruction with exactly the type the pointer wants. const target = sema.mod.getTarget(); const ptr_ty = try Type.ptr(sema.arena, sema.mod, .{ @@ -4097,6 +4184,12 @@ fn storeToInferredAlloc( .@"addrspace" = target_util.defaultAddressSpace(target, .local), }); const bitcasted_ptr = try block.addBitCast(ptr_ty, ptr); + // Add the stored instruction to the set we will use to resolve peer types + // for the inferred allocation. + try inferred_alloc.data.prongs.append(sema.arena, .{ + .stored_inst = operand, + .placeholder = Air.refToIndex(bitcasted_ptr).?, + }); return sema.storePtr(block, src, bitcasted_ptr, operand); } diff --git a/src/value.zig b/src/value.zig index b52e67e31c..46624a822d 100644 --- a/src/value.zig +++ b/src/value.zig @@ -4935,7 +4935,16 @@ pub const Value = extern union { /// peer type resolution. This is stored in a separate list so that /// the items are contiguous in memory and thus can be passed to /// `Module.resolvePeerTypes`. - stored_inst_list: std.ArrayListUnmanaged(Air.Inst.Ref) = .{}, + prongs: std.MultiArrayList(struct { + /// The dummy instruction used as a peer to resolve the type. + /// Although this has a redundant type with placeholder, this is + /// needed in addition because it may be a constant value, which + /// affects peer type resolution. + stored_inst: Air.Inst.Ref, + /// The bitcast instruction used as a placeholder when the + /// new result pointer type is not yet known. + placeholder: Air.Inst.Index, + }) = .{}, /// 0 means ABI-aligned. alignment: u32, }, diff --git a/test/behavior/if.zig b/test/behavior/if.zig index a78bf74502..7618363474 100644 --- a/test/behavior/if.zig +++ b/test/behavior/if.zig @@ -128,3 +128,15 @@ test "if peer expressions inferred optional type" { try expect(left.? == 98); try expect(right.? == 99); } + +test "if-else expression with runtime condition result location is inferred optional" { + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; + + const A = struct { b: u64, c: u64 }; + var d: bool = true; + const e = if (d) A{ .b = 15, .c = 30 } else null; + try expect(e != null); +} From d7711ec9532d4c38bf1911f5c0ed2c813705ae15 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 17:16:07 -0700 Subject: [PATCH 2/3] print_zir: fix wrong union tag for validate_deref closes #12125 --- src/print_zir.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/print_zir.zig b/src/print_zir.zig index 8df8eaae07..b7ec089ce8 100644 --- a/src/print_zir.zig +++ b/src/print_zir.zig @@ -232,13 +232,13 @@ const Writer = struct { .validate_array_init_ty, .validate_struct_init_ty, .make_ptr_const, + .validate_deref, => try self.writeUnNode(stream, inst), .ref, .ret_tok, .ensure_err_payload_void, .closure_capture, - .validate_deref, => try self.writeUnTok(stream, inst), .bool_br_and, From 04572f6e341e6ff19877d1ae3b79e3baa653e652 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 18:10:54 -0700 Subject: [PATCH 3/3] Sema: fix coerceResultPtr It did not handle properly when the dummy operand was a comptime_int; it was crashing in coerce because comptime_int is supposed to be comptime-known. So when calling coerceResultPtr, we pass the actual operand, not a dummy operand, which means it will have the proper comptime value when necessary. --- src/Sema.zig | 42 +++++++++++++++++++++-------------------- test/behavior/array.zig | 5 +---- test/behavior/if.zig | 16 ++++++++++++++++ 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index d4de78e5d1..92df7a6b1f 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -2030,7 +2030,8 @@ fn zirCoerceResultPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileE defer trash_block.instructions.deinit(sema.gpa); const dummy_ptr = try trash_block.addTy(.alloc, sema.typeOf(ptr)); - return coerceResultPtr(sema, block, src, ptr, dummy_ptr, pointee_ty, &trash_block); + const dummy_operand = try trash_block.addBitCast(pointee_ty, .void_value); + return coerceResultPtr(sema, block, src, ptr, dummy_ptr, dummy_operand, &trash_block); } fn coerceResultPtr( @@ -2039,13 +2040,14 @@ fn coerceResultPtr( src: LazySrcLoc, ptr: Air.Inst.Ref, dummy_ptr: Air.Inst.Ref, - pointee_ty: Type, + dummy_operand: Air.Inst.Ref, trash_block: *Block, ) CompileError!Air.Inst.Ref { const target = sema.mod.getTarget(); const addr_space = target_util.defaultAddressSpace(target, .local); + const pointee_ty = sema.typeOf(dummy_operand); + const prev_trash_len = trash_block.instructions.items.len; - const dummy_operand = try trash_block.addBitCast(pointee_ty, .void_value); try sema.storePtr2(trash_block, src, dummy_ptr, src, dummy_operand, src, .bitcast); { @@ -2078,21 +2080,24 @@ fn coerceResultPtr( while (true) { const air_tags = sema.air_instructions.items(.tag); const air_datas = sema.air_instructions.items(.data); + + if (trash_block.instructions.items.len == prev_trash_len) { + if (try sema.resolveDefinedValue(block, src, new_ptr)) |ptr_val| { + return sema.addConstant(ptr_ty, ptr_val); + } + if (pointee_ty.eql(Type.@"null", sema.mod)) { + const opt_ty = sema.typeOf(new_ptr).childType(); + const null_inst = try sema.addConstant(opt_ty, Value.@"null"); + _ = try block.addBinOp(.store, new_ptr, null_inst); + return Air.Inst.Ref.void_value; + } + return sema.bitCast(block, ptr_ty, new_ptr, src); + } + const trash_inst = trash_block.instructions.pop(); + switch (air_tags[trash_inst]) { .bitcast => { - if (Air.indexToRef(trash_inst) == dummy_operand) { - if (try sema.resolveDefinedValue(block, src, new_ptr)) |ptr_val| { - return sema.addConstant(ptr_ty, ptr_val); - } - if (pointee_ty.eql(Type.@"null", sema.mod)) { - const opt_ty = sema.typeOf(new_ptr).childType(); - const null_inst = try sema.addConstant(opt_ty, Value.@"null"); - _ = try block.addBinOp(.store, new_ptr, null_inst); - return Air.Inst.Ref.void_value; - } - return sema.bitCast(block, ptr_ty, new_ptr, src); - } const ty_op = air_datas[trash_inst].ty_op; const operand_ty = sema.typeOf(ty_op.operand); const ptr_operand_ty = try Type.ptr(sema.arena, sema.mod, .{ @@ -3309,8 +3314,7 @@ fn zirResolveInferredAlloc(sema: *Sema, block: *Block, inst: Zir.Inst.Index) Com defer bitcast_block.instructions.deinit(gpa); trash_block.instructions.shrinkRetainingCapacity(empty_trash_count); - const pointee_ty = sema.typeOf(peer_inst_list[i]); - const sub_ptr = try coerceResultPtr(sema, &bitcast_block, src, ptr, dummy_ptr, pointee_ty, &trash_block); + const sub_ptr = try coerceResultPtr(sema, &bitcast_block, src, ptr, dummy_ptr, peer_inst_list[i], &trash_block); assert(bitcast_block.instructions.items.len > 0); // If only one instruction is produced then we can replace the bitcast @@ -4190,7 +4194,7 @@ fn storeToInferredAlloc( .stored_inst = operand, .placeholder = Air.refToIndex(bitcasted_ptr).?, }); - return sema.storePtr(block, src, bitcasted_ptr, operand); + return sema.storePtr2(block, src, bitcasted_ptr, src, operand, src, .bitcast); } fn storeToInferredAllocComptime( @@ -21707,8 +21711,6 @@ fn storePtr2( if ((try sema.typeHasOnePossibleValue(block, src, elem_ty)) != null) return; - // TODO handle if the element type requires comptime - if (air_tag == .bitcast) { // `air_tag == .bitcast` is used as a special case for `zirCoerceResultPtr` // to avoid calling `requireRuntimeBlock` for the dummy block. diff --git a/test/behavior/array.zig b/test/behavior/array.zig index aea4dd357b..b99ac27651 100644 --- a/test/behavior/array.zig +++ b/test/behavior/array.zig @@ -570,10 +570,7 @@ test "type coercion of pointer to anon struct literal to pointer to array" { } test "array with comptime only element type" { - const a = [_]type{ - u32, - i32, - }; + const a = [_]type{ u32, i32 }; try testing.expect(a[0] == u32); try testing.expect(a[1] == i32); } diff --git a/test/behavior/if.zig b/test/behavior/if.zig index 7618363474..8117c1db3f 100644 --- a/test/behavior/if.zig +++ b/test/behavior/if.zig @@ -140,3 +140,19 @@ test "if-else expression with runtime condition result location is inferred opti const e = if (d) A{ .b = 15, .c = 30 } else null; try expect(e != null); } + +test "result location with inferred type ends up being pointer to comptime_int" { + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; + + var a: ?u32 = 1234; + var b: u32 = 2000; + var c = if (a) |d| blk: { + if (d < b) break :blk @as(u32, 1); + break :blk 0; + } else @as(u32, 0); + try expect(c == 1); +}