Skip to content

ptrCast between pointer types of different sizes no longer allowed #7022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

ptrCast between pointer types of different sizes no longer allowed #7022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/std/heap/general_purpose_allocator.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,12 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
const start_ptr = @ptrCast([*]u8, bucket) + bucketStackFramesStart(size_class);
const addr = start_ptr + one_trace_size * traces_per_slot * slot_index +
@enumToInt(trace_kind) * @as(usize, one_trace_size);
return @ptrCast(*[stack_n]usize, @alignCast(@alignOf(usize), addr));
if (stack_n > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, you can save one level of indentation by handling the exceptional case before the general one:

if (stack_n == 0) return &[_]usize{};
return @ptrCast(*[stack_n]usize, @alignCast(@alignOf(usize), addr));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could do that, but it would have to look more like

comptime var empty_arr = [_]usize{};
if (stack_n == 0) return &empty_arr;
return @ptrCast(*[stack_n]usize, @alignCast(@alignOf(usize), addr));

Because in your fix, it doesn't work since &[_]usize{} is a const pointer.

Maybe this would be better too, since we won't need 2 return statements:

comptime var empty_arr = [_]usize{};
return if (stack_n != 0) 
    @ptrCast(*[stack_n]usize, @alignCast(@alignOf(usize), addr))
else
    &empty_arr;

However, the downside to that is that the compiler will generate empty_arr even in the case where stack_n is not zero. But due to lazy evaluation this is maybe ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in your fix, it doesn't work since &[_]usize{} is a const pointer.

Welp, I didn't expect the constness to matter there.
You're basically forced to return a local pointer to a zero sized variable, it's not that nice :\

But due to lazy evaluation this is maybe ok?

It's an empty array, its size won't matter much/at all.

return @ptrCast(*[stack_n]usize, @alignCast(@alignOf(usize), addr));
} else {
comptime var ret = [0]usize {};
return &ret;
}
}

