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

pointers to "comptime mutable" data should be const pointers outside the comptime scope #1487

Open
tgschultz opened this issue Sep 8, 2018 · 9 comments
Labels
bug Observed behavior contradicts documented or intended behavior frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Milestone

Comments

@tgschultz
Copy link
Contributor

The following crashes on Windows.

const std = @import("std");

var allocator = &std.heap.DirectAllocator.init().allocator;

test "" {
    var memory = allocator.alloc(u8, 1);
}

replacing the allocator line with:

var direct_allocator = std.heap.DirectAllocator.init();
var allocator = &direct_allocator.allocator;

succeeds.

I'm guessing this is because DirectAllocator needs to maintain state on Windows because of the heap handle, and something about the failing test case's construction causes the outer struct to become invalid.

@andrewrk andrewrk added this to the 0.3.0 milestone Sep 8, 2018
@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Sep 8, 2018
@andrewrk
Copy link
Member

Here's a further reduced test case:

const std = @import("std");
const assert = std.debug.assert;

var a = &init().field;

const Foo = struct {
    field: i32,
};

fn init() Foo {
    return Foo{ .field = 1234 };
}

test "oaeu" {
    assert(a.* == 1234);
    a.* += 1;
    assert(a.* == 1235);
}

@andrewrk
Copy link
Member

Ah. What's going on is that the "hidden" global data is valid, but constant. But when we get a pointer to the data, it's a mutable pointer, so Zig lets us attempt to mutate the data. This causes a segfault because the data is in the read only section. So it's a missing compile error. E.g. @typeOf(allocator) in your example should be *const Allocator, not *Allocator. Either that or somehow zig figures out to make the intermediate value a global variable for some reason.

@andrewrk
Copy link
Member

Even simpler:

var a = blk: {
    var x: i32 = undefined;
    break :blk &x;
};

export fn entry() void {
    @compileLog(@typeOf(a));
}

This outputs *i32. It makes sense that the type of &x would be *i32, but once the pointer "escapes" the comptime block scope, the pointer should become const.

@andrewrk andrewrk changed the title "hidden" global data is not always valid? pointers to "comptime mutable" data should be const pointers outside the comptime scope Sep 25, 2018
@andrewrk
Copy link
Member

Postponing to 0.4.0 since there's a reasonable workaround, and the fix is rather involved.

@ghost
Copy link

ghost commented Jul 15, 2020

What if we access this data multiple times in the code? We'd need to instantiate it once for every handover to runtime. Inefficient, and not obvious. I don't think it should be allowed at all. See #5874.

@SpexGuy
Copy link
Contributor

SpexGuy commented Jul 15, 2020

We'd need to instantiate it once for every handover to runtime

This is not true if the data is const at runtime. It exists in one place in the globals section and nothing can change it.

@ghost
Copy link

ghost commented Jul 16, 2020

Consider the following code:

comptime var comptime_data: u32 = 3;
const comppoint = &comptime_data;
var runpoint = comppoint;
var runtime_data = runpoint.*;
comppoint.* = 4;
var runtime_data2 = runpoint.*;
std.debug.print("{}, {}", .{runtime_data, runtime_data2});

What will be the output of print? If there is only one copy of pointed_data at runtime, then it should have the final value at comptime, which is 4 -- however, at the time of runtime_data's assignment, it has the value 3, so we'd need one instance of runpoint which points to 3, and then another for runtime_data2, which points to 4. We'd need two globals, even though we only wrote one variable, and at runtime they have the same name. This is automatic memory management in a fancy suit -- "Oh don't worry about the details, everything just works, but it's not the most efficient".

@andrewrk andrewrk added frontend Tokenization, parsing, AstGen, Sema, and Liveness. and removed stage1 The process of building from source via WebAssembly and the C backend. labels Dec 27, 2022
@andrewrk
Copy link
Member

Promoting to a stage2 issue. Here's a self-contained test case that is expected to pass:

const std = @import("std");

var a = blk: {
    var x: i32 = undefined;
    break :blk &x;
};

test "mutable comptime memory should become const once it leaves scope" {
    try std.testing.expectEqual(*const i32, @TypeOf(a));
}

@ianprime0509
Copy link
Contributor

Given the changes made in #19414, and specifically this test case:

// The pointer constness shouldn't matter - *any* reference to a comptime var is illegal in a global's value.
export const f: *const u32 = f: {
var x: u32 = 123;
break :f &x;
};
Is it safe to say that the expectation mentioned above has changed, and this issue can be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants