From 418197b6c51b99ee2a0be5e394753868f7dbf982 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 16 Mar 2022 12:21:48 -0700 Subject: [PATCH 1/2] stage2: elem_ptr needs to know if slice or direct access This fixes one of the major issues plaguing the `std.sort` comptime tests. The high level issue is that at comptime, we need to know whether `elem_ptr` is being used to subslice an array-like pointer or access a child value. High-level example: var x: [2][2]i32 = undefined; var a = &x[0]; // elem_ptr, type *[2]i32 var y: [5]i32 = undefined; var b = y[1..3]; // elem_ptr, type *[2]i32 `a` is pointing directly to the 0th element of `x`. But `b` is subslicing the 1st and 2nd element of `y`. At runtime with a well defined memory layout, this is an inconsequential detail. At comptime, the values aren't laid out exactly in-memory so we need to know the difference. This becomes an issue specifically in this case: var c: []i32 = a; var d: []i32 = b; When converting the `*[N]T` to `[]T` we need to know what array to point to. For runtime, its all the same. For comptime, we need to know if its the parent array or the child value. See the behavior tests for more details. This commit fixes this by adding a boolean to track this on the `elem_ptr`. We can't just immediately deref the child for `&x[0]` because it is legal to ptrCast it to a many-pointer, do arithmetic, and then cast it back (see behavior test) so we need to retain access to the "parent" indexable. --- src/Sema.zig | 2 +- src/value.zig | 28 +++++++++++++++- test/behavior/pointers.zig | 68 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index 8a762d91d423cc29f6db35e5b94b5b582f0c3538..4a69c5b8958bc0388101f6c848d3fb750ae3b093 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -16739,7 +16739,7 @@ fn elemPtrArray( const index_u64 = index_val.toUnsignedInt(); // @intCast here because it would have been impossible to construct a value that // required a larger index. - const elem_ptr = try array_ptr_val.elemPtr(array_ptr_ty, sema.arena, @intCast(usize, index_u64)); + const elem_ptr = try array_ptr_val.elemPtrDirect(array_ptr_ty, sema.arena, @intCast(usize, index_u64)); return sema.addConstant(result_ty, elem_ptr); } } diff --git a/src/value.zig b/src/value.zig index d63452ee5640f5156184842d66385b1534971013..2ff196d4911c7e1e31cb3a92e07a83901149f62e 100644 --- a/src/value.zig +++ b/src/value.zig @@ -505,6 +505,7 @@ pub const Value = extern union { .array_ptr = try payload.data.array_ptr.copy(arena), .elem_ty = try payload.data.elem_ty.copy(arena), .index = payload.data.index, + .direct = payload.data.direct, }, }; return Value{ .ptr_otherwise = &new_payload.base }; @@ -2402,7 +2403,11 @@ pub const Value = extern union { .decl_ref_mut => return val.castTag(.decl_ref_mut).?.data.decl.val.elemValueAdvanced(index, arena, buffer), .elem_ptr => { const data = val.castTag(.elem_ptr).?.data; - return data.array_ptr.elemValueAdvanced(index + data.index, arena, buffer); + if (!data.direct) + return data.array_ptr.elemValueAdvanced(index + data.index, arena, buffer); + + const underlying = try data.array_ptr.elemValueAdvanced(data.index, arena, buffer); + return underlying.elemValueAdvanced(index, arena, buffer); }, // The child type of arrays which have only one possible value need @@ -2465,12 +2470,25 @@ pub const Value = extern union { /// Returns a pointer to the element value at the index. pub fn elemPtr(val: Value, ty: Type, arena: Allocator, index: usize) Allocator.Error!Value { + return val.elemPtrAdvanced(ty, arena, index, false); + } + + /// Returns a pointer to the element value at the index. The behavior + /// of this is slightly different for comptime; the "direct" means that + /// indexing indexes the referenced child value, not the parent array. + pub fn elemPtrDirect(val: Value, ty: Type, arena: Allocator, index: usize) Allocator.Error!Value { + return val.elemPtrAdvanced(ty, arena, index, true); + } + + pub fn elemPtrAdvanced(val: Value, ty: Type, arena: Allocator, index: usize, direct: bool) Allocator.Error!Value { const elem_ty = ty.elemType2(); const ptr_val = switch (val.tag()) { .slice => val.castTag(.slice).?.data.ptr, else => val, }; + // If the val is already an elem ptr, then we do ptr arithmetic logic + // and just move the index. if (ptr_val.tag() == .elem_ptr) { const elem_ptr = ptr_val.castTag(.elem_ptr).?.data; if (elem_ptr.elem_ty.eql(elem_ty)) { @@ -2478,6 +2496,12 @@ pub const Value = extern union { .array_ptr = elem_ptr.array_ptr, .elem_ty = elem_ptr.elem_ty, .index = elem_ptr.index + index, + + // Retain the direct preference. This enables a direct + // elem ptr (i.e. &arr[0]) to be bitcasted to a many-pointer + // with pointer arithmetic then casted back to a single + // pointer. + .direct = elem_ptr.direct, }); } } @@ -2485,6 +2509,7 @@ pub const Value = extern union { .array_ptr = ptr_val, .elem_ty = elem_ty, .index = index, + .direct = direct, }); } @@ -4194,6 +4219,7 @@ pub const Value = extern union { array_ptr: Value, elem_ty: Type, index: usize, + direct: bool, }, }; diff --git a/test/behavior/pointers.zig b/test/behavior/pointers.zig index 74089611db77dcb91e8eb4da3e52f57ee96dc423..96200e73fc070a1576162972b4a99d97d12b2f1f 100644 --- a/test/behavior/pointers.zig +++ b/test/behavior/pointers.zig @@ -437,3 +437,71 @@ test "indexing array with sentinel returns correct type" { var s: [:0]const u8 = "abc"; try testing.expectEqualSlices(u8, "*const u8", @typeName(@TypeOf(&s[0]))); } + +test "element pointer to slice" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest() !void { + var cases: [2][2]i32 = [_][2]i32{ + [_]i32{ 0, 1 }, + [_]i32{ 2, 3 }, + }; + + const items: []i32 = &cases[0]; // *[2]i32 + try testing.expect(items.len == 2); + try testing.expect(items[1] == 1); + try testing.expect(items[0] == 0); + } + }; + + try S.doTheTest(); + comptime try S.doTheTest(); +} + +test "element pointer arithmetic to slice" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest() !void { + var cases: [2][2]i32 = [_][2]i32{ + [_]i32{ 0, 1 }, + [_]i32{ 2, 3 }, + }; + + const elem_ptr = &cases[0]; // *[2]i32 + const many = @ptrCast([*][2]i32, elem_ptr); + const many_elem = @ptrCast(*[2]i32, &many[1]); + const items: []i32 = many_elem; + try testing.expect(items.len == 2); + try testing.expect(items[1] == 3); + try testing.expect(items[0] == 2); + } + }; + + try S.doTheTest(); + comptime try S.doTheTest(); +} + +test "array slicing to slice" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest() !void { + var str: [5]i32 = [_]i32{ 1, 2, 3, 4, 5 }; + var sub: *[2]i32 = str[1..3]; + var slice: []i32 = sub; // used to cause failures + try testing.expect(slice.len == 2); + try testing.expect(slice[0] == 2); + } + }; + + try S.doTheTest(); + comptime try S.doTheTest(); +} -- 2.54.0 From cdeb1fb88157130ba1260e6c2d4ef5a20df0c493 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 16 Mar 2022 16:29:08 -0700 Subject: [PATCH 2/2] Sema: different solution to elemVal I think that reusing the ComptimePtrLoad infrastructure is ultimately less logic and more robust than adding a `direct` flag to elem_ptr. * Some code in zirTypeInfo needed to be fixed to create proper Type/Value encodings. * comptime elemVal works by constructing an elem_ptr Value and then using the already existing pointerDeref function. There are some remaining calls to Value.elemValue which should be considered code smells at this point. --- src/Sema.zig | 78 +++++++++++++++++++++++++++++++++++++++++---------- src/value.zig | 28 +----------------- 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index 4a69c5b8958bc0388101f6c848d3fb750ae3b093..3df23ca2df8879cbdc363fd2b02cfe1b525d3132 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -10388,25 +10388,32 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai )).?; try sema.mod.declareDeclDependency(sema.owner_decl, fn_info_decl); try sema.ensureDeclAnalyzed(fn_info_decl); + var fn_ty_buffer: Value.ToTypeBuffer = undefined; + const fn_ty = fn_info_decl.val.toType(&fn_ty_buffer); const param_info_decl = (try sema.namespaceLookup( block, src, - fn_info_decl.val.castTag(.ty).?.data.getNamespace().?, + fn_ty.getNamespace().?, "Param", )).?; try sema.mod.declareDeclDependency(sema.owner_decl, param_info_decl); try sema.ensureDeclAnalyzed(param_info_decl); + var param_buffer: Value.ToTypeBuffer = undefined; + const param_ty = param_info_decl.val.toType(¶m_buffer); const new_decl = try params_anon_decl.finish( try Type.Tag.array.create(params_anon_decl.arena(), .{ .len = param_vals.len, - .elem_type = param_info_decl.ty, + .elem_type = try param_ty.copy(params_anon_decl.arena()), }), try Value.Tag.aggregate.create( params_anon_decl.arena(), param_vals, ), ); - break :v try Value.Tag.decl_ref.create(sema.arena, new_decl); + break :v try Value.Tag.slice.create(sema.arena, .{ + .ptr = try Value.Tag.decl_ref.create(sema.arena, new_decl), + .len = try Value.Tag.int_u64.create(sema.arena, param_vals.len), + }); }; const ret_ty_opt = if (info.return_type.tag() != .generic_poison) @@ -10823,7 +10830,10 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai try fields_anon_decl.arena().dupe(Value, union_field_vals), ), ); - break :v try Value.Tag.decl_ref.create(sema.arena, new_decl); + break :v try Value.Tag.slice.create(sema.arena, .{ + .ptr = try Value.Tag.decl_ref.create(sema.arena, new_decl), + .len = try Value.Tag.int_u64.create(sema.arena, union_field_vals.len), + }); }; const decls_val = try sema.typeInfoDecls(block, src, type_info_ty, union_ty.getNamespace()); @@ -10897,7 +10907,10 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai try Type.Tag.array_u8_sentinel_0.create(anon_decl.arena(), bytes.len), try Value.Tag.bytes.create(anon_decl.arena(), bytes[0 .. bytes.len + 1]), ); - break :v try Value.Tag.decl_ref.create(fields_anon_decl.arena(), new_decl); + break :v try Value.Tag.slice.create(sema.arena, .{ + .ptr = try Value.Tag.decl_ref.create(fields_anon_decl.arena(), new_decl), + .len = try Value.Tag.int_u64.create(sema.arena, bytes.len), + }); }; const struct_field_fields = try fields_anon_decl.arena().create([5]Value); @@ -10937,7 +10950,10 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai try Type.Tag.array_u8_sentinel_0.create(anon_decl.arena(), bytes.len), try Value.Tag.bytes.create(anon_decl.arena(), bytes[0 .. bytes.len + 1]), ); - break :v try Value.Tag.decl_ref.create(fields_anon_decl.arena(), new_decl); + break :v try Value.Tag.slice.create(sema.arena, .{ + .ptr = try Value.Tag.decl_ref.create(fields_anon_decl.arena(), new_decl), + .len = try Value.Tag.int_u64.create(sema.arena, bytes.len), + }); }; const struct_field_fields = try fields_anon_decl.arena().create([5]Value); @@ -10979,7 +10995,10 @@ fn zirTypeInfo(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Ai try fields_anon_decl.arena().dupe(Value, struct_field_vals), ), ); - break :v try Value.Tag.decl_ref.create(sema.arena, new_decl); + break :v try Value.Tag.slice.create(sema.arena, .{ + .ptr = try Value.Tag.decl_ref.create(sema.arena, new_decl), + .len = try Value.Tag.int_u64.create(sema.arena, struct_field_vals.len), + }); }; const decls_val = try sema.typeInfoDecls(block, src, type_info_ty, struct_ty.getNamespace()); @@ -11048,7 +11067,7 @@ fn typeInfoDecls( block, src, type_info_ty.getNamespace().?, - "EnumField", + "Declaration", )).?; try sema.mod.declareDeclDependency(sema.owner_decl, declaration_ty_decl); try sema.ensureDeclAnalyzed(declaration_ty_decl); @@ -11069,7 +11088,10 @@ fn typeInfoDecls( try Type.Tag.array_u8_sentinel_0.create(anon_decl.arena(), bytes.len), try Value.Tag.bytes.create(anon_decl.arena(), bytes[0 .. bytes.len + 1]), ); - break :v try Value.Tag.decl_ref.create(decls_anon_decl.arena(), new_decl); + break :v try Value.Tag.slice.create(decls_anon_decl.arena(), .{ + .ptr = try Value.Tag.decl_ref.create(decls_anon_decl.arena(), new_decl), + .len = try Value.Tag.int_u64.create(decls_anon_decl.arena(), bytes.len), + }); }; const fields = try decls_anon_decl.arena().create([2]Value); @@ -11092,7 +11114,10 @@ fn typeInfoDecls( try decls_anon_decl.arena().dupe(Value, decls_vals), ), ); - return try Value.Tag.decl_ref.create(sema.arena, new_decl); + return try Value.Tag.slice.create(sema.arena, .{ + .ptr = try Value.Tag.decl_ref.create(sema.arena, new_decl), + .len = try Value.Tag.int_u64.create(sema.arena, decls_vals.len), + }); } fn zirTypeof(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Air.Inst.Ref { @@ -16574,8 +16599,20 @@ fn elemVal( const runtime_src = if (maybe_slice_val) |slice_val| rs: { const index_val = maybe_index_val orelse break :rs elem_index_src; const index = @intCast(usize, index_val.toUnsignedInt()); - const elem_val = try slice_val.elemValue(sema.arena, index); - return sema.addConstant(array_ty.elemType2(), elem_val); + + const elem_ty = array_ty.elemType2(); + + var payload: Value.Payload.ElemPtr = .{ .data = .{ + .array_ptr = slice_val.slicePtr(), + .elem_ty = elem_ty, + .index = index, + } }; + const elem_ptr_val = Value.initPayload(&payload.base); + + if (try sema.pointerDeref(block, array_src, elem_ptr_val, array_ty)) |elem_val| { + return sema.addConstant(elem_ty, elem_val); + } + break :rs array_src; } else array_src; try sema.requireRuntimeBlock(block, runtime_src); @@ -16589,8 +16626,19 @@ fn elemVal( const array_val = maybe_array_val orelse break :rs array_src; const index_val = maybe_index_val orelse break :rs elem_index_src; const index = @intCast(usize, index_val.toUnsignedInt()); - const elem_val = try array_val.elemValue(sema.arena, index); - return sema.addConstant(array_ty.elemType2(), elem_val); + const elem_ty = array_ty.elemType2(); + + var payload: Value.Payload.ElemPtr = .{ .data = .{ + .array_ptr = array_val, + .elem_ty = elem_ty, + .index = index, + } }; + const elem_ptr_val = Value.initPayload(&payload.base); + + if (try sema.pointerDeref(block, array_src, elem_ptr_val, array_ty)) |elem_val| { + return sema.addConstant(elem_ty, elem_val); + } + break :rs array_src; }; try sema.requireRuntimeBlock(block, runtime_src); @@ -16739,7 +16787,7 @@ fn elemPtrArray( const index_u64 = index_val.toUnsignedInt(); // @intCast here because it would have been impossible to construct a value that // required a larger index. - const elem_ptr = try array_ptr_val.elemPtrDirect(array_ptr_ty, sema.arena, @intCast(usize, index_u64)); + const elem_ptr = try array_ptr_val.elemPtr(array_ptr_ty, sema.arena, @intCast(usize, index_u64)); return sema.addConstant(result_ty, elem_ptr); } } diff --git a/src/value.zig b/src/value.zig index 2ff196d4911c7e1e31cb3a92e07a83901149f62e..d63452ee5640f5156184842d66385b1534971013 100644 --- a/src/value.zig +++ b/src/value.zig @@ -505,7 +505,6 @@ pub const Value = extern union { .array_ptr = try payload.data.array_ptr.copy(arena), .elem_ty = try payload.data.elem_ty.copy(arena), .index = payload.data.index, - .direct = payload.data.direct, }, }; return Value{ .ptr_otherwise = &new_payload.base }; @@ -2403,11 +2402,7 @@ pub const Value = extern union { .decl_ref_mut => return val.castTag(.decl_ref_mut).?.data.decl.val.elemValueAdvanced(index, arena, buffer), .elem_ptr => { const data = val.castTag(.elem_ptr).?.data; - if (!data.direct) - return data.array_ptr.elemValueAdvanced(index + data.index, arena, buffer); - - const underlying = try data.array_ptr.elemValueAdvanced(data.index, arena, buffer); - return underlying.elemValueAdvanced(index, arena, buffer); + return data.array_ptr.elemValueAdvanced(index + data.index, arena, buffer); }, // The child type of arrays which have only one possible value need @@ -2470,25 +2465,12 @@ pub const Value = extern union { /// Returns a pointer to the element value at the index. pub fn elemPtr(val: Value, ty: Type, arena: Allocator, index: usize) Allocator.Error!Value { - return val.elemPtrAdvanced(ty, arena, index, false); - } - - /// Returns a pointer to the element value at the index. The behavior - /// of this is slightly different for comptime; the "direct" means that - /// indexing indexes the referenced child value, not the parent array. - pub fn elemPtrDirect(val: Value, ty: Type, arena: Allocator, index: usize) Allocator.Error!Value { - return val.elemPtrAdvanced(ty, arena, index, true); - } - - pub fn elemPtrAdvanced(val: Value, ty: Type, arena: Allocator, index: usize, direct: bool) Allocator.Error!Value { const elem_ty = ty.elemType2(); const ptr_val = switch (val.tag()) { .slice => val.castTag(.slice).?.data.ptr, else => val, }; - // If the val is already an elem ptr, then we do ptr arithmetic logic - // and just move the index. if (ptr_val.tag() == .elem_ptr) { const elem_ptr = ptr_val.castTag(.elem_ptr).?.data; if (elem_ptr.elem_ty.eql(elem_ty)) { @@ -2496,12 +2478,6 @@ pub const Value = extern union { .array_ptr = elem_ptr.array_ptr, .elem_ty = elem_ptr.elem_ty, .index = elem_ptr.index + index, - - // Retain the direct preference. This enables a direct - // elem ptr (i.e. &arr[0]) to be bitcasted to a many-pointer - // with pointer arithmetic then casted back to a single - // pointer. - .direct = elem_ptr.direct, }); } } @@ -2509,7 +2485,6 @@ pub const Value = extern union { .array_ptr = ptr_val, .elem_ty = elem_ty, .index = index, - .direct = direct, }); } @@ -4219,7 +4194,6 @@ pub const Value = extern union { array_ptr: Value, elem_ty: Type, index: usize, - direct: bool, }, }; -- 2.54.0