fn captureStackTrace(
Expand Down
21 changes: 13 additions & 8 deletions src/stage1/ir.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29850,23 +29850,28 @@ static IrInstGen *ir_analyze_ptr_cast(IrAnalyze *ira, IrInst* source_instr, IrIn
return ira->codegen->invalid_inst_gen;
}

if ((err = type_resolve(ira->codegen, dest_type, ResolveStatusZeroBitsKnown)))
if ((err = type_resolve(ira->codegen, dest_type, ResolveStatusSizeKnown)))
return ira->codegen->invalid_inst_gen;

if ((err = type_resolve(ira->codegen, src_type, ResolveStatusZeroBitsKnown)))
if ((err = type_resolve(ira->codegen, src_type, ResolveStatusSizeKnown)))
return ira->codegen->invalid_inst_gen;

if (safety_check_on &&
type_has_bits(ira->codegen, dest_type) &&
!type_has_bits(ira->codegen, if_slice_ptr_type))
{
size_t src_size = type_size_bits(ira->codegen, if_slice_ptr_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR pointers are either usize*8 or zero bits wide, is there some other edge case in between?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major case this intends to fix is an optional zero-sized pointer, which are 1 bit large. Do those not count as pointers? I know ?*allowzero T doesn't count as a pointer type, but I wasn't sure if this case was considered the same way.

Copy link

@kyle-github kyle-github Nov 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly in the past, pointers in C are not always a single machine word. Think far and near pointers in the DOS era. Also, the x32 mode for x86 CPUs does weird things where pointers are 32-bit. Java had/has a mode in the JVM where 32-bit pointers were used in 64-bit mode so registers could be 64-bit but pointers were smaller.

The C specification goes to some lengths to make sure that the size of a pointer is not specified. As long as usize is always the equivalent of C's uintptr_t then you can fit a pointer into it, but if Zig supports different memory models like the ones I noted above, pointers are not going to always be the same size.

Whether that is something that Zig wants to support, different pointer sizes, is a different story!

(edit: English is hard without caffeine!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The major case this intends to fix is an optional zero-sized pointer, which are 1 bit large. Do those not count as pointers?

Those should probably decay to an optional type with no payload that in turn decay into a u1 tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LemonBoy Yeah, but should they be allowed as the output/input type of @ptrCast?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting "down" from a regular pointer to a zero-sized pointer should be probably allowed as that's safe while the other way around should be prohibited as that's a great way to shoot yourself in the foot.

Copy link
Contributor Author

@foobles foobles Nov 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LemonBoy #1469, an accepted proposal this is meant to close, disallows casting from a sized to zero-sized pointer.

But should *void be able to be casted to ?*void? They have different memory layouts.

What makes me think that it shouldn't is that you can't cast *allowzero T to a ?*allowzero T, which sets a precedent. In fact, the same way that ?*allowzero T isn't even considered a "pointer type", I think I should change this PR so that ?*ZeroSizedType isn't considered a pointer-type either.

size_t dest_size = type_size_bits(ira->codegen, dest_type);
if (safety_check_on && src_size != dest_size) {
ErrorMsg *msg = ir_add_error(ira, source_instr,
buf_sprintf("'%s' and '%s' do not have the same in-memory representation",
buf_ptr(&src_type->name), buf_ptr(&dest_type->name)));
add_error_note(ira->codegen, msg, ptr_src->source_node,
buf_sprintf("'%s' has no in-memory bits", buf_ptr(&src_type->name)));
buf_sprintf("'%s' has a pointer size of %zu %s",
buf_ptr(&src_type->name),
src_size,
src_size == 1? "bit" : "bits"));
add_error_note(ira->codegen, msg, dest_type_src->source_node,
buf_sprintf("'%s' has in-memory bits", buf_ptr(&dest_type->name)));
buf_sprintf("'%s' has a pointer size of %zu %s",
buf_ptr(&dest_type->name),
dest_size,
dest_size == 1? "bit" : "bits"));
return ira->codegen->invalid_inst_gen;
}

Expand Down
39 changes: 31 additions & 8 deletions test/compile_errors.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1237,14 +1237,37 @@ pub fn addCases(cases: *tests.CompileErrorContext) void {
"tmp.zig:2:11: error: use of undefined value here causes undefined behavior",
});

cases.add("comptime ptrcast of zero-sized type",
\\fn foo() void {
\\ const node: struct {} = undefined;
\\ const vla_ptr = @ptrCast([*]const u8, &node);
cases.add("ptrcast pointers with different bit sizes",
\\var num: i32 = 10;
\\var empty: void = {};
\\var normal_sized_ptr: *i32 = #
\\var zero_sized_ptr: *void = ∅
\\var one_bit_ptr: ?*void = ∅
\\export fn normalToZero() void {
\\ var x = @ptrCast(*void, normal_sized_ptr);
\\}
\\export fn normalToOne() void {
\\ var x = @ptrCast(?*void, normal_sized_ptr);
\\}
\\export fn zeroToNormal() void {
\\ var x = @ptrCast(*i32, zero_sized_ptr);
\\}
\\export fn zeroToOne() void {
\\ var x = @ptrCast(?*void, zero_sized_ptr);
\\}
\\export fn oneToNormal() void {
\\ var x = @ptrCast(*i32, one_bit_ptr);
\\}
\\export fn oneToZero() void {
\\ var x = @ptrCast(*void, one_bit_ptr);
\\}
\\comptime { foo(); }
, &[_][]const u8{
"tmp.zig:3:21: error: '*const struct:2:17' and '[*]const u8' do not have the same in-memory representation",
"tmp.zig:7:13: error: '*i32' and '*void' do not have the same in-memory representation",
"tmp.zig:10:13: error: '*i32' and '?*void' do not have the same in-memory representation",
"tmp.zig:13:13: error: '*void' and '*i32' do not have the same in-memory representation",
"tmp.zig:16:13: error: '*void' and '?*void' do not have the same in-memory representation",
"tmp.zig:19:13: error: '?*void' and '*i32' do not have the same in-memory representation",
"tmp.zig:22:13: error: '?*void' and '*void' do not have the same in-memory representation",
});

cases.add("slice sentinel mismatch",
Expand Down Expand Up @@ -3145,8 +3168,8 @@ pub fn addCases(cases: *tests.CompileErrorContext) void {
\\}
, &[_][]const u8{
"tmp.zig:3:15: error: '*u0' and '?*u0' do not have the same in-memory representation",
"tmp.zig:3:31: note: '*u0' has no in-memory bits",
"tmp.zig:3:24: note: '?*u0' has in-memory bits",
"tmp.zig:3:31: note: '*u0' has a pointer size of 0 bits",
"tmp.zig:3:24: note: '?*u0' has a pointer size of 1 bit",
});

cases.add("comparing a non-optional pointer against null",
Expand Down
6 changes: 0 additions & 6 deletions test/stage1/behavior/cast.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,12 +513,6 @@ fn incrementVoidPtrArray(array: ?*c_void, len: usize) void {
}
}

test "*usize to *void" {
var i = @as(usize, 0);
var v = @ptrCast(*void, &i);
v.* = {};
}

test "compile time int to ptr of function" {
foobar(FUNCTION_CONSTANT);
}
Expand Down