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.

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

Conversation

foobles
Copy link
Contributor

@foobles foobles commented Nov 7, 2020

This should close #1469 and close #6861. This change prevents doing a @ptrCast() between any two pointer types with different memory layouts.

While not explicitly stated it should be disallowed in #1469, this change also prevents a cast from a normal-sized pointer to a 1-bit pointer (optional zero-sized pointer, e.g., ?*void)

@@ -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.

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

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

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.

@foobles foobles closed this Nov 25, 2020
@foobles foobles deleted the disallow-ptr-cast-different-sizes branch November 25, 2020 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants