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

Pointer to Tagged Union is being silently overwritten #6357

Closed
jamesmcgill opened this issue Sep 16, 2020 · 2 comments
Closed

Pointer to Tagged Union is being silently overwritten #6357

jamesmcgill opened this issue Sep 16, 2020 · 2 comments

Comments

@jamesmcgill
Copy link

In the following code example I am conditionally assigning a pointer to an tagged union in a loop.
The print statement next to the pointer assignment fires only once and with the correct entry in the array.
However somehow it seems to be getting silently assigned with each iteration of the loop and in the end will always unconditionally point to the last element in the array.

The code example is stripped down from my first program written in zig, so apologies if I am making some basic mistake here.

This is using the latest windows build: zig-windows-x86_64-0.6.0+d07383689

const std = @import("std");

const MaterialTag = enum
{
    Lambertian,
    Metal,
};

const Material = union(MaterialTag)
{
    Lambertian: u8,
    Metal: f32,
};

pub fn printMaterial(mat_ptr: *const Material) void
{
    const mat_type = @as(MaterialTag, mat_ptr.*);
    const mat_string = switch (mat_type)
    {
      .Lambertian => "Lambertian",
      .Metal => "Metal",
    };
    std.debug.warn("{}\n", .{mat_string});
}

pub fn main() anyerror!void
{
    const materials = [_]Material
    {
        .{ .Metal = 1.0 },
        .{ .Lambertian = 1 },
        .{ .Metal = 0.0 },  // found_ptr should point to this one
        .{ .Lambertian = 1 }, // but is this one instead
        // .{ .Metal = 1.0 },  // found_ptr is always last array item
    };

    var found : bool = false;
    var found_ptr: *const Material = undefined;
    for (materials) |mat|
    {
        std.debug.warn("Checking: ", .{});
        printMaterial(&mat);
        const isZero: bool = switch (mat)
        {
            MaterialTag.Lambertian => false,
            MaterialTag.Metal => |value| (value == 0.0),
        };
        if (isZero)
        {
            found = true;
            found_ptr = &mat;
            std.debug.warn("  -> found_ptr set to ", .{});
            printMaterial(found_ptr);
        }
    }

    if (found)
    {
        std.debug.warn("\nfound_ptr after loop -> ", .{});
        printMaterial(found_ptr);
    }
}
@rohlem
Copy link
Contributor

rohlem commented Sep 16, 2020

The issue is with lifetimes of temporaries. In particular, the capture of the ranged for loop you're using, mat from for (materials) |mat|, iterates over the elements by immutable value.
The lifetime of mat is exactly one loop iteration, so using its address (stored in found_ptr) after the address is over is undefined behaviour.
To instead iterate over the elements in-place, you can capture by pointer: for (materials) |*mat_ptr|. This instead gives you the original array's elements, in their original location, with original lifetime.
(See the fixed code here: https://godbolt.org/z/qehvK5 )

When passing things by immutable value (in captures as well as function arguments), Zig reserves the right to freely decide whether to implement this by copying them or pointing to them in-place.
In your example case, the compiler chooses to overwrite the temporary mat with the elements of the array one-by-one. It changes intentionally, since you intended to point at the original value, not the temporary.
In different scenarios, I think either decision can feel unintuitive (this particular case was previously changed, following #2915 - see also #5973 for potential future discussion).

... Sorry if I got carried away; use a pointer capture for this, |*mat_ptr| (or even an index capture |_, mat_index|). Hopefully in the future there will be some safety analysis in the compiler that warns you of escaping references to temporaries (at least local to a function). But the program's current misbehaviour is "as intended", sort of.

@jamesmcgill
Copy link
Author

Of course! I'm generating temporary copies in the loop.
Thanks for taking the time to look and explain this and sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants