Skip to content
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

Emit null or uninitialized pointers as 0 values. #8372

Closed
wants to merge 1 commit into from

Conversation

haberman
Copy link

This fixes a bug where IR analysis would fail when trying to
overwrite a null or uninitialized pointer. In particular this
made std.mem.zeroes() fail on any struct that contained a pointer.

Fixes one half of #7865 (the case where the pointer is not optional).

This fixes a bug where IR analysis would fail when trying to
overwrite a null or uninitialized pointer. In particular this
made std.mem.zeroes() fail on any struct that contained a pointer.
bigint_init_unsigned(&bn, val->data.x_ptr.data.hard_coded_addr.addr);
break;
case ConstPtrSpecialNull:
case ConstPtrSpecialInvalid:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case ConstPtrSpecialInvalid:

ConstPtrSpecialInvalid is there so that zero-initialized variables have an invalid value.

// Enforce explicitly setting this ID by making the zero value invalid.
ConstPtrSpecialInvalid,

Undefined is signalled by ConstValSpecialUndef.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, the following program fails if I remove this line (and it is not fixed by adding case ConstValSpecialUndef):

fn zero() [*c]u8 {
    var ret: [*c]u8 = undefined;
    @ptrCast(*[8]u8, &ret)[1] = 123;  // Succeeds if you remove this.
    return ret;
}

export const foo = zero();

I wonder if there is a bug somewhere else in the compiler where ConstValSpecialUndef is not getting set appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should have been clearer. ConstValSpecialUndef belongs to a different enum (notice Val instead of Ptr). See ZigValue.special.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also keep in mind that the plan is to eventually throw away the C++ code: #6378.
To fix some of the bugs you would need to rewrite quite a bit of code.

Copy link
Author

Choose a reason for hiding this comment

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

I should have been clearer. ConstValSpecialUndef belongs to a different enum (notice Val instead of Ptr).

I see. But it looks like ConstPtrValue.special is the discriminator that must be valid before we can read any other member of ConstPtrValue.data, no? It seems like we have to read ConstPtrValue.data before we can test it against ConstValSpecialUndef, but we can't do that without a valid discriminator. So it still seems like a bug that val->data.x_ptr.special == ConstPtrSpecialInvalid.

To fix some of the bugs you would need to rewrite quite a bit of code.

Hmm, that's too bad. I guess it's throwaway work to fix bugs here if everything is going to be rewritten in Zig?

@haberman
Copy link
Author

haberman commented Apr 3, 2021

What is the status of this PR?

The PR as written does seem to fix a bug, and I'm not aware of it introducing any other bugs.

It is perhaps working around a different bug, in which an undefined value has val->data.x_ptr.special == ConstPtrSpecialInvalid instead of val->data.x_ptr.special == <something else> && val->data.x_ptr.data.<something else>->special == ConstValSpecialUndef.

What is the way forward here?

  1. Accept the PR even though it's working around a different bug?
  2. Accept the PR once it fixes the other bug?
  3. Consider the bug unfixable until stage1 is rewritten in Zig?

It would be a bummer to have to wait for a Zig rewrite here. This bug breaks a lot of use cases where you might want to call std.mem.zeroes(T).

@LemonBoy
Copy link
Contributor

This is not the correct way of fixing the problem.
Have a look at the very first few lines of buf_write_value_bytes, every single undefined value is expanded there into (invalid most of the times) ZigValue that's then serialized, tripping that assertion in your case.

Fixing it properly is going to be quite hard but I think the problem could be worked around by:

  • Removing the undefined expansion logic
  • Adding some code to write abi_size zeros (or 0xAA) to buf whenever an undefined value is serialized

@Vexu Vexu added the stage1 The process of building from source via WebAssembly and the C backend. label Jun 13, 2021
@Vexu
Copy link
Member

Vexu commented Jun 21, 2021

I'm going to close this to clear up some older prs as this isn't the correct fix but if you do want to try LemonBoy's suggestion feel free to open a new pr.
Alternatively if you can find a workaround, the current estimate for the self hosted compiler is before the 0.9.0 release, which shoud also fix this.

@Vexu Vexu closed this Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage1 The process of building from source via WebAssembly and the C backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants