Skip to content

Copy elision causes side effects when modifying a struct in place. #4021

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

Open
momumi opened this issue Jan 1, 2020 · 5 comments
Open

Copy elision causes side effects when modifying a struct in place. #4021

momumi opened this issue Jan 1, 2020 · 5 comments
Labels
use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Milestone

Comments

@momumi
Copy link
Contributor

momumi commented Jan 1, 2020

This code attempts to square a complex number i² -> -1. When computing z = z.mul(z), the compiler decides to pass both values z1 and z2 by reference, however copy elision seems to modify z in place causing the incorrect value to be computed.

const std = @import("std");
const print = std.debug.warn;

const Complex = struct {
    x: f32 = 0,
    y: f32 = 0,

    pub fn mul(z1: Complex, z2: Complex) Complex {
        return Complex {
            .x = z1.x*z2.x - z1.y*z2.y,
            .y = z1.x*z2.y + z1.y*z2.x, // new value of .x gets used here
        };
    }
};

pub fn main() void {
    var c = Complex{ .x=0.0, .y=1.0 };
    var i: usize = 0;
    var z: Complex = undefined;

    print("incorrect:\n", .{});
    z = c;
    z = z.mul(z);
    print("z: {d:.2} {d:.2}\n", .{ z.x, z.y });

    z = c;
    z = z.mul(c);
    print("z: {d:.2} {d:.2}\n", .{ z.x, z.y });

    z = c;
    z = c.mul(z);
    print("z: {d:.2} {d:.2}\n", .{ z.x, z.y });

    // These examples produce the correct output
    print("correct:\n", .{});
    z = c.mul(c);
    print("z: {d:.2} {d:.2}\n", .{ z.x, z.y });

    z = c;
    var res = z.mul(z);
    print("z: {d:.2} {d:.2}\n", .{ res.x, res.y });
}

Output:

incorrect:
z: -1.00 -2.00
z: -1.00 -1.00
z: -1.00 -1.00
correct:
z: -1.00 0.00
z: -1.00 0.00

Related issues: #287, #2765, #3804

@ghost
Copy link

ghost commented Jan 2, 2020

Same issue as #3696 (although your code example is a better reduction than mine)

@mikdusan
Copy link
Member

mikdusan commented Apr 3, 2020

I would say to support a solution to aliasing, force copies. Except I only see 2 possibilities, both with their own drawbacks. Maybe we need a way to mark param as "always-copy" ?

drawback: forces callsites to pass addr

pub fn mul(z1: *const Complex, z2: *const Complex) Complex {
    const tmp_z1 = z1.*;
    const tmp_z2 = z2.*;
    return Complex {
        .x = tmp_z1.x*tmp_z2.x - tmp_z1.y*tmp_z2.y,
        .y = tmp_z1.x*tmp_z2.y + tmp_z1.y*tmp_z2.x,
    };
}

drawback: if zig chooses to pass-by-value it is a redundant copy

pub fn mul(z1: Complex, z2: Complex) Complex {
    const tmp_z1 = z1;
    const tmp_z2 = z2;
    return Complex{
        .x = tmp_z1.x * tmp_z2.x - tmp_z1.y * tmp_z2.y,
        .y = tmp_z1.x * tmp_z2.y + tmp_z1.y * tmp_z2.x,
    };
}

@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Apr 8, 2020
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 17, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Nov 6, 2020
@SpexGuy
Copy link
Contributor

SpexGuy commented Dec 8, 2020

Here are some examples that illustrate this problem:

// 1. without a  function
const Vec2 = struct { x: f32, y: f32 };
var v = Vec2{ .x = 4, .y = 6 };
v = .{ .x = -v.y, .y = v.x }; // sets v to (-6, -6)

// 2. with a function
fn rotate(v: Vec2) Vec2 {
    return .{ .x = -v.y, .y = v.x };
}
var v = Vec2{ .x = 4, .y = 6 };
v = rotate(v); // sets v to (-6, -6)

// 3. with a union
const U = union(enum) { a: u32, b: u32 };
var u = U{ .a = 4 };
u = .{ .b = u.a }; // debug panic or UB, accesses bad field

The problem is not limited to function calls, result location semantics plus struct initializers are enough to break things.

@ghost
Copy link

ghost commented Dec 12, 2020

We can solve this without modifying function code by making the copy at the callsite.

@JShorthouse
Copy link

Just got bitten by this as well, here's another example:

const std = @import("std");

const Vector3 = struct {
    x: f32,
    y: f32,
    z: f32,
};

fn vecFunction( vec: *Vector3 ) Vector3 {
    return Vector3{
        .x = vec.x * vec.y * vec.z,
        .y = vec.x * vec.y * vec.z,
        .z = vec.x * vec.y * vec.z,
    };
}

pub fn main() void {
    var vec = Vector3{ .x = 2.0, .y = 2.0, .z = 2.0 };

    vec = vecFunction( &vec );
    std.debug.print("{d}\n", .{vec});
}

Expected to be [ 8, 8, 8 ] but instead prints [ 8, 32, 512 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Projects
None yet
Development

No branches or pull requests

5 participants